Re-resolve target when one connection becomes TransientFailure #1679
Conversation
menghanl
force-pushed
the
balancer_resolve_now
branch
from
eeac94f
to
fc179e5
Compare
This allows ClientConn to get more up-to-date addresses from resolver. ClientConn compares new addresses with the cached ones. So if resolver returns the same set of addresses, ClientConn will not notify balancer about it. Also moved the initialization of resolver and balancer to avoid race. Balancer will only be started when ClientConn gets resolved addresses from balancer.
menghanl
force-pushed
the
balancer_resolve_now
branch
from
fc179e5
to
7e59d30
Compare
| return | ||
| } | ||
|
|
||
| if reflect.DeepEqual(cc.curAddresses, addrs) { |
There was a problem hiding this comment.
I have heard just importing the reflect package is a bad idea (in the form of performance or binary size or both?). We should look into that claim and remove it if so. Looks like we're already importing it, so LGTM for this PR.
There was a problem hiding this comment.
We should try the cmp package.
clientconn.go
Outdated
| // First time handling resolved addresses. Build a balancer use either | ||
| // the builder specified by dial option, or pickfirst. | ||
| var builder balancer.Builder | ||
| if cc.dopts.balancerBuilder != nil { |
There was a problem hiding this comment.
Go style:
builder := cc.dopts.balancerBuilder
if builder == nil {
builder = newPickfirstBuilder()
}
clientconn.go
Outdated
| return | ||
| } | ||
| cc.mu.Unlock() | ||
| go r.resolveNow(o) |
There was a problem hiding this comment.
Can this result in multiple simultaneous outgoing calls to the resolver's ResolveNow method? Is that OK?
There was a problem hiding this comment.
It's up to the resolver on how to deal with parallel ResolveNow(). In the dns resolver implementation, a new ResolveNow() is ignored if the previous one hasn't been processed.
From ClientConn's point, if multiple ResolveNow() result in multiple NewAddress() calls, only the last update will be kept. And we also ignore this update if it's the same as the cache we have.
There was a problem hiding this comment.
Maybe we should add a comment to ResolveNow() to let users know that it could be called multiple times concurrently.
clientconn.go
Outdated
| cc.mu.Lock() | ||
| r := cc.resolverWrapper | ||
| if r == nil { | ||
| cc.mu.Unlock() |
There was a problem hiding this comment.
cc.mu.Lock()
r := cc.resolverWrapper
cc.mu.Unlock()
if r == nil {
return
}
go r.resolveNow(o)
| return | ||
| } | ||
|
|
||
| if reflect.DeepEqual(cc.curAddresses, addrs) { |
There was a problem hiding this comment.
We should try the cmp package.
clientconn.go
Outdated
| // First time handling resolved addresses. Build a balancer use either | ||
| // the builder specified by dial option, or pickfirst. | ||
| var builder balancer.Builder | ||
| if cc.dopts.balancerBuilder != nil { |
clientconn.go
Outdated
| cc.mu.Lock() | ||
| r := cc.resolverWrapper | ||
| if r == nil { | ||
| cc.mu.Unlock() |
clientconn.go
Outdated
| return | ||
| } | ||
| cc.mu.Unlock() | ||
| go r.resolveNow(o) |
There was a problem hiding this comment.
It's up to the resolver on how to deal with parallel ResolveNow(). In the dns resolver implementation, a new ResolveNow() is ignored if the previous one hasn't been processed.
From ClientConn's point, if multiple ResolveNow() result in multiple NewAddress() calls, only the last update will be kept. And we also ignore this update if it's the same as the cache we have.
This allows ClientConn to get more up-to-date addresses from resolver.
ClientConn compares new addresses with the cached ones. So if resolver returns the same set of addresses, ClientConn will not notify balancer about it.
Also moved the initialization of resolver and balancer to avoid race. Balancer will only be started when ClientConn gets resolved addresses from balancer.