balancer: Add UpdateAddresses() to balancer.ClientConn interface #4215
Conversation
easwars
added
the
Type: API Change
easwars
force-pushed
the
update_addresses_to_clientconn
branch
from
3c23e8d
to
8006a39
Compare
easwars
force-pushed
the
update_addresses_to_clientconn
branch
from
8006a39
to
6f786bc
Compare
balancer_conn_wrappers.go
Outdated
| ccb.mu.Lock() | ||
| if ccb.subConns == nil { | ||
| ccb.mu.Unlock() | ||
| return | ||
| } | ||
| ccb.mu.Unlock() |
There was a problem hiding this comment.
I think I asked about this before and you didn't know (?) -- why is this check here? It seems superfluous given the LB policy would have been calling UpdateAddresses directly previously.
There was a problem hiding this comment.
After looking at the code a little bit more, I realized that this check is in all methods mainly to make sure that we do not process any of these calls once the balancer_wrapper is closed (which is done when the ClientConn is closed, or the LB policy is switched out). That is the reason why I let this stay. Let me know what you think.
There was a problem hiding this comment.
Do we have similar checks in acbw.UpdateAddresses? If so, I'd omit this to keep it as minimal as possible. If not...why not?
There was a problem hiding this comment.
I think this is what is happening. addrConn is torn down when either the ClientConn is closed, or the balancer is switched out. And correspondingly addrConn has a state associated with itself.
Now, the existing methods of acBalancerWrapper, namely UpdateAddresses() and Connect() call into appropriate methods on the addrConn which eventually check the state of the addrConn, and if it is in Shutdown state, they end up doing nothing.
So, we should be safe to omit this check here and rely on the checks underneath. What do you think?
easwars
force-pushed
the
update_addresses_to_clientconn
branch
from
fa900c0
to
7fa11f8
Compare
This is the first PR for the set of API changes outlined in #4207.
This PR adds the
UpdateAddresses()method to thebalancer.ClientConninterface and changes gRPC code to use the new method instead of similarly named method on theSubConninterface.