mvcc: tighten up watcher cancelation and revision handling #5464
Conversation
heyitsanthony
force-pushed
the
fix-victim-watchers
branch
from
ccfdd97
to
1b02fb6
Compare
| break | ||
| } | ||
| break | ||
| } |
There was a problem hiding this comment.
break here, instead of two breaks?
|
test failure is related to this change? |
|
@gyuho not sure; the test that's failing (TestGRPCStreamRequireLeader) is already broken in master but the failure mode is "etcdserver: not capable" and not a timeout. |
|
Oh you already created an issue. Got it. |
| @@ -307,7 +318,7 @@ func (s *watchableStore) moveVictims() (moved int) { | |||
| // try to send responses again | |||
| for w, eb := range wb { | |||
| select { | |||
| case w.ch <- WatchResponse{WatchID: w.id, Events: eb.evs, Revision: w.cur}: | |||
| case w.ch <- WatchResponse{WatchID: w.id, Events: eb.evs, Revision: w.minRev - 1}: | |||
There was a problem hiding this comment.
can we have some comments on why -1?
|
lgtm |
heyitsanthony
force-pushed
the
fix-victim-watchers
branch
from
1b02fb6
to
2280066
Compare
|
still broken following the api capability fixes, will investigate |
heyitsanthony
force-pushed
the
fix-victim-watchers
branch
from
2280066
to
585fd97
Compare
|
It was deadlocking waiting for the recvLoop to finish since the stream recv doesn't close until the handler exits; now it only waits on the send loop. The watcher wg patch is still necessary since the watchStream close needs to be synchronous to avoid spurious goroutine leak errors and must run concurrently with the recvloop goroutine to avoid deadlock |
Makes w.cur into w.minrev, the minimum revision for the next update, and retries cancelation if the watcher isn't found (because it's being processed by moveVictims). Fixes: etcd-io#5459
heyitsanthony
force-pushed
the
fix-victim-watchers
branch
from
585fd97
to
cfb3f96
Compare
Makes w.cur into w.minrev, the minimum revision for the next update, and
retries cancelation if the watcher isn't found (because it's being processed
by moveVictims).
Fixes: #5459