pickfirst: Register a health listener when used as a leaf policy #7832
Conversation
arjan-bal
added
Type: Feature
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7832 +/- ##
==========================================
+ Coverage 81.84% 82.08% +0.23%
==========================================
Files 377 377
Lines 38120 38179 +59
==========================================
+ Hits 31201 31339 +138
+ Misses 5603 5541 -62
+ Partials 1316 1299 -17
|
| // EnableHealthListener updates the state to configure pickfirst for using a | ||
| // generic health listener. | ||
| func EnableHealthListener(attrs *attributes.Attributes) *attributes.Attributes { | ||
| return attrs.WithValue(enableHealthListenerKeyType{}, enableHealthListenerValue) | ||
| } |
There was a problem hiding this comment.
For these types of functions we prefer to make them operate on the thing that contains the attributes. In this case, that would be a resolver.State.
There was a problem hiding this comment.
Changed the function to accept resolver.State.
| mu sync.Mutex | ||
| state connectivity.State | ||
| mu sync.Mutex | ||
| connectivityState connectivity.State |
There was a problem hiding this comment.
I think this deserves a comment, too, since there are now two very similar fields.
There was a problem hiding this comment.
Shouldn't this kind of tracking be done in the subconn struct (scData) and not here? I would expect the lb policy only has the concludedState and each subchannel needs to track its real state and its effective state, accounting for sticky-TF and health reporting? It seems confusing to me that the LB policy itself is tracking two different states, but I'm willing to believe it's simpler this way if you tried it the other way already.
There was a problem hiding this comment.
I was referring the Java implementation of Pickfirst and they were handling the sticky TF behaviour in the LB Policy. I don't see any issue with handling sticky TF in the subchannel state. I've update the PR to reflect the suggestions.
arjan-bal
force-pushed
the
pickfirst-health-listener
branch
from
2700efe to
cf153cb
Compare
arjan-bal
force-pushed
the
pickfirst-health-listener
branch
from
64e7a8a to
13ad224
Compare
| mu sync.Mutex | ||
| state connectivity.State | ||
| mu sync.Mutex | ||
| connectivityState connectivity.State |
There was a problem hiding this comment.
Shouldn't this kind of tracking be done in the subconn struct (scData) and not here? I would expect the lb policy only has the concludedState and each subchannel needs to track its real state and its effective state, accounting for sticky-TF and health reporting? It seems confusing to me that the LB policy itself is tracking two different states, but I'm willing to believe it's simpler this way if you tried it the other way already.
| @@ -600,11 +639,11 @@ func (b *pickfirstBalancer) updateSubConnState(sd *scData, newState balancer.Sub | |||
| // a transport is successfully created, but the connection fails | |||
| // before the SubConn can send the notification for READY. We treat | |||
| // this as a successful connection and transition to IDLE. | |||
| if (b.state == connectivity.Ready && newState.ConnectivityState != connectivity.Ready) || (oldState == connectivity.Connecting && newState.ConnectivityState == connectivity.Idle) { | |||
| if (oldState == connectivity.Ready && newState.ConnectivityState != connectivity.Ready) || (oldState == connectivity.Connecting && newState.ConnectivityState == connectivity.Idle) { | |||
There was a problem hiding this comment.
If oldState is Ready, doesn't newState have to be Idle?
And if newState is Idle, the previous state must have been Ready, right? We may have not gotten the update (because of the Connecting->Ready->Idle race), but....can this whole thing be replaced by just if newState.ConnectivityState == connectivity.Idle?
There was a problem hiding this comment.
If oldState is Ready, doesn't newState have to be Idle?
Yes, this is true because the health state is not sent via the connectivity listener anymore. So READY->TF->READY transitions are not possible and no duplicate READY updates are sent by addrConn.
And if newState is Idle, the previous state must have been Ready, right?
This is false as a transition from TF->IDLE is possible after the subchannel completes its backoff.
We can simplify the condition to oldState != TF && newState == IDLE, along with a comment explaining the assumptions that lead to this simplification. IMO, this may make it harder to understand. Do you think I should make the change?
There was a problem hiding this comment.
As discussed offline, please change to look like we want it to look (if we didn't have the bug) and add a clause that covers the bug:
if oldState == Ready || (oldState == Connecting && newState == Idle) {There was a problem hiding this comment.
(and add a TODO to remove the second half when the bug is fixed please)
As part of the dualstack changes described in A61,
pickfirstwill become the universal leaf policy. This PR provides aEnableHealthListenerfunction for petiole policies to inform pickfirst when it's functioning as a leaf policy.When functioning as a leaf policy,
pickfirstwill subscribe to health updates using the SubConn.RegisterHealthListener API introduced in #7780 once the SubConn connectivity state becomes READY. The health state will be used to update theClientConnstate as long as the SubConn's connectivity state remains READY.RELEASE NOTES: N/A