`cluster`: add `tombstone_retention_ms`/`delete.retention.ms` #23662

WillemKauf · 2024-10-07T22:24:37Z

As a follow up to #23231 and #23380, this PR adds both the cluster-level configuration property tombstone_retention_ms and the topic-level property delete.retention.ms.

The cluster level property tombstone_retention_ms controls the default value for delete.retention.ms, which is equivalent to the Kafka topic property.

There is a tight coupling between the tiered storage properties (both at a topic and a cluster level) and these configurations. Namely,

tombstone_retention_ms cannot be enabled at the same time as any of cloud_storage_enabled, cloud_storage_enable_remote_read, cloud_storage_enable_remote_write.
delete.retention.ms cannot be enabled at the same time as any of redpanda.remote.read, redpanda.remote.write.

These restrictions are in place to ensure the deletion of tombstone records does not occur for topics with tiered storage enabled, a current restriction for compacted topics in redpanda.

Docs issue: https://github.com/redpanda-data/documentation-private/issues/2759

Backports Required

Release Notes

Features

Adds the topic property delete.retention.ms, as well as the cluster property tombstone_retention_ms. Configuring these allow for the removal of tombstone records in compacted topics with tiered storage disabled in redpanda.

src/v/storage/ntp_config.h

WillemKauf · 2024-10-07T22:45:24Z

src/v/kafka/server/handlers/configs/config_response_utils.cc

@@ -100,7 +100,7 @@ consteval describe_configs_type property_config_type() {
        std::is_same_v<T, model::cleanup_policy_bitflags> ||
        std::is_same_v<T, model::timestamp_type> ||
        std::is_same_v<T, config::data_directory_path> ||
-        std::is_same_v<T, pandaproxy::schema_registry::subject_name_strategy> || 
+        std::is_same_v<T, pandaproxy::schema_registry::subject_name_strategy> ||


removed trailing whitespace

vbotbuildovich · 2024-10-08T04:04:36Z

new failures in https://buildkite.com/redpanda/redpanda/builds/55964#019269fb-8611-4e0e-ac13-76b3f62a8749:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_valid_settings"

new failures in https://buildkite.com/redpanda/redpanda/builds/55964#01926a00-34a4-458b-b616-878bdc7f3326:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_valid_settings"

non flaky failures in https://buildkite.com/redpanda/redpanda/builds/56007#01926cde-1430-41c7-90d6-85501446db06:

"rptest.tests.license_enforcement_test.LicenseEnforcementTest.test_license_enforcement.clean_node_before_recovery=False.clean_node_after_recovery=True"

non flaky failures in https://buildkite.com/redpanda/redpanda/builds/56054#01926f61-8440-4115-98e5-c50e6578e0e9:

"rptest.tests.topic_recovery_test.TopicRecoveryTest.test_no_data.cloud_storage_type=CloudStorageType.S3"

non flaky failures in https://buildkite.com/redpanda/redpanda/builds/56163#019272e8-8bbe-4c4d-a17e-60d80d5d2f06:

"rptest.tests.topic_recovery_test.TopicRecoveryTest.test_no_data.cloud_storage_type=CloudStorageType.S3"

non flaky failures in https://buildkite.com/redpanda/redpanda/builds/56275#0192775c-3fce-4633-99a9-335b127ef068:

"rptest.tests.describe_topics_test.DescribeTopicsTest.test_describe_topics_with_documentation_and_types"

non flaky failures in https://buildkite.com/redpanda/redpanda/builds/57341#0192dba4-607d-4b39-ab20-96b0efab9dfd:

"rptest.tests.topic_recovery_test.TopicRecoveryTest.test_fast3.cloud_storage_type=CloudStorageType.S3"

non flaky failures in https://buildkite.com/redpanda/redpanda/builds/57557#0192f9c1-b707-4035-acb6-613149a54e6d:

"rptest.tests.describe_topics_test.DescribeTopicsTest.test_describe_topics_with_documentation_and_types"

non flaky failures in https://buildkite.com/redpanda/redpanda/builds/57557#0192f9e1-6036-46a5-891a-36d5dbb760d4:

"rptest.tests.topic_delete_test.TopicDeleteCloudStorageTest.topic_delete_unavailable_test.cloud_storage_type=CloudStorageType.ABS"

non flaky failures in https://buildkite.com/redpanda/redpanda/builds/57557#0192f9e1-6038-4fd8-b0c3-81d499a9bcfe:

"rptest.tests.describe_topics_test.DescribeTopicsTest.test_describe_topics_with_documentation_and_types"

vbotbuildovich · 2024-10-08T17:51:32Z

Retry command for Build#56007

please wait until all jobs are finished before running the slash command

/ci-repeat 1
tests/rptest/tests/license_enforcement_test.py::LicenseEnforcementTest.test_license_enforcement@{"clean_node_after_recovery":true,"clean_node_before_recovery":false}

WillemKauf · 2024-10-09T02:35:07Z

Force push to:

Rebase to upstream dev to fix merge conflicts.

vbotbuildovich · 2024-10-09T04:58:15Z

the below tests from https://buildkite.com/redpanda/redpanda/builds/56054#01926f22-44e0-45a5-a2b1-6b2c6e7963da have failed and will be retried

kafka_server_rpfixture

the below tests from https://buildkite.com/redpanda/redpanda/builds/56054#01926f22-44dd-4ba4-ba1d-d24b156fea17 have failed and will be retried

kafka_server_rpfixture

the below tests from https://buildkite.com/redpanda/redpanda/builds/56275#01927707-5d1c-482c-a4a6-aa6b306a0a0f have failed and will be retried

kafka_server_rpfixture

the below tests from https://buildkite.com/redpanda/redpanda/builds/56275#01927707-5d1e-4522-b2e8-e8425117c06c have failed and will be retried

kafka_server_rpfixture

the below tests from https://buildkite.com/redpanda/redpanda/builds/56670#01929738-f679-4160-9f94-8a2c28bf866f have failed and will be retried

gtest_raft_rpunit

the below tests from https://buildkite.com/redpanda/redpanda/builds/57571#0192fa77-5464-4556-bb08-177b54038aa8 have failed and will be retried

partition_balancer_planner_test_rpunit
datalake_cloud_rpunit

vbotbuildovich · 2024-10-09T05:07:15Z

vbotbuildovich · 2024-10-09T05:07:19Z

Retry command for Build#56054

please wait until all jobs are finished before running the slash command

/ci-repeat 1
tests/rptest/tests/topic_recovery_test.py::TopicRecoveryTest.test_no_data@{"cloud_storage_type":1}

dotnwat

it looks like there is a bisection issue building this commit:

Author: Willem Kaufmann <willem.kaufmann@redpanda.com>
Date:   Wed Sep 4 13:09:21 2024 -0400

    `cluster`: add `topic_multi_property_validation()`

notes:

src/v/cluster/topic_configuration.cc:59:14: error: field designator 'tombstone_retention_ms' does not refer to any field in type 'storage::ntp_config::default_overrides'
   59 |             .tombstone_retention_ms = properties.delete_retention_ms});
      |             ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

WillemKauf · 2024-10-09T17:21:12Z

it looks like there is a bisection issue building this commit

Nice catch, corrected by moving changes to src/v/cluster/topic_configuration.cc to its proper commit (storage: add tombstone_retention_ms to ntp_config)

dotnwat

is bisect still an issue on that same commit

git fetch upstream
git checkout 7b2b010eedf5c56f5887985f943c7ea9ddab39bd
git rebase 9e810f72f373ad3df8ff86fbef9a3d75b14aaa15
bazel build //...

src/v/cluster/topic_properties.cc:152:9: error: no member named 'tombstone_retention_ms' in 'storage::ntp_config::default_overrides'
  152 |     ret.tombstone_retention_ms = delete_retention_ms;
      |     ~~~ ^

WillemKauf · 2024-10-09T18:35:00Z

