Skip to content

Conversation

MauriceVanVeen
Copy link
Member

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

@MauriceVanVeen MauriceVanVeen requested a review from a team as a code owner July 1, 2025 08:18
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)
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@MauriceVanVeen MauriceVanVeen force-pushed the maurice/kv-redundant-limit-marker branch from 60671d8 to 22f0599 Compare July 1, 2025 08:26
@neilalexander neilalexander requested a review from ripienaar July 7, 2025 15:56
@ripienaar
Copy link
Contributor

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

@ripienaar ripienaar requested a review from derekcollison July 8, 2025 06:36
@derekcollison
Copy link
Member

Which one are we keeping versus supresing?

@MauriceVanVeen
Copy link
Member Author

Which one are we keeping versus supresing?

The original explicit kv.Purge with TTL is kept. The duplicate subject delete marker that's created after the kv.Purge TTL expires is the one that's suppressed.

@@ -40,6 +40,8 @@ import (
"testing"
"time"

"github.com/nats-io/nats.go/jetstream"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove new line here

Copy link
Member Author

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.

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MauriceVanVeen MauriceVanVeen force-pushed the maurice/kv-redundant-limit-marker branch 2 times, most recently from aebc530 to 928e0d3 Compare July 11, 2025 08:55
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
@MauriceVanVeen MauriceVanVeen force-pushed the maurice/kv-redundant-limit-marker branch from 928e0d3 to 94b0fb3 Compare July 21, 2025 10:39
Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@neilalexander neilalexander merged commit 3fef571 into main Jul 21, 2025
111 of 114 checks passed
@neilalexander neilalexander deleted the maurice/kv-redundant-limit-marker branch July 21, 2025 11:24
neilalexander added a commit that referenced this pull request Jul 25, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants