-
Notifications
You must be signed in to change notification settings - Fork 4.6k
pickfirstleaf: Fix shuffling of addresses in resolver updates without endpoints #8610
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8610 +/- ##
==========================================
+ Coverage 82.11% 82.16% +0.04%
==========================================
Files 415 415
Lines 40692 40697 +5
==========================================
+ Hits 33416 33439 +23
+ Misses 5887 5881 -6
+ Partials 1389 1377 -12
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| pfBuilder := balancer.Get(pfbalancer.Name) | ||
| shffleConfig, err := pfBuilder.(balancer.ConfigParser).ParseConfig(json.RawMessage(`{ "shuffleAddressList": true }`)) |
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.
Lol, here and in the next statement, prefer adding the u back into shuffle. Saving one character in the variable name is not going to be very useful :)
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.
Changed to using complete words.
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| activeCfg := noShffleConfig |
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.
Nit: remove this line since this assignment is anyways happening on line 521.
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.
Changed to only declaring the variable, but not initializing here.
… endpoints (grpc#8610) The new `pick_first`, which is the default, doesn't shuffle the addresses at all for resolver updates that are missing the `Endpoints` field. This change fixes that. Since [gRPC automatically sets the the missing `Endpoints`](https://github.com/grpc/grpc-go/blob/1059e84f885bf7ed65b3b1a4fbe914360d8ab5b1/resolver_wrapper.go#L136-L138), occurrence of this bug should be uncommon in practice. RELEASE NOTES: * balancer/pick_first: When configured, shuffle addresses in resolver updates that lack endpoints. Since gRPC automatically adds endpoints to resolver updates, this bug should only affect implementers of custom LB policies that use pick_first for delegation but don't forward the endpoints.
Original PRs: #8610, #8615 RELEASE NOTES: * balancer/pick_first: * Fix bug that can cause balancer to get stuck in `IDLE` state on backend address change. * When configured, shuffle addresses in resolver updates that lack endpoints. Since gRPC automatically adds endpoints to resolver updates, this bug should only affect implementers of custom LB policies that use pick_first for delegation but don't forward the endpoints.
… endpoints (grpc#8610) The new `pick_first`, which is the default, doesn't shuffle the addresses at all for resolver updates that are missing the `Endpoints` field. This change fixes that. Since [gRPC automatically sets the the missing `Endpoints`](https://github.com/grpc/grpc-go/blob/1059e84f885bf7ed65b3b1a4fbe914360d8ab5b1/resolver_wrapper.go#L136-L138), occurrence of this bug should be uncommon in practice. RELEASE NOTES: * balancer/pick_first: When configured, shuffle addresses in resolver updates that lack endpoints. Since gRPC automatically adds endpoints to resolver updates, this bug should only affect implementers of custom LB policies that use pick_first for delegation but don't forward the endpoints.
The new
pick_first, which is the default, doesn't shuffle the addresses at all for resolver updates that are missing theEndpointsfield. This change fixes that. Since gRPC automatically sets the the missingEndpoints, occurrence of this bug should be uncommon in practice.RELEASE NOTES: