-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 (2.11): Truncate entries without quorum from different pterm
#6027
base: main
Are you sure you want to change the base?
NRG (2.11): Truncate entries without quorum from different pterm
#6027
Conversation
@@ -3383,35 +3383,37 @@ func (n *raft) processAppendEntry(ae *appendEntry, sub *subscription) { | |||
n.updateLeadChange(false) | |||
} | |||
|
|||
if (isNew && ae.pterm != n.pterm) || ae.pindex != n.pindex { |
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.
Removing isNew
makes sure we can also detect differences if we're catching up, otherwise we could wrongly assume our own state is correct.
Specifically covered by the test TestNRGWALEntryWithoutQuorumMustTruncate/diverged
@@ -3434,16 +3436,6 @@ func (n *raft) processAppendEntry(ae *appendEntry, sub *subscription) { | |||
return | |||
} | |||
|
|||
// Check if only our terms do not match here. | |||
if ae.pindex == n.pindex { |
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.
This condition could never be hit before, due to the condition above:
// Check if this is a lower or equal index than what we were expecting.
if ae.pindex <= n.pindex {
Now moved up so the pterm
can be corrected, otherwise we could spin.
Already covered by the test TestJetStreamClusterStreamCatchupNoState
. Previously it used a different way to pass, likely truncating a bit too much and eventually converging. Now it only corrects the pterm
as truncating is not needed.
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, have spoken to @MauriceVanVeen about some of the changes here and I think they're OK. Should be simpler than the previous approach of starting catchups at n.commit
.
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'm a little confused about some of these changes, but if you and Neil are confident, so am i.
It would be easier to review if each commit was atomic / self-contained and the commit message explained what the intent / reason was.
You have some comments in GH which is convenient, but if you look at this commit in a month will you find them?
Unlike GH comments, commit messages become part of history along with the change.
// If terms are equal, and we are not catching up, we have simply already processed this message. | ||
// So we will ACK back to the leader. This can happen on server restarts based on timings of snapshots. | ||
if ae.pterm == n.pterm && !catchingUp { | ||
success = true | ||
} else if ae.pindex == n.pindex { |
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 don't get this bit.
If our previous index already matches, what are we truncating?
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.
Realistically, only updating the pterm
when the passed index == n.pindex
.
We could possibly also just set n.pterm = ae.pterm
directly instead, but not sure why it was previously chosen to be a call to truncateWAL
(so am just doing the same thing as what was done previously).
} | ||
|
||
// Encode an AppendEntry. | ||
// An AppendEntry is encoded into a buffer and that's stored into the WAL. |
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 don't get the reference to the WAL here.
Looks to me like this is pure side-effect helper: attach a buffer to the AE which contains a serialized version of the AE itself.
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.
The AppendEntry needs to be encoded into a buffer, and that buffer is what is stored into the WAL (outside of this encoding part). Without encoding we will not be able to store it.
Could maybe rephrase it to make that clearer?
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
…mmitted Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
df79a38
to
f9d2cc4
Compare
pterm
@mprimi you good with this PR? |
Reverts the changes made in #5987, but the tests are kept.
Instead opting for a simpler approach:
isNew
condition whenpterm
orpindex
don't match, to ensure consistency even during catchupae.pindex == n.pindex
condition up sopterm
can be corrected (otherwise it would not be executed)Signed-off-by: Maurice van Veen github@mauricevanveen.com