Skip to content
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

NRG: Don't switch to candidate when waiting for pending applies #6056

Conversation

MauriceVanVeen
Copy link
Member

@MauriceVanVeen MauriceVanVeen commented Oct 30, 2024

A follower could become candidate/leader before processing a snapshot. Based on timing the snapshot could enter into the apply queue, and before n.PauseApply() is called the follower could freely become leader and then fail processing the snapshot.

When a snapshot is not successfully applied yet, block the follower from becoming candidate until the snapshot is processed.

Signed-off-by: Maurice van Veen github@mauricevanveen.com

@MauriceVanVeen MauriceVanVeen changed the title [FIXED] Don't become candidate while processing stream snapshot NRG (2.11): Don't become candidate while processing stream snapshot Oct 30, 2024
@MauriceVanVeen MauriceVanVeen force-pushed the maurice/dont-become-candidate-during-process-stream-snapshot branch from 474a4b5 to 943d20d Compare October 30, 2024 10:20
@MauriceVanVeen MauriceVanVeen changed the title NRG (2.11): Don't become candidate while processing stream snapshot NRG (2.11): Don't become candidate while processing snapshot Oct 30, 2024
server/raft.go Outdated Show resolved Hide resolved
@MauriceVanVeen MauriceVanVeen force-pushed the maurice/dont-become-candidate-during-process-stream-snapshot branch from 943d20d to 3bc1227 Compare October 30, 2024 10:41
server/raft.go Outdated Show resolved Hide resolved
@MauriceVanVeen MauriceVanVeen force-pushed the maurice/dont-become-candidate-during-process-stream-snapshot branch from 3bc1227 to 61c6c59 Compare October 30, 2024 14:08
@derekcollison
Copy link
Member

Let me know when ready for review.

@neilalexander
Copy link
Member

Maurice and I have been on Zoom talking about this change and we've come up with a cleaner fix, so will be posting an update soon. Will let you know when ready for review.

@MauriceVanVeen MauriceVanVeen force-pushed the maurice/dont-become-candidate-during-process-stream-snapshot branch from 61c6c59 to c3e65f2 Compare October 30, 2024 16:13
@MauriceVanVeen MauriceVanVeen changed the title NRG (2.11): Don't become candidate while processing snapshot [FIXED] Don't become candidate while processing snapshot Oct 30, 2024
@MauriceVanVeen MauriceVanVeen force-pushed the maurice/dont-become-candidate-during-process-stream-snapshot branch 2 times, most recently from 3eb4281 to be280f0 Compare October 30, 2024 16:28
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
@MauriceVanVeen MauriceVanVeen force-pushed the maurice/dont-become-candidate-during-process-stream-snapshot branch from be280f0 to 7af96d8 Compare October 30, 2024 16:45
@MauriceVanVeen MauriceVanVeen changed the title [FIXED] Don't become candidate while processing snapshot NRG: Don't switch to candidate when waiting for pending applies Oct 30, 2024
Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@neilalexander neilalexander marked this pull request as ready for review October 30, 2024 17:16
@neilalexander neilalexander requested a review from a team as a code owner October 30, 2024 17:16
@neilalexander
Copy link
Member

@derekcollison This one is ready for review now!

if n.observer || n.paused {
// Avoid petitioning to become leader if we're behind on applies.
if n.observer || n.paused || n.applied < n.commit {
n.resetElect(minElectionTimeout / 4)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the div 4?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't want to slow down the potential for an election too much, so this ends up being 1 second in normal operation instead of 4 to 9 seconds. Divide because then it speeds up in the unit tests along with all the other consts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@derekcollison derekcollison merged commit 1ee2b8a into main Oct 30, 2024
5 checks passed
@derekcollison derekcollison deleted the maurice/dont-become-candidate-during-process-stream-snapshot branch October 30, 2024 18:15
neilalexander added a commit that referenced this pull request Nov 25, 2024
Includes the following:

- #5661
- #5666
- #5671
- #5344
- #5684
- #5689
- #5691
- #5714
- #5717
- #5707
- #5792
- #5912
- #5957
- #5700
- #5975
- #5991
- #5987
- #6027
- #6038
- #6053
- #5848
- #6055
- #6056
- #6060
- #6061
- #6072
- #5832
- #6073
- #6107

Signed-off-by: Neil Twigg <neil@nats.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants