Skip to content
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

Refinements to MSG TTL spec during implementation #315

Merged
merged 9 commits into from
Jan 23, 2025
Merged

Conversation

ripienaar
Copy link
Contributor

No description provided.

@ripienaar
Copy link
Contributor Author

I think this is the only deviations so far? But please let me know about others and once you are done implementing we can merge this with all the changes included

@ripienaar ripienaar marked this pull request as ready for review January 9, 2025 10:55
@neilalexander
Copy link
Member

Notes:

  • Keep reject on Nats-TTL with TTLs disabled for client publishes, but allow keeping the header via sources/mirrors, just ignore them
  • Ensure LimitsTTL cannot be enabled at the same time as mirroring
  • Future option to remove individual headers on a per-source/mirror basis to remove TTLs later if we want

@ripienaar
Copy link
Contributor Author

OK, so to summarise where we got to, would this be your understanding @neilalexander :

  • We will accept Nats-TTL over sources/mirrors always and leave it as is
  • If the Source has ttl handling enabled we will process it
  • Mirrors may not enable that setting

right?

@neilalexander
Copy link
Member

As it stands, AllowMsgTTL can be enabled on a mirror but LimitsTTL cannot be. Otherwise yep, that's right.

@ripienaar
Copy link
Contributor Author

As it stands, AllowMsgTTL can be enabled on a mirror but LimitsTTL cannot be. Otherwise yep, that's right.

updated with bd022dc to capture these

Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM

@ripienaar
Copy link
Contributor Author

Have updated with the split in delete markers and TTL with a default.

Note this has the side effect that already deployed streams will have their running behaviour changed without user input or control should the server later update this default.

@ripienaar
Copy link
Contributor Author

Updated again for defaults being placed in the config unless pedantic

Signed-off-by: R.I.Pienaar <rip@devco.net>
Signed-off-by: R.I.Pienaar <rip@devco.net>
Signed-off-by: R.I.Pienaar <rip@devco.net>
Signed-off-by: R.I.Pienaar <rip@devco.net>
Signed-off-by: R.I.Pienaar <rip@devco.net>
Signed-off-by: R.I.Pienaar <rip@devco.net>
Signed-off-by: R.I.Pienaar <rip@devco.net>
Signed-off-by: R.I.Pienaar <rip@devco.net>
Signed-off-by: R.I.Pienaar <rip@devco.net>
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

@ripienaar ripienaar merged commit 57b94a7 into main Jan 23, 2025
1 check passed
@ripienaar ripienaar deleted the ttl_refinements branch January 23, 2025 11:31
// LimitsTTL activates writing of messages when limits are applied with a specific TTL
LimitsTTL time.Duration `json:"limits_ttl"`
// Enables placing markers in the stream for certain message delete operations
SubjectDeleteMarkers bool `json:"subject_delete_markers,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

This setting is really not needed - the TTL setting is going to be highly dependent on the use-case, so auto-setting it is not the right approach - user should just specify the SubjectDeleteMarkerTTL and that infers that SubjectDeleteMarkers

Copy link
Member

Choose a reason for hiding this comment

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

We had this conversation quite a few times, but the prevailing problem is that unless KVs etc are setting it for you, that users setting this on their own streams may have no way to intuitively know what a sensible value even is, i.e. for lost connections, ordered consumer recreation, etc, hence the "sensible default".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also do not know what a correct setting is. If a user cannot pick a default, neither can we. All we are doing is guaranteeing that people will feel the system is broken rather than that it is behaving in a way that they chose - even if at the time of choosing they might not have realised the full weight of the choice.

We are still pretending we can answer the question: What is the default outage time?

We can not.

Further this pushes a bunch of complexity onto everywhere, want to unset this setting now there are other coupled settings to also unset and understand etc.

I am, still, strongly against this design.

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