balancer: Make ExitIdle compulsory for Balancers #8367
Conversation
arjan-bal
added
the
Type: API Change
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8367 +/- ##
==========================================
+ Coverage 82.17% 82.34% +0.16%
==========================================
Files 419 419
Lines 42034 42041 +7
==========================================
+ Hits 34541 34618 +77
+ Misses 6027 5969 -58
+ Partials 1466 1454 -12
🚀 New features to boost your workflow:
|
arjan-bal
force-pushed
the
balancer-exit-idle
branch
2 times, most recently
from
c91eab8 to
dbd5915
Compare
arjan-bal
force-pushed
the
balancer-exit-idle
branch
from
dbd5915 to
111c588
Compare
| @@ -102,6 +102,10 @@ type fakePetiole struct { | |||
| bOpts balancer.BuildOptions | |||
| } | |||
|
|
|||
| func (fp *fakePetiole) ExitIdle() { | |||
| fp.Balancer.ExitIdle() | |||
There was a problem hiding this comment.
This isn't necessary, is it? Balancer is embedded so this should happen automatically?
There was a problem hiding this comment.
You're right, removed it. I was working on top of the previous PR that added ExitIdle implementations but didn't add the method to the Balancer interface. This was an artifact of the that.
| if ib, ok := b.Balancer.(balancer.ExitIdler); ok { | ||
| ib.ExitIdle() | ||
| } | ||
| b.Balancer.ExitIdle() |
There was a problem hiding this comment.
Removed. Fixed throughout.
| @@ -1034,7 +1034,7 @@ func (s) TestSubConn_RegisterHealthListener(t *testing.T) { | |||
| return bd.Data.(balancer.Balancer).UpdateClientConnState(ccs) | |||
| }, | |||
| ExitIdle: func(bd *stub.BalancerData) { | |||
| bd.Data.(balancer.ExitIdler).ExitIdle() | |||
| bd.Data.(balancer.Balancer).ExitIdle() | |||
There was a problem hiding this comment.
This seems like a very common pattern. I wonder if it makes sense for the stub balancer stuff to support an embedded child balancer by default? (Not in this PR for sure.)
There was a problem hiding this comment.
Opened an issue: #8371
Fixes: #8345
RELEASE NOTES:
ExitIdlemethod toBalancer. Earlier implementing this method was optional.