-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
NRG: Don't drain response or proposal queues when switching to leader #7703
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
base: main
Are you sure you want to change the base?
Conversation
The `runAsCandidate` and `runAsFollower` goroutines both try to drain the append entry response and proposal queues if they are still populated from the previous leadership, but we might be blocked in `runAsCandidate` or `runAsFollower` while switching to leader waiting for the state change to complete and accidentally drain incoming proposals. Signed-off-by: Neil Twigg <neil@nats.io>
bc20b68 to
cfd1531
Compare
MauriceVanVeen
left a comment
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.
LGTM
derekcollison
left a comment
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.
Be good to have a test that shows what this is fixing or accommodating for.
sciascid
left a comment
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.
How about moving these calls to drain before the loop?
| n.votes.popOne() | ||
| case <-n.resp.ch: | ||
| // Ignore append entry responses received from before the state change. | ||
| n.resp.drain() |
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.
How about moving the calls to drain before the loop?
Then we just need to make sure we push into the queues only if the node if the process is the leader. This is already checked for proposals, but appears to be easy to add for appendEntry responses.
Doing so we would avoid the need to re-notify...
The
runAsCandidateandrunAsFollowergoroutines both try to drain the append entry response and proposal queues if they are still populated from the previous leadership, but we might be blocked inrunAsCandidateorrunAsFollowerwhile switching to leader waiting for the state change to complete and accidentally drain incoming proposals.Signed-off-by: Neil Twigg neil@nats.io