endpointsharding, lazy: Remove intermediary gracefulswitch balancers #8052
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8052 +/- ##
==========================================
+ Coverage 82.29% 82.31% +0.01%
==========================================
Files 384 388 +4
Lines 38926 39028 +102
==========================================
+ Hits 32034 32124 +90
- Misses 5565 5577 +12
Partials 1327 1327
|
arjan-bal
force-pushed
the
avoid-registering-lazy
branch
from
83370dd to
0bc66fd
Compare
dfawley
left a comment
dfawley
left a comment
There was a problem hiding this comment.
Mostly looks fine, but the minor API thing should be decided.
| // UpdateChildState in its UpdateClientConnState. | ||
| return crr.Balancer.UpdateClientConnState(balancer.ClientConnState{ | ||
| BalancerConfig: gracefulSwitchPickFirst, | ||
| BalancerConfig: nil, // pickfirst can handle nil configs. |
There was a problem hiding this comment.
Delete the field entirely to remove complications from example?
There was a problem hiding this comment.
Removed throughout.
| // picker update. | ||
| return b.child.UpdateClientConnState(balancer.ClientConnState{ | ||
| BalancerConfig: endpointShardingLBConfig, | ||
| BalancerConfig: nil, // pickfirst can handle nil configs. |
There was a problem hiding this comment.
And then I'd delete it from all our code to avoid clutter. Most LB policies should handle not having configs, except some special xds things.
| // NewBalancer returns a load balancing policy that manages homogeneous child | ||
| // policies each owning a single endpoint. The endpointsharding balancer | ||
| // forwards the LoadBalancingConfig in ClientConn state updates to its children. | ||
| func NewBalancer(cc balancer.ClientConn, opts balancer.BuildOptions, childBuilder balancer.Builder, esOpts Options) balancer.Balancer { |
There was a problem hiding this comment.
I'm wondering about the signature here.
There's a lot of options:
- Keep this & make anyone wanting to use es+lazy wrap lazy in a Builder implementation.
- Keep this & implement a Builder in
lazy, still. - Create
type ChildBuilder interface { Build(..) .. }withoutName()and makelazyimplement that. - Accept a function matching the signature of
Buildhere.
None of these are great. I think I might like (4) the best for usability? But I dislike it for understandability. I suppose that could be helped with:
type ChildBuilder = func(...) ... // matching Build?
There was a problem hiding this comment.
Changed to use a type alias. Didn't export the ChildBuilder type since it's only an alias.
There was a problem hiding this comment.
Updated to use a type declaration instead of an alias.
balancer/lazy/lazy.go
Outdated
|
|
||
| func (builder) Build(cc balancer.ClientConn, bOpts balancer.BuildOptions) balancer.Balancer { | ||
| // NewBalancer is the constructor for the lazy balancer. | ||
| func NewBalancer(cc balancer.ClientConn, bOpts balancer.BuildOptions, childBuilder balancer.Builder) balancer.Balancer { |
There was a problem hiding this comment.
The same concerns as above apply here, too.
There was a problem hiding this comment.
Changed, using an alias to the type of the balancer.Build method now.
balancer/lazy/lazy.go
Outdated
|
|
||
| // childBuilderFunc creates a new balancer with the ClientConn. It has the same | ||
| // type as the balancer.Builder.Build method. | ||
| type childBuilderFunc = func(cc balancer.ClientConn, opts balancer.BuildOptions) balancer.Balancer |
There was a problem hiding this comment.
This really should be exported, otherwise you expose unexported symbols. The user would have to look at the source code to learn how to use it.
Also I'm completely unclear what the difference is in making this a type alias vs. just a type definition (w/ vs. w/o the =).
There was a problem hiding this comment.
A type alias provides a new name for an existing type without introducing a new type that has a different identity. So the unexported childBuilderFunc isn't its own type and doesn't need to be exposed to package users. The language server expands the childBuilderFunc type to func(cc balancer.ClientConn, opts balancer.BuildOptions) balancer.Balancer.
Using a type declaration introduces a new type. For a type declaration, it makes sense to export the type because external users need to create variables of the type to call this function. The language server also mentions the type now.
There was a problem hiding this comment.
Changing to a type declaration and exporting the type. The benefit is the shorter function signature and the godoc of the type being visible.
arjan-bal
force-pushed
the
avoid-registering-lazy
branch
from
5104d5c to
dc2ffa5
Compare
2 participants
Presently both
lazyandendpointshardingcreategracefulswtichbalancers to manage their children. Thegracefulswitchbalancer is unnecessary as the type of the child balancer in not expected to change in existing use cases. Having thegracefulswitchchild requires the child balancer's builder to be registered in the global LB registry. ThelazyLB is not a complete LB policy and should not be registered globally.This PR makes the constructors of
endpointpointshardingandlazyaccept the child balancer's builder to avoid using the global registry. This PR also removes theParseConfigfunctions/methods inlazyandendpointshardingas they will simply forward theLoadBalancingConfigs to their children. The children are responsible to parse theLoadBalancingConfigJSON. Sincepickfirstcan use anilLoadBalancingConfig, the existing users ofendpointshardingdon't need to provide aLoadBalancingConfig.RELEASE NOTES:
DisableAutoReconnectoption will not attempt to callExitIdleautomatically on their children when the children report idle.