xds/ringhash: cache connectivity state of subchannels inside picker #6351
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
zasweq
approved these changes
Jun 6, 2023
| case connectivity.TransientFailure: | ||
| // Save error to be reported via picker. | ||
| b.connErr = state.ConnectionError | ||
| b.regeneratePicker() | ||
| case connectivity.Shutdown: | ||
| // When an address was removed by resolver, b called RemoveSubConn but | ||
| // kept the sc's state in scStates. Remove state for this sc here. | ||
| delete(b.scStates, sc) |
There was a problem hiding this comment.
Nit: Not a part of this PR, but this scStates field I feel like is wrongly named. This is a map from balancer.SubConn to the Ring Hashes SubConn with extra info. I feel like states is only a part of the reason this map exists.
There was a problem hiding this comment.
Yeah it probably was changed during development. 99% of why there exists a map is state tracking. It also does a queueConnect in rare cases.
| case connectivity.Shutdown: | ||
| // When an address was removed by resolver, b called RemoveSubConn but | ||
| // kept the sc's state in scStates. Remove state for this sc here. | ||
| delete(b.scStates, sc) | ||
| } | ||
|
|
||
| if sendUpdate { | ||
| if oldSCState != newSCState { | ||
| // Because the picker caches the state of the subconns, we always regnerate |
| if sendUpdate { | ||
| if oldSCState != newSCState { | ||
| // Because the picker caches the state of the subconns, we always regnerate | ||
| // and update the picker when the effective state changes. |
There was a problem hiding this comment.
Nit: "of the SubConn." Optional addition: "This is so the Client Conn gets the most updated state as a former picker has a static cache of SubConn states at it's build time."
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.
I did not add a new test for this. The sequence of operations required here is fairly long and specific and requires races that are too difficult to reliably stimulate. Corresponding change in C++: grpc/grpc@
98cdac3(#32326). (gRPC-Java always kept a copy of the states in the picker.)RELEASE NOTES: