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.

Sign up for GitHub

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

Jump to bottom

add Tombstone property #847

Merged
merged 4 commits into from
Nov 12, 2024
Merged

add Tombstone property #847

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
820e238
add tombstone cluster property
Deflaimun Nov 11, 2024
e4b1482
Update modules/reference/pages/properties/cluster-properties.adoc
Deflaimun Nov 12, 2024
326cfd3
Update modules/reference/pages/properties/cluster-properties.adoc
Deflaimun Nov 12, 2024
6cb3e60
add related topic
Deflaimun Nov 12, 2024
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
18 changes: 18 additions & 0 deletions modules/reference/pages/properties/cluster-properties.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4856,6 +4856,24 @@ 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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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`.
The retention time (in ms) 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if we should add (in ms) as we already have the units field

Deflaimun marked this conversation as resolved.
Show resolved Hide resolved

*Unit:* milliseconds

*Requires restart:* No

*Visibility:* `user`

*Type:* integer

*Accepted values:* [`-17592186044416`, `17592186044415`]
Copy link

Choose a reason for hiding this comment

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

[1ms, 17592186044415]

Deflaimun marked this conversation as resolved.
Show resolved Hide resolved

*Default:* `null`
Copy link

@WillemKauf WillemKauf Nov 11, 2024 •
edited
Loading

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 •
edited
Loading

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

Copy link

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.

Copy link

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]

---

=== topic_fds_per_partition

Required file handles per partition when creating topics.
Expand Down