raft: send de-fortification requests with the correct term #131828
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
There was a problem hiding this comment.
Nice, this came out well. 
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @arulajmani and @iskettaneh)
-- commits line 49 at r3:
"to the old leader"
pkg/raft/raft.go line 1186 at r3 (raw file):
nit: since this is a little surprising, consider driving the point home even further in the second sentence:
We reset it if we ever step up to become leader again, but not when stepping down as leader and not even when learning of a new leader in a later term.
pkg/raft/tracker/fortificationtracker.go line 30 at r3 (raw file):
fortification map[pb.PeerID]pb.Epoch // term is the leadership term associated with fortification tracking.
This would be a good place to add a few more words around how this differs from raft.term and why we need it to.
pkg/raft/tracker/fortificationtracker.go line 31 at r3 (raw file):
// term is the leadership term associated with fortification tracking. term uint64
tiny nit here and in Reset: move term above fortification to match their respective scopes. The fortification state is scoped to a single leadership term.
pkg/raft/tracker/fortificationtracker.go line 61 at r3 (raw file):
// Reset clears out any previously tracked fortification and prepares the // fortification tracker to be used at the supplied term.
"used by a newly elected leader"
There was a problem hiding this comment.
Reviewed 21 of 21 files at r1, 14 of 14 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status:
pkg/raft/raft.go line 854 at r2 (raw file):
func (r *raft) sendDeFortify(to pb.PeerID) { if to == r.id { // We handle the case where the leader is trying to de-fortify itself
maybe keep the comment before the TODO just to help future readers know why we have this special case
pkg/raft/testdata/de_fortification_basic.txt line 985 at r3 (raw file):
INFO 2 [logterm: 4, index: 5] sent MsgVote request to 3 at term 5 stabilize
Curious if we really need the debug level logging in this test, or can we get away with info?
arulajmani
force-pushed
the
ll-msg-defortify-correct-term
branch
from
f774409 to
c18dc54
Compare
There was a problem hiding this comment.
RFAL
Reviewable status:
pkg/raft/raft.go line 1186 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: since this is a little surprising, consider driving the point home even further in the second sentence:
We reset it if we ever step up to become leader again, but not when stepping down as leader and not even when learning of a new leader in a later term.
Done.
pkg/raft/tracker/fortificationtracker.go line 30 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
This would be a good place to add a few more words around how this differs from
raft.termand why we need it to.
Done.
pkg/raft/tracker/fortificationtracker.go line 31 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
tiny nit here and in
Reset: movetermabovefortificationto match their respective scopes. The fortification state is scoped to a single leadership term.
Done.
pkg/raft/testdata/de_fortification_basic.txt line 985 at r3 (raw file):
Previously, iskettaneh wrote…
Curious if we really need the debug level logging in this test, or can we get away with info?
Not for the one we added in this patch, but some of the de-fortification related log lines from the previous PR are behind a debug log.
arulajmani
force-pushed
the
ll-msg-defortify-correct-term
branch
from
c18dc54 to
a264fb0
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/raft/raft.go line 854 at r2 (raw file):
Previously, iskettaneh wrote…
maybe keep the comment before the TODO just to help future readers know why we have this special case
That was accidental. Fixed.
There was a problem hiding this comment.
Reviewed 30 of 30 files at r4, all commit messages.
Reviewable status:
pkg/raft/tracker/fortificationtracker.go line 33 at r4 (raw file):
// The term differs from raft.term in that raft.term is the highest term known // to a peer, whereas FortificationTracker.term is the highest term a peer was // a leader at.
"was a leader at since the last time it restarted."
Addresses a TODO, where we were plopping an incorrect term on MsgDeFortifyLeader. A MsgDeFortifyLeader should include the leadership term that's being de-fortified, not the currently known term to the old leader that's trying to de-fortify. To enable this, we track the term on the FortificationTracker and ensure it's only reset when a peer steps up to become leader again. Informs cockroachdb#129098 Release note: None
arulajmani
force-pushed
the
ll-msg-defortify-correct-term
branch
from
a264fb0 to
7e28f1e
Compare
There was a problem hiding this comment.
TFTRs!
bors r=nvanbenschoten,iskettaneh
Reviewable status:
pkg/raft/tracker/fortificationtracker.go line 33 at r4 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
"was a leader at since the last time it restarted."
Good catch. Done.
First 2 commits from #131668
Addresses a TODO, where we were plopping an incorrect term on
MsgDeFortifyLeader. A MsgDeFortifyLeader should include the leadership
term that's being de-fortified, not the currently known term to old
leader that's trying to de-fortify. To enable this, we track the term on
the FortificationTracker and ensure it's only reset when a peer steps up
to become leader again.
Informs #129098
Release note: None