-
Notifications
You must be signed in to change notification settings - Fork 360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(api): add consistent hash table size api #3348
feat(api): add consistent hash table size api #3348
Conversation
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
api/v1alpha1/loadbalancer_types.go
Outdated
|
||
// The table size for consistent hashing, must be prime number limited to 5000011. | ||
// | ||
// +kubebuilder:validation:Minimum=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make it 2
? because 1
is actually not a prime number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, make the minimum to the smallest prime number 2
now.
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
@@ -58,7 +58,7 @@ type ConsistentHash struct { | |||
|
|||
// The table size for consistent hashing, must be prime number limited to 5000011. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add CEL to validate it must be prime number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like CEL in Kubernetes 1 doesn't support complex validation logic like IsPrime
function. But we can validate whether table size is prime number when translating ClientTrafficPolicy to IR.
func IsPrime(n int) bool {
if n <= 1 {
return false
}
for i := 2; i*i <= n; i++ {
if n%i == 0 {
return false
}
}
return true
}
Footnotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for adding this! Leave some comments.
@@ -58,7 +58,7 @@ type ConsistentHash struct { | |||
|
|||
// The table size for consistent hashing, must be prime number limited to 5000011. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks !
* Add consistent hash table size api Signed-off-by: Dingkang Li <dingkang1743@gmail.com> * Set consistent hash table size's minimum as the smallest prime number: 2 Signed-off-by: Dingkang Li <dingkang1743@gmail.com> * Comment tableSize api as notImplementedHide Signed-off-by: Dingkang Li <dingkang1743@gmail.com> --------- Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
What this PR does / why we need it:
Add knob in BackendTrafficPolicy to configure consistent hash table size.
Which issue(s) this PR fixes:
Fixes #3272