Prepare balancer interfaces for >=google.golang.org/grpc@1.30.0 upgrade.
#12658
Conversation
client/v3/balancer/balancer.go
Outdated
| @@ -191,6 +214,13 @@ func (bb *baseBalancer) HandleResolvedAddrs(addrs []resolver.Address, err error) | |||
| // (DO NOT) delete(bb.scToSt, sc) | |||
| } | |||
| } | |||
|
|
|||
| return nil | |||
There was a problem hiding this comment.
@ptabor I think we could do better here than a nil. For example, every time there is a
if err != nil {
bb.lg.Warn("NewSubConn failed", zap.String("picker", bb.picker.String()), zap.String("balancer-id", bb.id), zap.Error(err), zap.String("address", addr.Addr))
continue
}above we could append the err to a slice and then coalesce that slice here into a non-nil return if desired.
(An early exit after the bb.lg.Warn() would be a breaking change so coalescing would be the safe way to do it.)
There was a problem hiding this comment.
Agree. We should both log them and collect.
As long as we don't wrap them into ErrBadResolverState it should have not semantic impact.
Mid-term: I would consider throwing ErrBadResolverState in case there is no SubConn resolved, but this requires separate testing. Maybe worth adding a TODO.
Codecov Report
@@ Coverage Diff @@
## master #12658 +/- ##
===========================================
+ Coverage 51.78% 63.42% +11.63%
===========================================
Files 409 406 -3
Lines 32671 32579 -92
===========================================
+ Hits 16919 20662 +3743
+ Misses 13756 9800 -3956
- Partials 1996 2117 +121
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
client/v3/balancer/balancer.go
Outdated
| @@ -191,6 +214,13 @@ func (bb *baseBalancer) HandleResolvedAddrs(addrs []resolver.Address, err error) | |||
| // (DO NOT) delete(bb.scToSt, sc) | |||
| } | |||
| } | |||
|
|
|||
| return nil | |||
There was a problem hiding this comment.
Agree. We should both log them and collect.
As long as we don't wrap them into ErrBadResolverState it should have not semantic impact.
Mid-term: I would consider throwing ErrBadResolverState in case there is no SubConn resolved, but this requires separate testing. Maybe worth adding a TODO.
|
Thank you. Hopefully final request: Could you please 'git squash' it into a single commit. If I 'squashed and merge', your authorship would not be preserved. |
dhermes
force-pushed
the
pr-12636-balancer
branch
from
56148fb
to
50ca440
Compare
|
Forgot to commit my branch after |
dhermes
force-pushed
the
pr-12636-balancer
branch
from
8a2a037
to
ea34f8d
Compare
No description provided.