endpointsharding: Allow children to remain idle if configured

[arjan-bal]

This change allows the children of endpointsharding to remain in IDLE state. This allows lazy initialization of children and therefore lazy creation of subchannels for endpoints. This enables the usage of endpointsharding in the ringhash balancer for managing pickfirst children.

RELEASE NOTES: N/A

[codecov]

Codecov Report

Attention: Patch coverage is 89.18919% with 4 lines in your changes missing coverage. Please review.

Project coverage is 82.26%. Comparing base (67bee55) to head (930c996).
Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
balancer/endpointsharding/endpointsharding.go	89.18%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8031      +/-   ##
==========================================
+ Coverage   82.22%   82.26%   +0.03%     
==========================================
  Files         383      384       +1     
  Lines       38688    38926     +238     
==========================================
+ Hits        31813    32024     +211     
- Misses       5555     5575      +20     
- Partials     1320     1327       +7     

Files with missing lines	Coverage Δ
balancer/endpointsharding/endpointsharding.go	77.12% <89.18%> (+0.96%)	⬆️

... and 44 files with indirect coverage changes

[dfawley]

Can we move this into balancerWrapper's Close method?

Also since calls into a balancer.Balancer must not happen concurrently, I don't think it requires a lock.

[arjan-bal]

Added a Close method and moved this into the Close method.

[dfawley]

Because we're calling into the child from a goroutine here:

• All calls into the child need to grab bw.es.childMu to avoid concurrent calls in,
• That means Close() needs it (and also needed it before you added the method explicitly here, so good that we caught it), and
• That means we shouldn't embed the child Balancer, since directly calling it is wrong.

[arjan-bal]

That means Close() needs it (and also needed it before you added the method explicitly here, so good that we caught it)

Before this PR, calls to balancerWrapper.Close() and updates to endpointsharding.closedwere holding es.ChildMu:
In endpointshardng.Close():

grpc-go/balancer/endpointsharding/endpointsharding.go

Line 187 in 9d4fa67

func (es *endpointSharding) Close() {

In endpointshardng.UpdateClientConnState():

grpc-go/balancer/endpointsharding/endpointsharding.go

Lines 104 to 105 in 9d4fa67

	es.childMu.Lock()
	defer es.childMu.Unlock()

grpc-go/balancer/endpointsharding/endpointsharding.go

Lines 150 to 157 in 9d4fa67

	// Delete old children that are no longer present.
	for _, e := range children.Keys() {
	child, _ := children.Get(e)
	bal := child.(balancer.Balancer)
	if _, ok := newChildren.Get(e); !ok {
	bal.Close()
	}
	}

Maybe I'm not understanding this point correctly?

That means we shouldn't embed the child Balancer, since directly calling it is wrong.

I've removed the embedded balancer.Balancer from balancerWrapper and provided methods named updateClientConnStateLocked and closeLocked and that expect the caller to lock es.ChildMu.

[dfawley]

Before this PR, calls to balancerWrapper.Close() and updates to endpointsharding.closed were holding es.ChildMu:

Oh, I see... I missed where it grabbed the lock in Close and thought it was just delegating directly to the wrappers.

This now seems good and makes it explicit what is required.

[dfawley]

Suggested change

	// mu synchronizes access to the stored children balancer states.
	// mu synchronizes access to the stored children balancer states in children.

?

Or "in the children field" or something to be more specific.

[dfawley]

Could you rename this local to childBalancerWrapper or something more clear -- at least child or something? bal to me makes me think this is the endpointsharding balancer itself when I see it later on. I guess we use es for that, but the more common convention is that the Balancer implementation within the package is simply named balancer, and b is the receiver.

[dfawley]

Similar nit about the name of things.. This local isn't even necessary:

child, _ := children.Get(e)
if _, ok := newChildren.Get(e); !ok {
	child.(*balancerWrapper).closeLocked()
}

[dfawley]

Before this PR, calls to balancerWrapper.Close() and updates to endpointsharding.closed were holding es.ChildMu:

Oh, I see... I missed where it grabbed the lock in Close and thought it was just delegating directly to the wrappers.

This now seems good and makes it explicit what is required.

[dfawley]

*its