core, grpclb,xds: let leaf LB policies explicitly refresh name resolution when subchannel connection is broken #8048
Conversation
This file contains 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
…is broken, let LB policies handle it.
…c in round_robin.
…x/move_subchannel_refreshing_ns_to_lb
…c in ring hash LB policy.
voidzcy
changed the title
ejona86
reviewed
Apr 9, 2021
…l's refresh for cases LoadBalancer intentionally does not want a refresh.
…when handling subchannel state changes. Log a warning and do a refresh if it is not.
… a name resolution refresh for the resolver in Channel, so use ignoreRefreshNameResolutionCheck() to avoid false-positive warnings.
voidzcy
force-pushed
the
bugfix/move_subchannel_refreshing_ns_to_lb
branch
from
9256ec1
to
2ff23c6
Compare
Fixed by adding a flag. The Channel will still do the automatic refresh if the LoadBalancer did not do so. PTAL. |
ejona86
approved these changes
Apr 15, 2021
There was a problem hiding this comment.
Just to help check that we've updated our code appropriately, replace the warning with an exception/panic and run the tests to make sure they pass. After a few releases with the warning, I wouldn't be surprised if we want to do something similar for a grpc-java release.
…o no-op, update its doc and not mark it as deprecated for now.
… for OOB channel state changes.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Currently each subchannel implicitly refreshes the name resolution when its connection is broken. That is, this feature is built into subchannel's internal implementation. Although it eliminates the burden of having LB implementations refreshing the resolver when connections to backends are broken, this is gives LB policies no chance to disable or override this refresh (e.g., in some complex load balancing hierarchy like xDS, LB policies may embed a resolver inside for resolving backends so the refreshing resolution operation should be hooked to the resolver embedded in the LB policy instead of the one in Channel).
This is likely to be a breaking change for users implementing their own LB policies (performing load balancing directly on backends). In order to make this transition smoothly, we add a check to SubchannelImpl that checks if the LoadBalancer has explicitly called
Helper.refreshNameResolutionfor broken subchannels created by it. If not, it logs a warning and do the refresh.A temporary LoadBalancer.Helper API
ignoreRefreshNameResolution()is added to avoid false-positive warnings for xDS that intentionally does not want a refresh. Once the migration is done, this should be deleted.See details in #8088.