[FIXED] Subject state consistency #6226
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MauriceVanVeen
commented
Dec 6, 2024
| @@ -2614,10 +2614,6 @@ func (fs *fileStore) numFilteredPendingWithLast(filter string, last bool, ss *Si | |||
| // Always reset. | |||
| ss.First, ss.Last, ss.Msgs = 0, 0, 0 | |||
|
|
|||
| if filter == _EMPTY_ { | |||
There was a problem hiding this comment.
This is redundant, if isAll := filter == _EMPTY_ || filter == fwcs we early return above.
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
MauriceVanVeen
force-pushed
the
maurice/subject-state-consistency
branch
from
06983f6 to
283b607
Compare
MauriceVanVeen
commented
Dec 6, 2024
| @@ -7502,6 +7498,9 @@ func (fs *fileStore) Compact(seq uint64) (uint64, error) { | |||
| // Update fss | |||
| smb.removeSeqPerSubject(sm.subj, mseq) | |||
| fs.removePerSubject(sm.subj) | |||
| // Need to mark the sequence as deleted. Otherwise, recalculating ss.First | |||
| // for per-subject info would be able to find it still. | |||
| smb.dmap.Insert(mseq) | |||
There was a problem hiding this comment.
Another important addition; TestFileStoreExpireMsgsOnStart was failing as it relied on the bug existing. Now that the bug is gone, we need to actually mark the sequence as deleted. Otherwise, mb.recalculateFirstForSubj could still find and select that sequence.
neilalexander
added a commit
that referenced
this pull request
Dec 13, 2024
MauriceVanVeen
added a commit
that referenced
this pull request
Dec 13, 2024
wallyqs
pushed a commit
that referenced
this pull request
Dec 13, 2024
MauriceVanVeen
pushed a commit
that referenced
this pull request
Dec 17, 2024
Subject state would not always remain consistent given a specific pattern of message removals. There were three bugs: - `recalculateFirstForSubj` in memstore would do `startSeq+1`, but filestore would always just start at `mb.first.seq`. These are now consistent. - `recalculateFirstForSubj` was not called when `ss.Msgs == 1`, which could mean we had a stale `ss.FirstSeq` if it needed to be recalculated. - If after recalculation it turns out `ss.FirstSeq` equals the message we're trying to remove, we need to `recalculateFirstForSubj` again, since `ss.Last` is also lazy and could be incorrect. Apart from that, filestore and memstore are now both equivalent when it comes to first updating per-subject state and then removing the message, as well as `removeSeqPerSubject` and how it updates the subject state. Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
wallyqs
pushed a commit
that referenced
this pull request
Dec 17, 2024
Subject state would not always remain consistent given a specific pattern of message removals. There were three bugs: - `recalculateFirstForSubj` in memstore would do `startSeq+1`, but filestore would always just start at `mb.first.seq`. These are now consistent. - `recalculateFirstForSubj` was not called when `ss.Msgs == 1`, which could mean we had a stale `ss.FirstSeq` if it needed to be recalculated. - If after recalculation it turns out `ss.FirstSeq` equals the message we're trying to remove, we need to `recalculateFirstForSubj` again, since `ss.Last` is also lazy and could be incorrect. Apart from that, filestore and memstore are now both equivalent when it comes to first updating per-subject state and then removing the message, as well as `removeSeqPerSubject` and how it updates the subject state. Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Subject state would not always remain consistent given a specific pattern of message removals.
There were three bugs:
recalculateFirstForSubjin memstore would dostartSeq+1, but filestore would always just start atmb.first.seq. These are now consistent.recalculateFirstForSubjwas not called whenss.Msgs == 1, which could mean we had a staless.FirstSeqif it needed to be recalculated.ss.FirstSeqequals the message we're trying to remove, we need torecalculateFirstForSubjagain, sincess.Lastis also lazy and could be incorrect.Apart from that, filestore and memstore are now both equivalent when it comes to first updating per-subject state and then removing the message, as well as
removeSeqPerSubjectand how it updates the subject state.Signed-off-by: Maurice van Veen github@mauricevanveen.com