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
dfawley
left a comment
dfawley
left a comment
There was a problem hiding this comment.
Looks good, just one thing I noticed in a few places that could possibly eliminate some code.
| } | ||
|
|
||
| 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.
| }, | ||
| 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.)
2 participants
Fixes: #8345
RELEASE NOTES:
ExitIdlemethod toBalancer. Earlier implementing this method was optional.