Raft pre-vote extension implementation #530
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -120,6 +120,53 @@ func (r *RequestVoteResponse) GetRPCHeader() RPCHeader { | |||||||
| return r.RPCHeader | ||||||||
| } | ||||||||
|
|
||||||||
| // RequestPreVoteRequest is the command used by a candidate to ask a Raft peer | ||||||||
| // for a vote in an election. | ||||||||
| type RequestPreVoteRequest struct { | ||||||||
| RPCHeader | ||||||||
|
|
||||||||
| // Provide the term and our id | ||||||||
| Term uint64 | ||||||||
|
|
||||||||
| // Deprecated: use RPCHeader.Addr instead | ||||||||
| Candidate []byte | ||||||||
|
There was a problem hiding this comment.
Suggested change
Could we remove this field. Seeing as its been deprecated for the regular RequestVoteRequest it seems reasonable for pre-vote to just never support this way of specifying the candidate server. Also in processing pre vote requests we never use this field. |
||||||||
|
|
||||||||
| // Used to ensure safety | ||||||||
| LastLogIndex uint64 | ||||||||
| LastLogTerm uint64 | ||||||||
|
|
||||||||
| // Used to indicate to peers if this vote was triggered by a leadership | ||||||||
| // transfer. It is required for leadership transfer to work, because servers | ||||||||
| // wouldn't vote otherwise if they are aware of an existing leader. | ||||||||
| LeadershipTransfer bool | ||||||||
|
There was a problem hiding this comment. Do leadership transfers utilize pre-vote? I think they should probably avoid pre-voting. There was a problem hiding this comment. yes it does. I traced leadership transfer and it work as follow:
From this point normal leadership election will happen (including pre-vote) the only exception is that we send the LeadershipTransfer as part of the vote (and pre-vote) to avoid not granting the vote/pre-vote when the voting node think it still have a leader. I'm mitigated about if we should keep it as is or completely remove pre-vote from the equation when performing leadership transfer I think it won't change anything about the behaviour in leadership transfer as in all cases we will end up in leader-election. I would be interested in hearing your thoughts and @banks about this. There was a problem hiding this comment. 🤔 My initial thought here is that we should probably skip pre-vote for a leadership transfer to keep it easier to reason about:
I've not looked to see how hard that is in the code though. But I'd be tempted rather than adding this flag here to instead just avoid sending prevote round at all when responding to a TimeoutNow. There was a problem hiding this comment.
This is not necessarily true as we allow providing the server we would like to transfer leadership to and it's a best effort transfer, so if the server we specify is not able to become a leader the transfer won't happen. That said, I agree on the fact that leadership transfer is probably not something that we will benefit from optimizing the election for and removing it from the pre-vote flow remove some of the complexity. |
||||||||
| } | ||||||||
|
|
||||||||
| // GetRPCHeader - See WithRPCHeader. | ||||||||
| func (r *RequestPreVoteRequest) GetRPCHeader() RPCHeader { | ||||||||
| return r.RPCHeader | ||||||||
| } | ||||||||
|
|
||||||||
| // RequestPreVoteResponse is the response returned from a RequestPreVoteRequest. | ||||||||
| type RequestPreVoteResponse struct { | ||||||||
| RPCHeader | ||||||||
|
|
||||||||
| // Newer term if leader is out of date. | ||||||||
| Term uint64 | ||||||||
|
|
||||||||
| // Peers is deprecated, but required by servers that only understand | ||||||||
| // protocol version 0. This is not populated in protocol version 2 | ||||||||
| // and later. | ||||||||
| Peers []byte | ||||||||
|
There was a problem hiding this comment. I'd maybe lean towards taking this out. I can understand that it might be simpler initially to have identical code in Vote and PreVote so I might be persuaded, but it seems odd that we'd add in legacy stuff that will never be used and will need to be cleaned up again in the future. |
||||||||
|
|
||||||||
| // Is the vote granted. | ||||||||
| Granted bool | ||||||||
| } | ||||||||
|
|
||||||||
| // GetRPCHeader - See WithRPCHeader. | ||||||||
| func (r *RequestPreVoteResponse) GetRPCHeader() RPCHeader { | ||||||||
| return r.RPCHeader | ||||||||
| } | ||||||||
|
|
||||||||
| // InstallSnapshotRequest is the command sent to a Raft peer to bootstrap its | ||||||||
| // log (and state machine) from a snapshot on another peer. | ||||||||
| type InstallSnapshotRequest struct { | ||||||||
|
|
||||||||
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.
I think we should check that
transportactually supports prevote here and auto-disable it if not.For example if a product like Consul has a custom transport that wraps NetTransport and they don't realise they need to implement the optional interface, then when they upgrade they will end up silently trying to prevote but not actually sending messages and then timing out pointlessly.