clientv3: respect up/down notifications from grpc #5845
Conversation
| numGets uint32 | ||
| // mu protects upEps, downEps, and numGets |
There was a problem hiding this comment.
Maybe mu sync.Mutex on the top of upEps and numGets?
And what is downEps? I don't see it in the struct fields?
There was a problem hiding this comment.
comment was outdated; fixed
heyitsanthony
force-pushed
the
clientv3-ignore-dead-eps
branch
from
24bed9c
to
4085ae1
Compare
heyitsanthony
force-pushed
the
clientv3-ignore-dead-eps
branch
3 times, most recently
from
941a85f
to
a4563d1
Compare
heyitsanthony
force-pushed
the
clientv3-ignore-dead-eps
branch
2 times, most recently
from
5414a8b
to
38f2cd0
Compare
|
fixed up to use balancer for this functionality since withblock patch was rejected PTAL /cc @xiang90 |
| v := atomic.AddUint32(&b.numGets, 1) | ||
| ep := b.eps[v%uint32(len(b.eps))] | ||
| return grpc.Address{Addr: getHost(ep)}, func() {}, nil | ||
| b.mu.Lock() |
There was a problem hiding this comment.
it seems that we need to do more work here?
see comments on Get at https://godoc.org/google.golang.org/grpc#Balancer
Also: https://github.com/grpc/grpc-go/blob/master/balancer.go#L272-L364
There was a problem hiding this comment.
I think most of that complication is from making it as general as possible. It's safe to assume FailFast is false so there's no need to implement the suggested convoluted blocking logic.
There was a problem hiding this comment.
do we expose the failfast option to user now?
heyitsanthony
force-pushed
the
clientv3-ignore-dead-eps
branch
5 times, most recently
from
8755254
to
8b53cea
Compare
| defer b.mu.Unlock() | ||
|
|
||
| if b.pinAddr != nil { | ||
| if _, ok := b.upEps[b.pinAddr.Addr]; ok || time.Since(b.pinTime) < b.pinWait { |
There was a problem hiding this comment.
i am not very clear about this. why do we need to have a pinWait? if one pined address is still up, should we use it until it fails? current pinWait is 500ms, so a new rpc coming after 500ms will choose a new endpoint now?
There was a problem hiding this comment.
OK, this code is bad; the dial timeout is already in the grpc dialer and there should only be one connecting / up endpoint at a time. I can simplify it.
heyitsanthony
force-pushed
the
clientv3-ignore-dead-eps
branch
3 times, most recently
from
c78bd3f
to
83e7ddb
Compare
|
@xiang90 all fixed PTAL |
| b.mu.Unlock() | ||
| // notify client that a connection is up | ||
| select { | ||
| case b.upc <- struct{}{}: |
There was a problem hiding this comment.
it seems like we only needs to send this once when we create the client? should we rename this to ready? and wrap it with sync.Once?
|
LGTM |
|
There may be a subtle bug in this if |
heyitsanthony
force-pushed
the
clientv3-ignore-dead-eps
branch
from
83e7ddb
to
af6b266
Compare
|
Added a test for the failover case. Now blocked on grpc/grpc-go#810 |
heyitsanthony
force-pushed
the
clientv3-ignore-dead-eps
branch
from
e4946c9
to
7985874
Compare
heyitsanthony
force-pushed
the
clientv3-ignore-dead-eps
branch
from
7985874
to
9e82185
Compare
heyitsanthony
force-pushed
the
clientv3-ignore-dead-eps
branch
2 times, most recently
from
7de19f7
to
11f2b99
Compare
heyitsanthony
force-pushed
the
clientv3-ignore-dead-eps
branch
from
11f2b99
to
7a9fa02
Compare
| return func(rpcCtx context.Context, f rpcFunc) { | ||
| for { | ||
| err := f(rpcCtx) | ||
| // ignore grpc conn closing on fail-fast calls; they are transient errors |
There was a problem hiding this comment.
is there a dial failure error? can connclosing happen after writing the request?
There was a problem hiding this comment.
There is no explicit dial failure error; the closest thing is the helpfully unexported errConnClosing (which gets grpc.Errorf()'d into a grpc formatted error). Transport errors seem to either go through ConnectionErrorf or prefixed with "transport:". I guess the safest policy is retry only on isConnClosing(err) and bail out otherwise?
heyitsanthony
force-pushed
the
clientv3-ignore-dead-eps
branch
from
7a9fa02
to
3eadf96
Compare
|
OK, changed the retry logic to bail if |
|
lgtm |
Partial patch; will need to revendor grpc once the fix on that end is merged.
Fixes #5842