[client channel] combine two subchannel data structures into one#40880
Closed
markdroth wants to merge 2 commits intogrpc:masterfrom
Closed
[client channel] combine two subchannel data structures into one#40880markdroth wants to merge 2 commits intogrpc:masterfrom
markdroth wants to merge 2 commits intogrpc:masterfrom
Conversation
…mbine_subchannel_maps
Member
|
The changes seem reasonable, but I'd like to explore testing/experiment status: I'm not convinced there's sufficient testing right now to call this safe. |
Member
Author
|
Is there a specific testing gap you're worried about on this one? The only two features affected by these data structures are:
So it looks to me like we do have coverage here. |
Member
Author
|
I chatted with @ctiller, and we agreed that this can go forward, but he didn't have time to review before his vacation. @murgatroid99, can you please review? Thanks! |
murgatroid99
approved these changes
Nov 7, 2025
yuanweiz
pushed a commit
to yuanweiz/grpc
that referenced
this pull request
Nov 12, 2025
…c#40880) Currently, the client channel maintains two different data structures for tracking subchannels: - `subchannel_refcount_map_`, which tracks the number of subchannel wrappers per subchannel. This is used to determine when to create and remove channelz node linkage. - `subchannel_wrappers_`, which tracks the set of all subchannel wrappers across all subchannels. This was originally introduced back in grpc#20039 as part of propagating the health check service name to subchannels, but we switched to using a different approach for the health check service name in grpc#26441. However, in between those two, we started using this data structure for handling keepalive time in grpc#23313 -- which is actually somewhat inefficient, since we may wind up setting the keepalive time on the underlying subchannel more than once in the case where there is more than one subchannel wrapper per subchannel (which happens frequently during updates). This PR combines these two data structures into one: a map from subchannel to a set of subchannel wrappers for that subchannel. This is used both for channelz node updates and keepalive propagation -- and it's more efficient for the latter, because we can now update each subchannel exactly once. This also paves the way for a subsequent change that will be needed as part of the MAX_CONCURRENT_STREAMS design. Closes grpc#40880 COPYBARA_INTEGRATE_REVIEW=grpc#40880 from markdroth:client_channel_combine_subchannel_maps f289c20 PiperOrigin-RevId: 829602334
sreenithi
pushed a commit
to sreenithi/grpc
that referenced
this pull request
Nov 26, 2025
…c#40880) Currently, the client channel maintains two different data structures for tracking subchannels: - `subchannel_refcount_map_`, which tracks the number of subchannel wrappers per subchannel. This is used to determine when to create and remove channelz node linkage. - `subchannel_wrappers_`, which tracks the set of all subchannel wrappers across all subchannels. This was originally introduced back in grpc#20039 as part of propagating the health check service name to subchannels, but we switched to using a different approach for the health check service name in grpc#26441. However, in between those two, we started using this data structure for handling keepalive time in grpc#23313 -- which is actually somewhat inefficient, since we may wind up setting the keepalive time on the underlying subchannel more than once in the case where there is more than one subchannel wrapper per subchannel (which happens frequently during updates). This PR combines these two data structures into one: a map from subchannel to a set of subchannel wrappers for that subchannel. This is used both for channelz node updates and keepalive propagation -- and it's more efficient for the latter, because we can now update each subchannel exactly once. This also paves the way for a subsequent change that will be needed as part of the MAX_CONCURRENT_STREAMS design. Closes grpc#40880 COPYBARA_INTEGRATE_REVIEW=grpc#40880 from markdroth:client_channel_combine_subchannel_maps f289c20 PiperOrigin-RevId: 829602334
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently, the client channel maintains two different data structures for tracking subchannels:
subchannel_refcount_map_, which tracks the number of subchannel wrappers per subchannel. This is used to determine when to create and remove channelz node linkage.subchannel_wrappers_, which tracks the set of all subchannel wrappers across all subchannels. This was originally introduced back in Apply health check service name changes to existing subchannels. #20039 as part of propagating the health check service name to subchannels, but we switched to using a different approach for the health check service name in pass health check service name through LB policies via a channel arg #26441. However, in between those two, we started using this data structure for handling keepalive time in Keepalive Throttling #23313 -- which is actually somewhat inefficient, since we may wind up setting the keepalive time on the underlying subchannel more than once in the case where there is more than one subchannel wrapper per subchannel (which happens frequently during updates).This PR combines these two data structures into one: a map from subchannel to a set of subchannel wrappers for that subchannel. This is used both for channelz node updates and keepalive propagation -- and it's more efficient for the latter, because we can now update each subchannel exactly once.
This also paves the way for a subsequent change that will be needed as part of the MAX_CONCURRENT_STREAMS design.