Prefer FixedResultPicker over custom Picker implementations#12590
Merged
ejona86 merged 2 commits intogrpc:masterfrom Jan 5, 2026
Merged
Prefer FixedResultPicker over custom Picker implementations#12590ejona86 merged 2 commits intogrpc:masterfrom
ejona86 merged 2 commits intogrpc:masterfrom
Conversation
ejona86
force-pushed
the
use-fixedresultpicker
branch
from
13240f3 to
9287f85
Compare
Member
Author
|
@kannanjgithub, FYI, the second commit here ("Fix grpclb incompatibility") ends up fixing an RLS metric bug. See the "Fix RLS test" for a description of the bug. |
shivaspeaks
approved these changes
Jan 4, 2026
kannanjgithub
approved these changes
Jan 5, 2026
These other implementations pre-date FixedResultPicker, and are no longer needed. gRPC-LB's tests failed when using FixedResultPicker because it didn't transition to READY. This was because GrpclbState.maybeUpdatePicker() didn't consider the ConnectivityState when checking if anything had changed. The old PickFirstLoadBalancer.Picker didn't implement equals() so every update was considered different, ignoring the connectivity state. PickFirstLeafLoadBalancer wasn't impacted because it didn't pick a subchannel when in CONNECTING, and neither did PickFirstLoadBalancer after the first update. This is fixed by gRPC-LB checking the connectivity state. A follow-up will have PickFirstLoadBalancer no longer return the useless subchannel when picking during CONNECTING.
This is a follow-up from noticing a breakage in gRPC-LB because it wasn't checking if the connectivity state changed even if the picker was identical. lb_serverStatusCodeConversion() has been misleading since 42e1829 ("xds: Do RLS fallback policy eagar start"). At that point, the subchannel it marked as READY was for the default target's policy, not the policy for wilderness. However, since old PF policy provided a subchannel when CONNECTING, everything was "fine", but RLS would mistakenly count toward target_picks. This demonstrates that RLS target_picks has been broken since it was introduced for PF, as PF relied on the caller to avoid the picker when it was CONNECTING. This may have been hard to notice in production, as the metrics become correct as soon as the connection is established, so as long as you use the channel for a while, the duplicate counting would become a small percentage of the overall amount.
ejona86
force-pushed
the
use-fixedresultpicker
branch
from
faf0add to
c486dd0
Compare
Member
Author
|
Instead of squashing this into a single commit, I split the middle commit in two; the first half merged with the first commit and the second half merged with the third commit. That makes the commit messages clearer. But I didn't make any changes to the code from the earlier three-commit version. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These other implementations pre-date FixedResultPicker, and are no longer needed.