endpointsharding, lazy: Remove intermediary gracefulswitch balancers #8052

arjan-bal · 2025-01-30T10:50:41Z

Presently both lazy and endpointsharding create gracefulswtich balancers to manage their children. The gracefulswitch balancer is unnecessary as the type of the child balancer in not expected to change in existing use cases. Having the gracefulswitch child requires the child balancer's builder to be registered in the global LB registry. The lazy LB is not a complete LB policy and should not be registered globally.

This PR makes the constructors of endpointpointsharding and lazy accept the child balancer's builder to avoid using the global registry. This PR also removes the ParseConfig functions/methods in lazy and endpointsharding as they will simply forward the LoadBalancingConfigs to their children. The children are responsible to parse the LoadBalancingConfig JSON. Since pickfirst can use a nil LoadBalancingConfig, the existing users of endpointsharding don't need to provide a LoadBalancingConfig.

RELEASE NOTES:

balancer/endpointsharding: The constructor accepts the child balancer's builder and a struct with optional configuration.
balancer/endpointsharding: Balancers created with the new DisableAutoReconnect option will not attempt to call ExitIdle automatically on their children when the children report idle.

codecov · 2025-01-30T10:55:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.31%. Comparing base (b0e2ae9) to head (dc2ffa5).
Report is 12 commits behind head on master.

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

Files with missing lines	Coverage Δ
balancer/endpointsharding/endpointsharding.go	`77.40% <100.00%> (+0.27%)`	⬆️
balancer/lazy/lazy.go	`81.53% <100.00%> (+7.11%)`	⬆️
balancer/leastrequest/leastrequest.go	`88.69% <100.00%> (ø)`
balancer/weightedroundrobin/balancer.go	`83.38% <100.00%> (-0.10%)`	⬇️

... and 47 files with indirect coverage changes

dfawley

Mostly looks fine, but the minor API thing should be decided.

dfawley · 2025-01-30T22:04:59Z

examples/features/customloadbalancer/client/customroundrobin/customroundrobin.go

@@ -100,7 +99,7 @@ func (crr *customRoundRobin) UpdateClientConnState(state balancer.ClientConnStat
 	// is guaranteed to happen since the aggregator will always call
 	// UpdateChildState in its UpdateClientConnState.
 	return crr.Balancer.UpdateClientConnState(balancer.ClientConnState{
-		BalancerConfig: gracefulSwitchPickFirst,
+		BalancerConfig: nil, // pickfirst can handle nil configs.


Delete the field entirely to remove complications from example?

Removed throughout.

dfawley · 2025-01-30T22:05:49Z

balancer/weightedroundrobin/balancer.go

@@ -236,7 +233,7 @@ func (b *wrrBalancer) UpdateClientConnState(ccs balancer.ClientConnState) error
 	// This causes child to update picker inline and will thus cause inline
 	// picker update.
 	return b.child.UpdateClientConnState(balancer.ClientConnState{
-		BalancerConfig: endpointShardingLBConfig,
+		BalancerConfig: nil, // pickfirst can handle nil configs.


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.

dfawley · 2025-01-30T22:12:05Z

balancer/endpointsharding/endpointsharding.go

+// 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 {


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(..) .. } without Name() and make lazy implement that.

Accept a function matching the signature of Build here.

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

?

Changed to use a type alias. Didn't export the ChildBuilder type since it's only an alias.

Updated to use a type declaration instead of an alias.

dfawley · 2025-01-30T22:12:41Z

balancer/lazy/lazy.go

-
-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 {


The same concerns as above apply here, too.

Changed, using an alias to the type of the balancer.Build method now.

dfawley · 2025-01-31T18:30:03Z

balancer/lazy/lazy.go

@@ -45,8 +45,12 @@ const (
 	logPrefix = "[lazy-lb %p] "
 )

+// 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


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 =).

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.

Changing to a type declaration and exporting the type. The benefit is the shorter function signature and the godoc of the type being visible.

…rpc#8052)

avoid registering lazy

46ba28c

arjan-bal added the Type: Internal Cleanup Refactors, etc label Jan 30, 2025

arjan-bal added this to the 1.71 Release milestone Jan 30, 2025

arjan-bal requested a review from dfawley January 30, 2025 10:50

arjan-bal assigned dfawley Jan 30, 2025

Remove graceful switch from lazy and fix endpointsharding usages

0bc66fd

arjan-bal force-pushed the avoid-registering-lazy branch from 83370dd to 0bc66fd Compare January 30, 2025 11:13

dfawley mentioned this pull request Jan 30, 2025

ringhash: Delegate subchannel creation to pickfirst #8047

Merged

dfawley reviewed Jan 30, 2025

View reviewed changes

dfawley assigned arjan-bal and unassigned dfawley Jan 30, 2025

Address review comments

12d3752

arjan-bal assigned dfawley and unassigned arjan-bal Jan 31, 2025

dfawley reviewed Jan 31, 2025

View reviewed changes

dfawley approved these changes Jan 31, 2025

View reviewed changes

Change alias to type declarition

dc2ffa5

arjan-bal force-pushed the avoid-registering-lazy branch from 5104d5c to dc2ffa5 Compare February 3, 2025 05:58

arjan-bal merged commit 990f5e0 into grpc:master Feb 3, 2025
14 of 15 checks passed

arjan-bal deleted the avoid-registering-lazy branch February 3, 2025 06:51

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Feb 15, 2025

endpointsharding, lazy: Remove intermediary gracefulswitch balancers (g…

d9821c9

…rpc#8052)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

endpointsharding, lazy: Remove intermediary gracefulswitch balancers #8052

endpointsharding, lazy: Remove intermediary gracefulswitch balancers #8052

arjan-bal commented Jan 30, 2025 •

edited

Loading

codecov bot commented Jan 30, 2025 •

edited

Loading

dfawley left a comment

dfawley Jan 30, 2025

arjan-bal Jan 31, 2025

dfawley Jan 30, 2025

arjan-bal Jan 31, 2025

dfawley Jan 30, 2025

arjan-bal Jan 31, 2025

arjan-bal Feb 3, 2025

dfawley Jan 30, 2025

arjan-bal Jan 31, 2025

dfawley Jan 31, 2025

arjan-bal Feb 3, 2025 •

edited

Loading

arjan-bal Feb 3, 2025

endpointsharding, lazy: Remove intermediary gracefulswitch balancers #8052

endpointsharding, lazy: Remove intermediary gracefulswitch balancers #8052

Conversation

arjan-bal commented Jan 30, 2025 • edited Loading

codecov bot commented Jan 30, 2025 • edited Loading

Codecov Report

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjan-bal Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjan-bal commented Jan 30, 2025 •

edited

Loading

codecov bot commented Jan 30, 2025 •

edited

Loading

arjan-bal Feb 3, 2025 •

edited

Loading