[FIXED] Make sure to process extended purge operations correctly when being replayed. #4212
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
…eplayed on a restart. Signed-off-by: Derek Collison <derek@nats.io>
7 tasks
|
Awesome! 🎉 I believe there might be a bug with both |
derekcollison
added a commit
that referenced
this pull request
Jun 4, 2023
PR #4212 fixed the issue I reported in #4196. However, I believe there might be a bug when both `sequence` and `keep` are set during recovery. In the `PurgeEx` the following check is done (for both `filestore.go` and `memstore.go`): ```go if sequence > 1 && keep > 0 { return 0, ErrPurgeArgMismatch } ``` The `TestJetStreamClusterPurgeExReplayAfterRestart` also triggers this case, meaning that during the test this error is returned but it succeeds because the purge was already performed. Is this intended behaviour? To elaborate a bit more, I believe the following happens: - when running the purge normally it will properly run the `keep` (since it's not combined with `sequence` yet) - when replaying the purge though, the `sequence` is added to the `keep`, which errors out in the above if Which means that during normal operation all will be well, but purges with `keep` will be ignored upon replaying. I'm proposing to remove the `sequence > 1 && keep > 0` check and subsequent error. Which, for reference, was introduced in #3121. Hoping this ensures that during recovery, purges that haven't executed yet will still be executed. An alternative approach, which wouldn't remove the error: not allow combining `sequence` and `keep` normally and only allowing it during recovery. Which would preserve the current behaviour, and correctly apply `sequence+keep` during recovery still. However, not sure if it's possible to know if we're in "recovery mode" from within `PurgeEx`. Resolves #4196
MauriceVanVeen
referenced
this pull request
Jul 11, 2023
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.
This is an extension to the excellent work by @MauriceVanVeen and his original PR #4197 to fully resolve for all use cases.
Signed-off-by: Derek Collison derek@nats.io
Resolves #4196