-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[FIXED] No redundant limit marker on KV purge #7026
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
Conversation
server/sdm.go
Outdated
// isSubjectDeleteMarker returns whether the headers indicate this message is a subject delete marker. | ||
// Either it's a usual marker with JSMarkerReason, or it's a KV Purge marker as the KVOperation. | ||
func isSubjectDeleteMarker(hdr []byte) bool { | ||
return len(getHeader(JSMarkerReason, hdr)) == 0 && !bytes.Equal(getHeader(KVOperation, hdr), KVOperationValuePurge) |
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.
These could probably be sliceHeader
for performance.
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.
Done
60671d8
to
22f0599
Compare
Generally in favour of this, though I think we've not had customers filing issues for this so it's perhaps not as important as stated. In the past we've tried not to add KV specific stuff into the server, thats my only concern, wdyt @derekcollison |
Which one are we keeping versus supresing? |
The original explicit |
server/jetstream_test.go
Outdated
@@ -40,6 +40,8 @@ import ( | |||
"testing" | |||
"time" | |||
|
|||
"github.com/nats-io/nats.go/jetstream" | |||
|
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.
remove new line here
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 was due to a conflict in the file. Fixed after rebase.
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
aebc530
to
928e0d3
Compare
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
928e0d3
to
94b0fb3
Compare
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
Includes the following: - #7031 - #7033 - #7034 - #7035 - #7036 - #7040 - #7043 - #7045 - #7047 - #7046 - #7050 - #7051 - #7052 - #7053 - #7061 - #7063 - #7064 - #7065 - #7066 - #7070 - #7072 - #7080 - #7026 - #6728 - #7074 - #7089 - #7095 - #7087 - #7094 - #7096 - #7099 Signed-off-by: Neil Twigg <neil@nats.io>
When subject delete markers are enabled and you do a KV Purge with a TTL, you effectively get two subject delete markers. One of them is the explicit KV Purge operation, the other is the subject delete marker announcing (again) that the data is purged.
This PR is proposing to not have redundant limit markers, the KV Purge operation is in essence already a subject delete marker and should not trigger a "vanilla" subject delete marker duplicate.
Signed-off-by: Maurice van Veen github@mauricevanveen.com