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

add Tombstone property #847

Merged
merged 4 commits into from
Nov 12, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions modules/reference/pages/properties/cluster-properties.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -4856,6 +4856,26 @@ Transaction manager's synchronization timeout. Maximum time to wait for internal

---

=== tombstone_retention_ms

The retention time for tombstone records in a compacted topic. Cannot be enabled at the same time as any of `cloud_storage_enabled`, `cloud_storage_enable_remote_read`, or `cloud_storage_enable_remote_write`. A typical default setting is `86400000`, or 24 hours.

*Unit:* milliseconds

*Requires restart:* No

*Visibility:* `user`

*Type:* integer

*Accepted values:* [`1`, `17592186044415`]

*Default:* `null`
Copy link

@WillemKauf WillemKauf Nov 11, 2024

Choose a reason for hiding this comment

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

Not sure if we usually add recommended values for properties that default to null in the docs, but up to your discretion as to whether we want to mention a typical default of 24 hours/86400000 ms (see kafka docs here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have recommended values we can add them in the description cc @kbatuigas

Copy link
Contributor Author

@Deflaimun Deflaimun Nov 11, 2024

Choose a reason for hiding this comment

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

@WillemKauf per the code, look like this accepts negative values. Should this be a bounded_property to avoid this? I can add 0 as starting point too.

Asking because someties allowing negative numbers in time-based properties is intended

Choose a reason for hiding this comment

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

The valid range is actually defined here: [1ms, 17592186044415] is correct.

Choose a reason for hiding this comment

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

I notice some other time-based properties that might need fixing:

https://docs.redpanda.com/current/reference/properties/cluster-properties/#raft_replica_max_flush_delay_ms is also bounded by [1ms, 17592186044415] but is negative in the docs.

https://docs.redpanda.com/current/reference/properties/cluster-properties/#raft_heartbeat_interval_ms is also bounded by [1ms, 17592186044415] but is negative in the docs.

There are a lot more properties in core that should have these bounds enforced, but don't... hm.


Copy link
Contributor

Choose a reason for hiding this comment

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

We could also add the main doc as a related topic? The xref will be xref:manage:cluster-maintenance/compaction-settings.adoc#tombstone-record-removal[Tombstone record removal]

*Related topics:* xref:manage:cluster-maintenance/compaction-settings.adoc#tombstone-record-removal[Tombstone record removal]

---

=== topic_fds_per_partition

Required file handles per partition when creating topics.
Expand Down