-
Notifications
You must be signed in to change notification settings - Fork 9.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: cancel required leader streams when memeber lost its leader #5332
Conversation
6e4de5d
to
de6894e
Compare
// We are more conservative on canceling existing streams. Reconnecting streams | ||
// cost much more than just rejecting new requests. So we wait until the member | ||
// cannot find a leader for 3 election timeouts to cancel existing streams. | ||
if noLeaderCnt >= 3 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
declare election retry max as a const somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do.
LGTM. Defer to @heyitsanthony Thanks. |
id := sws.watchStream.Watch(creq.Key, creq.RangeEnd, rev) | ||
if id != -1 && creq.ProgressNotify { | ||
sws.progress[id] = true | ||
go func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can recvLoop
stay the same and instead move the errc/goroutine machinery into Watch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. i feel this makes the driver func clearer. but i will try the other way.
overall looks ok |
@heyitsanthony All fixed. PTAL. |
lgtm |
/cc @heyitsanthony @gyuho