is bisect still an issue on that same commit

Yup, missed another line, my bad.

Corrected by moving changes in src/v/cluster/topic_properties.cc to its proper commit (storage: add tombstone_retention_ms to ntp_config)

vbotbuildovich · 2024-10-09T21:39:38Z

Retry command for Build#56163

please wait until all jobs are finished before running the slash command

/ci-repeat 1
tests/rptest/tests/topic_recovery_test.py::TopicRecoveryTest.test_no_data@{"cloud_storage_type":1}

Feediver1

lgtm

src/v/config/configuration.cc

WillemKauf · 2024-10-10T15:20:57Z

Force push to:

Rebase to upstream dev to fix merge conflicts
Apply requested docs changes to config/configuration.cc.

vbotbuildovich · 2024-10-11T20:47:08Z

Retry command for Build#56370

please wait until all jobs are finished before running the slash command

/ci-repeat 1
tests/rptest/tests/cloud_storage_timing_stress_test.py::CloudStorageTimingStressTest.test_cloud_storage@{"cleanup_policy":"delete"}

WillemKauf · 2024-10-16T09:13:19Z

/ci-repeat 1
tests/rptest/tests/cloud_storage_timing_stress_test.py::CloudStorageTimingStressTest.test_cloud_storage@{"cleanup_policy":"delete"}

WillemKauf · 2024-10-16T15:36:19Z

/ci-repeat 1
release

WillemKauf · 2024-10-16T21:23:52Z

/ci-repeat 1
release
tests/rptest/tests/describe_topics_test.py::DescribeTopicsTest.test_describe_topics

WillemKauf · 2024-10-25T03:07:01Z

Force push to:

Rebase to upstream dev to fix merge conflicts

dotnwat

looks pretty solid to me. just a few questions/comments

src/v/cluster/topic_table.cc

src/v/storage/ntp_config.h

dotnwat · 2024-10-25T21:16:34Z

src/v/kafka/server/handlers/configs/config_response_utils.cc

