balancer: add SubConn.Shutdown; deprecate Balancer.RemoveSubConn #6493
Conversation
| @@ -115,6 +115,13 @@ type SubConn interface { | |||
| // creates a new one and returns it. Returns a close function which must | |||
| // be called when the Producer is no longer needed. | |||
| GetOrBuildProducer(ProducerBuilder) (p Producer, close func()) | |||
| // Shutdown shuts down the SubConn gracefully. Any started RPCs will be | |||
| // allowed to complete. No future calls should be made on the SubConn. | |||
There was a problem hiding this comment.
No future calls should be made on the SubConn
Do we handle calls on a closed subConn gracefully?
From what I see UpdateAddresses and Connect become no-ops if the state is SHUTDOWN. It would have been nicer if these methods returned an error though. At least Connect() could have returned an error, as the underlying addrConn.connect() already returns an error if the addrConn is shutdown, but we drop that error in our acBalancerWrapper.
Would it be better to say that future calls will be no-ops instead of saying future calls shouldn't be made?
There was a problem hiding this comment.
t would have been nicer if these methods returned an error though.
UpdateAddresses is going away, but note that adding error return values to things typically makes the calling code more complex and the API harder to reason about.
Would it be better to say that future calls will be no-ops instead of saying future calls shouldn't be made?
I'm of the opinion that telling people not to do things is better than stating what will happen if they do things that are not sensible. This is more straightforward and also gives us flexibility in how we decide to behave.
There was a problem hiding this comment.
note that adding error return values to things typically makes the calling code more complex and the API harder to reason about
It feels completely logical for an operation like Connect to return something to its caller. But I do agree that in this case though, the caller could (and does) rely on subsequent state updates to figure out how things went. But for UpdateAddresses, there is no clean way to let the caller know that the update did not happen. Good that it is going away :)
| @@ -115,6 +115,13 @@ type SubConn interface { | |||
| // creates a new one and returns it. Returns a close function which must | |||
| // be called when the Producer is no longer needed. | |||
| GetOrBuildProducer(ProducerBuilder) (p Producer, close func()) | |||
| // Shutdown shuts down the SubConn gracefully. Any started RPCs will be | |||
| // allowed to complete. No future calls should be made on the SubConn. | |||
There was a problem hiding this comment.
note that adding error return values to things typically makes the calling code more complex and the API harder to reason about
It feels completely logical for an operation like Connect to return something to its caller. But I do agree that in this case though, the caller could (and does) rely on subsequent state updates to figure out how things went. But for UpdateAddresses, there is no clean way to let the caller know that the update did not happen. Good that it is going away :)
Part 3 of #6472
RELEASE NOTES: