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
do not add previous hosts predicate when consistent hashing is enabled #50759
do not add previous hosts predicate when consistent hashing is enabled #50759
Conversation
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@@ -73,7 +90,6 @@ func ConvertPolicy(in *networking.HTTPRetry) *route.RetryPolicy { | |||
} | |||
|
|||
// A policy was specified. Start with the default and override with user-provided fields where appropriate. | |||
out := DefaultPolicy() |
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.
donot we need to distinguish hash here
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.
it is moved up and handle both cases
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.
IC, nvm
if len(in.Route) == 1 { | ||
hostnames = append(hostnames, processDestination(in.Route[0], serviceRegistry, listenerPort, hashByDestination, out, action)) | ||
hash := hashByDestination[in.Route[0]] | ||
consistentHash = hash != nil |
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.
How do you handle consitent hash for the below else branch
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.
consistent hash and weighted desitnations do not work together
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.
do we have validation that checked it
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.
I do not think we have - but we should add IMO (atleast warn)
@hzxuzhonghu can you PTAL when you get chance? |
@howardjohn @hzxuzhonghu Can you PTAL when you get chance? |
@hzxuzhonghu Can you PTAL when you get chance? |
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
Fixes #50756