+tristate<T> override_if_not_default(
+  const tristate<T>& overrides, const std::optional<T>& def) {
+    // Overrides is {disabled}, cluster default is {value}
+    if (overrides.is_disabled() && def.has_value()) {


why does it matter if def.has_value here?

For the two cases you have commented on (// Overrides is {disabled}, cluster default is {value}, // Overrides is {std::nullopt}, cluster default is {value}), note that the following case in which (overrides.is_empty() || overrides.is_disabled()) && !def.has_value() is excluded.

I.e, I'm treating the override and default as "equivalent" when the tristate is either disabled or empty, and the optional is in the std::nullopt state.

This makes a lot of logical sense for overrides.is_empty() && !def.has_value(), but I recognize overrides.is_disabled() && !def.has_value() is a bit more of a stretch. However, in the case of delete.retention.ms, its default state is often disabled, and the corresponding cluster property tombstone_retention_ms's default state is often std::nullopt. This, IMO, provides motivation for considering this state in override_if_not_default() in the way it is currently written.

override is considered equivalent to the default are

do you think we could add to the comment a truth table for all 6 cases (or maybe it's 9 if we count the cases where the default has a value and its actually equal to the tristate rather than just semantics equal). then we could use the retention.ms and truth table as an example for override_if_not_default usage.

Added truth table in a code comment

src/v/kafka/server/handlers/configs/config_response_utils.cc

src/v/cluster/metadata_cache.cc

src/v/kafka/server/handlers/incremental_alter_configs.cc

src/v/kafka/server/handlers/topics/types.h

dotnwat · 2024-10-25T21:36:20Z

src/v/kafka/server/handlers/topics/types.h

+// Either parse configuration or return nullopt
+inline std::optional<bool>
+get_bool_value(const config_map_t& config, std::string_view key) {
+    if (auto it = config.find(key); it != config.end()) {
+        try {
+            // Ignore case.
+            auto str_value = it->second;
+            std::transform(
+              str_value.begin(),
+              str_value.end(),
+              str_value.begin(),
+              [](const auto& c) { return std::tolower(c); });
+            return string_switch<std::optional<bool>>(str_value)
+              .match("true", true)
+              .match("false", false);
+        } catch (const std::runtime_error&) {
+            throw boost::bad_lexical_cast();
+        };
+    }
+    return std::nullopt;
+}
+
+inline model::shadow_indexing_mode
+get_shadow_indexing_mode(const config_map_t& config) {


did these need to move to the header or could we have added a declaration to the header? or alternatively, could the implementation of delete_retention_ms_validator have been in the same .cc file?

I think it is preferred to leave the implementation of delete_retention_ms_validator in validators.h to keep parity with the other definitions.

So, we could leave this as is with the inline definitions for get_bool_value() and get_shadow_indexing_mode() in the header, or I could change it to what you proposed with declarations in types.h and implementations in types.cc.

Please let me know what you prefer.

the general rule of thumb is to be reducing the amount of code in headers. its not any sort of hard and fast rule, but i don't see in this case why it needs to move to the header. is there a dependency issue that makes it hard to do?

Moved declaration/implementation

src/v/cluster/metadata_cache.cc

vbotbuildovich · 2024-10-30T06:10:32Z

Retry command for Build#57341

please wait until all jobs are finished before running the slash command

/ci-repeat 1
tests/rptest/tests/topic_recovery_test.py::TopicRecoveryTest.test_fast3@{"cloud_storage_type":1}

Plumb the `delete_retention_ms` config through `topic_configuration`, and `topic_properties`. Also modify necessary compatibility sites. Also modify `tools/offline_log_viewer` to be compatible with the updated `serde`.

When applying `update_topic_properties_cmd`, there may be some checks that depend on multiple properties. The current motivating case is ensuring that `delete.retention.ms` is not enabled at the same time as any tiered storage properties. Add `topic_table::topic_multi_property_validation()` to perform these checks and abort the topic property update if the `properties` are found to be invalid.

The storage layer will now utilize a topic's configured `delete.retention.ms` parameter for housekeeping. At the `ntp_config` layer, this is referred to as `tombstone_retention_ms`.

WillemKauf · 2024-11-01T14:47:42Z

Force push to:

Rebase to upstream dev to fix merge conflict
Fix bug in std::ostream& operator<<(std::ostream& o, const topic_properties& properties) (extra comma)

andrwng

Sorry for the delay looking at this. Looking pretty nice and complete, but have a few nits and questions about code placement themed around simplifying and future-proofing

src/v/kafka/server/handlers/configs/config_response_utils.cc

src/v/kafka/server/handlers/topics/validators.h

src/v/config/configuration.cc

src/v/kafka/server/handlers/topics/types.cc

tests/rptest/tests/delete_retention_ms_test.py

src/v/kafka/server/handlers/configs/config_response_utils.cc

andrwng · 2024-11-04T19:35:22Z

tests/rptest/tests/delete_retention_ms_test.py

+        #We cannot enable delete.retention.ms if any cloud storage properties are also enabled.
+        test_cases = [


Does this cover the entirety of the truth table from the earlier commit? Might be worth duplicating the truth table here

Also nit: space between # and the first word

No it does not, and the test was not really intended to test the truth table for the overrides- more so tiered storage and delete.retention.ms property interaction.

Got it. Does it make sense to include a test case for just the delete.retention.ms behavior without tiered storage involvement?

test_alter_topic_config covers this already I think.

Similar in intention to the `override_if_not_default` overload for `std::optional<>`. This ensures that `tristate` topic properties may still display `DEFAULT_CONFIG` instead of `DYNAMIC_TOPIC_CONFIG` in the case that their value is `{disabled}` and the cluster default is `std::nullopt`. This is useful for topic properties that may want to default to a `{disabled}` state without indicating `DYNAMIC_TOPIC_CONFIG` as a source.

Adds configuration for the topic property `delete.retention.ms` through the `CreateTopic`, `AlterConfig`, and `IncrementalAlterConfig` Kafka APIs.

WillemKauf · 2024-11-04T23:24:35Z

src/v/kafka/server/handlers/topics/types.h

+// no value                =>  tristate.has_value() == false;
+// value present           =>  tristate.has_value() == true;
+
+template<typename T>


Unfortunate that this template function had to be moved to the header in order to use it in validators.h.

vbotbuildovich · 2024-11-05T02:05:01Z

Retry command for Build#57557

please wait until all jobs are finished before running the slash command

/ci-repeat 1
tests/rptest/tests/describe_topics_test.py::DescribeTopicsTest.test_describe_topics_with_documentation_and_types
tests/rptest/tests/topic_delete_test.py::TopicDeleteCloudStorageTest.topic_delete_unavailable_test@{"cloud_storage_type":2}

Adding support for `tombstone_retention_ms`, which is a cluster property that is equivalent to Kafka's `log.cleaner.delete.retention.ms`, but is more precisely named, and represents the retention time for tombstone records in compacted topics in `redpanda`. This controls the default value for the topic level property `delete.retention.ms`.

WillemKauf · 2024-11-05T03:51:00Z

Force push to:

Fix doc string in failing describe_topics_test.py ducktape test post changes in configuration.cc

WillemKauf requested review from dotnwat, andrwng and mattschumpert October 7, 2024 22:24

WillemKauf requested a review from a team as a code owner October 7, 2024 22:24

github-actions bot added the area/redpanda label Oct 7, 2024

WillemKauf commented Oct 7, 2024

View reviewed changes

src/v/storage/ntp_config.h Outdated Show resolved Hide resolved

WillemKauf force-pushed the tombstones_config branch from 9c73ed7 to a6d89fd Compare October 7, 2024 22:44

WillemKauf commented Oct 7, 2024

View reviewed changes

WillemKauf force-pushed the tombstones_config branch from a6d89fd to f737a4d Compare October 8, 2024 01:24

WillemKauf force-pushed the tombstones_config branch from f737a4d to ebf9304 Compare October 8, 2024 12:19

WillemKauf force-pushed the tombstones_config branch from ebf9304 to 4f52fec Compare October 9, 2024 02:34

WillemKauf force-pushed the tombstones_config branch from 4f52fec to f9cf04f Compare October 9, 2024 13:39

dotnwat reviewed Oct 9, 2024

View reviewed changes

WillemKauf force-pushed the tombstones_config branch from f9cf04f to 7a6ad8c Compare October 9, 2024 17:21

dotnwat reviewed Oct 9, 2024

View reviewed changes

WillemKauf force-pushed the tombstones_config branch from 7a6ad8c to ece7cc1 Compare October 9, 2024 18:33

Feediver1 previously approved these changes Oct 10, 2024

View reviewed changes

Deflaimun reviewed Oct 10, 2024

View reviewed changes

src/v/config/configuration.cc Outdated Show resolved Hide resolved

WillemKauf dismissed Feediver1’s stale review via 71c4da7 October 10, 2024 15:20

WillemKauf force-pushed the tombstones_config branch from ece7cc1 to 71c4da7 Compare October 10, 2024 15:20

WillemKauf force-pushed the tombstones_config branch from 7f15d42 to 1410957 Compare October 25, 2024 03:06

dotnwat reviewed Oct 25, 2024

View reviewed changes

WillemKauf requested a review from dotnwat October 29, 2024 14:07

WillemKauf force-pushed the tombstones_config branch from 1410957 to c6eafb3 Compare October 30, 2024 02:35

WillemKauf force-pushed the tombstones_config branch from c6eafb3 to 1b9b437 Compare November 1, 2024 14:44

WillemKauf added 3 commits November 1, 2024 10:46

cluster: add delete_retention_ms to topic_configuration

9f36a3e

Plumb the `delete_retention_ms` config through `topic_configuration`, and `topic_properties`. Also modify necessary compatibility sites. Also modify `tools/offline_log_viewer` to be compatible with the updated `serde`.

storage: add tombstone_retention_ms to ntp_config

89bfffa

The storage layer will now utilize a topic's configured `delete.retention.ms` parameter for housekeeping. At the `ntp_config` layer, this is referred to as `tombstone_retention_ms`.

WillemKauf force-pushed the tombstones_config branch from 1b9b437 to 2375c86 Compare November 1, 2024 14:46

andrwng reviewed Nov 4, 2024

View reviewed changes

WillemKauf added 2 commits November 4, 2024 18:20

kafka: add delete.retention.ms to topic config handler

526f52f

Adds configuration for the topic property `delete.retention.ms` through the `CreateTopic`, `AlterConfig`, and `IncrementalAlterConfig` Kafka APIs.

WillemKauf force-pushed the tombstones_config branch from 2375c86 to 877b01f Compare November 4, 2024 23:22

WillemKauf commented Nov 4, 2024

View reviewed changes

andrwng previously approved these changes Nov 4, 2024

View reviewed changes

WillemKauf added 2 commits November 4, 2024 22:49

rptest: add delete_retention_ms_test.py

817afe9

WillemKauf dismissed andrwng’s stale review via 817afe9 November 5, 2024 03:50

WillemKauf force-pushed the tombstones_config branch from 877b01f to 817afe9 Compare November 5, 2024 03:50

andrwng approved these changes Nov 5, 2024

View reviewed changes

WillemKauf merged commit 8a949ed into redpanda-data:dev Nov 5, 2024
17 checks passed

		#We cannot enable delete.retention.ms if any cloud storage properties are also enabled.
		test_cases = [

cluster: add tombstone_retention_ms/delete.retention.ms #23662

cluster: add tombstone_retention_ms/delete.retention.ms #23662

Conversation

WillemKauf commented Oct 7, 2024 • edited Loading

Backports Required

Release Notes

Features

Choose a reason for hiding this comment

vbotbuildovich commented Oct 8, 2024 • edited Loading

vbotbuildovich commented Oct 8, 2024

Retry command for Build#56007

WillemKauf commented Oct 9, 2024

vbotbuildovich commented Oct 9, 2024 • edited Loading

vbotbuildovich commented Oct 9, 2024 • edited Loading

vbotbuildovich commented Oct 9, 2024

Retry command for Build#56054

dotnwat left a comment

Choose a reason for hiding this comment

WillemKauf commented Oct 9, 2024

dotnwat left a comment

Choose a reason for hiding this comment

WillemKauf commented Oct 9, 2024 • edited Loading

vbotbuildovich commented Oct 9, 2024

Retry command for Build#56163

Feediver1 left a comment

Choose a reason for hiding this comment

WillemKauf commented Oct 10, 2024

vbotbuildovich commented Oct 11, 2024

Retry command for Build#56370

WillemKauf commented Oct 16, 2024

WillemKauf commented Oct 16, 2024

WillemKauf commented Oct 16, 2024

WillemKauf commented Oct 25, 2024 • edited Loading

dotnwat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillemKauf Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbotbuildovich commented Oct 30, 2024

Retry command for Build#57341

WillemKauf commented Nov 1, 2024

andrwng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbotbuildovich commented Nov 5, 2024 • edited Loading

Retry command for Build#57557

WillemKauf commented Nov 5, 2024

`cluster`: add `tombstone_retention_ms`/`delete.retention.ms` #23662

`cluster`: add `tombstone_retention_ms`/`delete.retention.ms` #23662

WillemKauf commented Oct 7, 2024 •

edited

Loading

vbotbuildovich commented Oct 8, 2024 •

edited

Loading

vbotbuildovich commented Oct 9, 2024 •

edited

Loading

vbotbuildovich commented Oct 9, 2024 •

edited

Loading

WillemKauf commented Oct 9, 2024 •

edited

Loading

WillemKauf commented Oct 25, 2024 •

edited

Loading

WillemKauf Oct 28, 2024 •

edited

Loading

vbotbuildovich commented Nov 5, 2024 •

edited

Loading