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

Do not reset the electionElapsed if the node doesn't grant vote #167

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Feb 20, 2024

// If the local node receives a MsgVote message with higher term,
// but it doesn't grant the vote; it turns into a follower,
// but it shouldn't reset the electionElapsed, to ensure it
// has higher priority to start a campaign in the next round
// of election. If we reject a node, it's highly likely we
// will reject it again if it immediately campaigns again.
// So it may waste a long time to elect a leader if we reset
// the electionElapsed.

@ahrtr ahrtr marked this pull request as draft February 20, 2024 15:48
@ahrtr ahrtr force-pushed the prevote_20240220 branch 3 times, most recently from c9af84c to e0f4b39 Compare February 20, 2024 16:42
@ahrtr ahrtr changed the title Do not reset the electionElapsed if the node doesn't grant vote or preVote Do not reset the electionElapsed if the node doesn't grant vote Feb 20, 2024
@ahrtr ahrtr force-pushed the prevote_20240220 branch 3 times, most recently from d1301de to 1744073 Compare February 20, 2024 17:39
@ahrtr ahrtr marked this pull request as ready for review February 20, 2024 17:41
@ahrtr ahrtr requested review from pav-kv and erikgrinaker February 20, 2024 17:41
@ahrtr
Copy link
Member Author

ahrtr commented Feb 20, 2024

cc @pav-kv @erikgrinaker PTAL

@joshuazh-x
Copy link
Contributor

I assume the motivation of this change comes from etcd-io/etcd#17455. However, there might be a potential side effect if we don't reset election timeout.

When such a node does not reset its election timeout, it may quickly start a new campaign and makes the winning candidate step down to follower again before it becomes leader. It can be even worse if the node cannot win election (for example, it does not have sufficient connections to other nodes due to network issue). Then the node may keep stopping other candidates from becoming leader.

@ahrtr
Copy link
Member Author

ahrtr commented Feb 21, 2024

I assume the motivation of this change comes from etcd-io/etcd#17455.

Yes.

When such a node does not reset its election timeout, it may quickly start a new campaign and makes the winning candidate step down to follower again before it becomes leader.

This PR doesn't change the local node's frequency to campaign. The PR just prevents any node being affected by other unqualified candidate.

  • This PR only fixes the case when pre-vote isn't enabled. So as long as pre-vote is enabled, there is no any side effect.
  • The side effect you mentioned only exist when two nodes campaign at the same time. It's resolved by the randomised election timeout.

If the local node receives a MsgVote message with higher
term but it doesn't grant the vote; it turns into a follower,
but it shouldn't reset the electionElapsed, to ensure it
has higher priority to start a campaign in the next round
of election. If we reject a node, it's highly likely we
will reject it again if it immediately campaigns again.
So it may waste a long time to elect a leader if we reset
the electionElapsed.

Signed-off-by: Benjamin Wang <[email protected]>
@ahrtr
Copy link
Member Author

ahrtr commented Feb 22, 2024

Prefer to the alternative PR #169,

@pav-kv
Copy link
Contributor

pav-kv commented Mar 15, 2024

@ahrtr

In this PR, the local node's term has already increased by 1 even it rejects the Vote. So Conceptually it should reset the electionElapsed.

Why does it conceptually need to reset the timeout? The timeout behaviours are heuristics, so there is no right or wrong ways. We should consider behaviours and do some calculations to compare heuristics.

Do we need to reset electionTimeout at all in situations when a non-leader state remains non-leader? E.g. when we BecomeFollower, and we were already in StateFollower, just with a lower term. Upd: I guess we do need to sometimes reset it, because of #167 (comment).

@ahrtr
Copy link
Member Author

ahrtr commented Mar 15, 2024

Upd: I guess we do need to sometimes reset it, because of #167 (comment).

YES, it's one of the reasons why I prefer to #169. The other reason is #169 has better readability.

Why does it conceptually need to reset the timeout? The timeout behaviours are heuristics, so there is no right or wrong ways. We should consider behaviours and do some calculations to compare heuristics.

Do we need to reset electionTimeout at all in situations when a non-leader state remains non-leader?

All election time parameters (e.g. randomizedElectionTimeout) are supposed to be bind with each term. If the term changes (increases), then we should reset all such time parameters. At least, this is current implementation. If we don't reset the value, and carry over to next term, we don't know what other side effect it may cause apart from #167 (comment). I agree with "there is no right or wrong ways", but I tend not to take risk for now.

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.

4 participants