feat(grpc): add gracefulswitch load balancing policy #2399
Closed
Conversation
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
cjqzhao
force-pushed
the
add-gracefulswitch
branch
2 times, most recently
from
551c605 to
ecd0f18
Compare
dfawley
changed the title
cjqzhao
force-pushed
the
add-gracefulswitch
branch
from
ecd0f18 to
28869a7
Compare
rename mock picker and remove spaces make test picker private
cjqzhao
force-pushed
the
add-gracefulswitch
branch
from
28869a7 to
962fb29
Compare
dfawley
reviewed
Aug 25, 2025
Comment on lines
+46
to
+48
| /** | ||
| Struct for Graceful Switch. | ||
| */ |
Collaborator
There was a problem hiding this comment.
Please write a good docstring for this.
| Struct for Graceful Switch. | ||
| */ | ||
| pub struct GracefulSwitchPolicy { | ||
| subchannel_to_policy: HashMap<WeakSubchannel, ChildKind>, |
Collaborator
There was a problem hiding this comment.
Can we not use the ChildManager for this policy? I was really hoping we could.
The goal was to make it so that no policy would need to manage its own tracking of children.
Ideally graceful switch could do something with the incoming resolver update so that the update sharder would produce 1 or 2 children with the proper configs.
If this is too much to change at the last minute for you, then we can figure out what TODOs to add.
Contributor
Author
There was a problem hiding this comment.
I found it a lot easier to manage children directly when I looked into using ChildManager but I can look into redesigning. The challenge about this is that the ResolverUpdate from shard_update would not be used and there would have to be some other design. I think that changes will need to be made for the Child Manager due to the Child manager forwarding resolver updates to children even when unnecessary. I will draft an appropriate design and get approval from you and Easwar.
Comment on lines
+67
to
+70
| match config.unwrap().convert_to::<Arc<GracefulSwitchLbConfig>>() { | ||
| Ok(cfg) => (*cfg).clone(), | ||
| Err(e) => panic!("convert_to failed: {e}"), | ||
| }; |
Collaborator
There was a problem hiding this comment.
Why the match on the Result instead of .unwrap()?
| if update.service_config.as_ref().is_ok_and(|sc| sc.is_some()) { | ||
| return Err("can't do service configs yet".into()); | ||
| } | ||
| let cfg: Arc<GracefulSwitchLbConfig> = |
Collaborator
There was a problem hiding this comment.
I think you should be able to elide this type?
Comment on lines
+63
to
+65
| if update.service_config.as_ref().is_ok_and(|sc| sc.is_some()) { | ||
| return Err("can't do service configs yet".into()); | ||
| } |
Collaborator
There was a problem hiding this comment.
This seems wrong. Graceful switch shouldn't need to do anything special with service configs. It should just forward it to all children.
Comment on lines
+74
to
+76
| // Determine if we can switch the new policy in. If there is no children | ||
| // yet or the new policy isn't the same as the latest policy, then | ||
| // we can swap. |
Collaborator
There was a problem hiding this comment.
Suggested change
| // Determine if we can switch the new policy in. If there is no children | |
| // yet or the new policy isn't the same as the latest policy, then | |
| // we can swap. | |
| // Determine if we can switch to the new policy. If there is no child | |
| // yet or the new policy isn't the same as the previous policy, then | |
| // we can swap. |
| if needs_switch { | ||
| target_child_kind = self.switch_to(config); | ||
| } | ||
| { |
Contributor
Author
There was a problem hiding this comment.
Due to locking. I wanted to lock the managing_policy just to call resolve_update and then release it for resolve_child_controller.
dfawley
added a commit
to dfawley/tonic
that referenced
this pull request
Nov 6, 2025
General: - Add Debug to many traits and derive/impl in structs. - Pass LB config to LB policies via `Option<LbConfig>` instead of `Option<&LbConfig>`. It should be rare that policies want to store a config except for the leaf policy. Child manager: The original assumption was that all children would be the same type/configuration, but several policies (including gracefulswitch) will not have that property. So, several changes are made: - Children are considered unique by both their identifier and their LbPolicyBuilder's name(). - Make it so the sharder also can shard LbConfig and provide it via the ChildUpdate.child_update field in addition to the ResolverUpdate. - Make ResolverUpdateSharder a generic instead of Box<dyn>. - Add booleans so users of child manager can easily easily tell whether any child policies updated themselves, and which ones did. - Pass &mut self for sharder so that it can maintain and update its state if needed. - Change the sharder's output ChildUpdate.child_update field to an Option; if None then the child will not be called during the resolver update, but will remain in the child manager. - Change child_states into children and provide the whole Child struct, exposing the fields it contains. - Provide mutable access to the sharder. - Minor test cleanups Graceful switch: The previous implementation in hyperium#2399 contained a lot of logic to manage child policy delegation. It was intended that only ChildManager should need to have this kind of logic. - Create a new implementation of this policy that delegates to ChildManager. - Uses a Sharder that simply emits the active policy with no update alongside any new policy in the new LbConfig. - maybe_swap is called after every call into the ChildManager to determine if child updates necessitate a swap. - This logic is simple: if the active policy is not Ready, or if there is a new policy and it is not Connecting, then set the new policy as the active policy and call resolver_update on the ChildManager. The sharder will see that no LbConfig is provided and just emit the active policy with no config, causing the ChildManager to drop the previously active policy. If no swap is needed, update the picker of the active policy if it had an update. - Minor test cleanups/fixes vs. hyperium#2399.
dfawley
added a commit
to dfawley/tonic
that referenced
this pull request
Nov 6, 2025
General: - Add Debug to many traits and derive/impl in structs. - Pass LB config to LB policies via `Option<LbConfig>` instead of `Option<&LbConfig>`. It should be rare that policies want to store a config except for the leaf policy. Child manager: The original assumption was that all children would be the same type/configuration, but several policies (including gracefulswitch) will not have that property. So, several changes are made: - Children are considered unique by both their identifier and their LbPolicyBuilder's name(). - Make it so the sharder also can shard LbConfig and provide it via the ChildUpdate.child_update field in addition to the ResolverUpdate. - Make ResolverUpdateSharder a generic instead of Box<dyn>. - Add booleans so users of child manager can easily easily tell whether any child policies updated themselves, and which ones did. - Pass &mut self for sharder so that it can maintain and update its state if needed. - Change the sharder's output ChildUpdate.child_update field to an Option; if None then the child will not be called during the resolver update, but will remain in the child manager. - Change child_states into children and provide the whole Child struct, exposing the fields it contains. - Provide mutable access to the sharder. - Minor test cleanups Graceful switch: The previous implementation in hyperium#2399 contained a lot of logic to manage child policy delegation. It was intended that only ChildManager should need to have this kind of logic. - Create a new implementation of this policy that delegates to ChildManager. - Uses a Sharder that simply emits the active policy with no update alongside any new policy in the new LbConfig. - maybe_swap is called after every call into the ChildManager to determine if child updates necessitate a swap. - This logic is simple: if the active policy is not Ready, or if there is a new policy and it is not Connecting, then set the new policy as the active policy and call resolver_update on the ChildManager. The sharder will see that no LbConfig is provided and just emit the active policy with no config, causing the ChildManager to drop the previously active policy. If no swap is needed, update the picker of the active policy if it had an update. - Minor test cleanups/fixes vs. hyperium#2399.
dfawley
added a commit
to dfawley/tonic
that referenced
this pull request
Nov 6, 2025
General: - Add Debug to many traits and derive/impl in structs. - Pass LB config to LB policies via `Option<LbConfig>` instead of `Option<&LbConfig>`. It should be rare that policies want to store a config except for the leaf policy. Child manager: The original assumption was that all children would be the same type/configuration, but several policies (including gracefulswitch) will not have that property. So, several changes are made: - Children are considered unique by both their identifier and their LbPolicyBuilder's name(). - Make it so the sharder also can shard LbConfig and provide it via the ChildUpdate.child_update field in addition to the ResolverUpdate. - Make ResolverUpdateSharder a generic instead of Box<dyn>. - Add booleans so users of child manager can easily easily tell whether any child policies updated themselves, and which ones did. - Pass &mut self for sharder so that it can maintain and update its state if needed. - Change the sharder's output ChildUpdate.child_update field to an Option; if None then the child will not be called during the resolver update, but will remain in the child manager. - Change child_states into children and provide the whole Child struct, exposing the fields it contains. - Provide mutable access to the sharder. - Change the LB config to be a flat JSON array to facilitate use within another LB policy that should not need a struct to contain on the children. - Minor test cleanups Graceful switch: The previous implementation in hyperium#2399 contained a lot of logic to manage child policy delegation. It was intended that only ChildManager should need to have this kind of logic. - Create a new implementation of this policy that delegates to ChildManager. - Uses a Sharder that simply emits the active policy with no update alongside any new policy in the new LbConfig. - maybe_swap is called after every call into the ChildManager to determine if child updates necessitate a swap. - This logic is simple: if the active policy is not Ready, or if there is a new policy and it is not Connecting, then set the new policy as the active policy and call resolver_update on the ChildManager. The sharder will see that no LbConfig is provided and just emit the active policy with no config, causing the ChildManager to drop the previously active policy. If no swap is needed, update the picker of the active policy if it had an update. - Minor test cleanups/fixes vs. hyperium#2399.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Add graceful switch load balancing policy.
@dfawley @easwars