[CORE-8161] `storage`: add `min.cleanable.dirty.ratio`, schedule compaction by `dirty_ratio` #24991

WillemKauf · 2025-01-31T17:48:09Z

Based on PR #24649.

WIP, tests to be added.

Instead of unconditionally compacting all logs during a round of housekeeping, users may now optionally schedule log compaction in the log_manager using the cluster/topic property min_cleanable_dirty_ratio/min.cleanable.dirty.ratio.

As mentioned in the above PR,

The dirty ratio of a log is defined as the ratio between the number of bytes in "dirty" segments and the total number of bytes in closed segments.
Dirty segments are closed segments which have not yet been cleanly compacted- i.e, duplicates for keys in this segment could be found in the prefix of the log up to this segment.

By setting the min.cleanable.dirty.ratio on a per topic basis, users can avoid unnecessary read/write amplification during compaction as the log grows in size.

A housekeeping scan will still be performed every log_compaction_interval_ms, and the log's dirty_ratio will be tested against min.cleanable.dirty.ratio in determining it's eligibility for compaction. Additionally, logs are now compacted in descending order according to their dirty ratio, offering a better "bang for buck" heuristic for compaction scheduling.

Backports Required

Release Notes

Improvements

Allow for optional scheduling of compaction via min_cleanable_dirty_ratio

Adds the book-keeping variables `_dirty/closed_segment_bytes` to `disk_log_impl`, as well as some getter/setter functions. These functions will be used throughout `disk_log_impl` where required (segment rolling, compaction, segment eviction) to track the bytes contained in dirty and closed segments.

Uses the added functions `update_dirty/closed_segment_bytes()` in the required locations within `disk_log_impl` in order to bookkeep the dirty ratio. Bytes can be either removed or added by rolling new segments, compaction, and retention enforcement.

vbotbuildovich · 2025-02-01T00:26:01Z

Retry command for Build#61464

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/partition_force_reconfiguration_test.py::NodeWiseRecoveryTest.test_node_wise_recovery@{"dead_node_count":2}

vbotbuildovich · 2025-02-01T01:55:27Z

CI test results

test results on build#61464

test_id	test_kind	job_url	test_status	passed
gtest_raft_rpunit.gtest_raft_rpunit	unit	https://buildkite.com/redpanda/redpanda/builds/61464#0194be30-4fe3-4201-a506-11eca43a36e9	FLAKY	1/2
kafka_server_rpfixture.kafka_server_rpfixture	unit	https://buildkite.com/redpanda/redpanda/builds/61464#0194be30-4fe3-4201-a506-11eca43a36e9	FAIL	0/2
rptest.tests.cloud_storage_scrubber_test.CloudStorageScrubberTest.test_scrubber.cloud_storage_type=CloudStorageType.S3	ducktape	https://buildkite.com/redpanda/redpanda/builds/61464#0194be87-f8fb-4fbd-9207-0641084924fe	FLAKY	1/2
rptest.tests.describe_topics_test.DescribeTopicsTest.test_describe_topics_with_documentation_and_types	ducktape	https://buildkite.com/redpanda/redpanda/builds/61464#0194be87-f8f9-4197-b75d-9e2b0e669087	FAIL	0/20
rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.S3	ducktape	https://buildkite.com/redpanda/redpanda/builds/61464#0194be87-f8fb-4fbd-9207-0641084924fe	FLAKY	1/2
rptest.tests.partition_force_reconfiguration_test.NodeWiseRecoveryTest.test_node_wise_recovery.dead_node_count=2	ducktape	https://buildkite.com/redpanda/redpanda/builds/61464#0194be87-f8fb-4fbd-9207-0641084924fe	FAIL	0/20
rptest.tests.retention_policy_test.RetentionPolicyTest.test_changing_topic_retention_with_restart.cloud_storage_type=CloudStorageType.ABS	ducktape	https://buildkite.com/redpanda/redpanda/builds/61464#0194be87-f8f9-4197-b75d-9e2b0e669087	FLAKY	1/3
test_compat_rpunit.test_compat_rpunit	unit	https://buildkite.com/redpanda/redpanda/builds/61464#0194be2d-b364-4273-8107-604c1a24978e	FAIL	0/2

test results on build#61480

test_id	test_kind	job_url	test_status	passed
gtest_raft_rpunit.gtest_raft_rpunit	unit	https://buildkite.com/redpanda/redpanda/builds/61480#0194c30c-66c7-425f-882b-a62f163db808	FLAKY	1/2
gtest_raft_rpunit.gtest_raft_rpunit	unit	https://buildkite.com/redpanda/redpanda/builds/61480#0194c30c-66c8-4db9-a2da-7dc5f0262638	FLAKY	1/2
rptest.tests.e2e_shadow_indexing_test.EndToEndThrottlingTest.test_throttling.cloud_storage_type=CloudStorageType.S3	ducktape	https://buildkite.com/redpanda/redpanda/builds/61480#0194c368-0af7-4d62-86e0-9ef129bfeb88	FLAKY	1/2
rptest.tests.offset_for_leader_epoch_archival_test.OffsetForLeaderEpochArchivalTest.test_querying_archive	ducktape	https://buildkite.com/redpanda/redpanda/builds/61480#0194c368-0af6-4ff1-92d6-ad77df6dc1f7	FLAKY	1/5
rptest.tests.partition_force_reconfiguration_test.NodeWiseRecoveryTest.test_node_wise_recovery.dead_node_count=2	ducktape	https://buildkite.com/redpanda/redpanda/builds/61480#0194c368-0af7-4d62-86e0-9ef129bfeb88	FAIL	0/20

vbotbuildovich · 2025-02-01T23:40:34Z

Retry command for Build#61480

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

/ci-repeat 1
tests/rptest/tests/partition_force_reconfiguration_test.py::NodeWiseRecoveryTest.test_node_wise_recovery@{"dead_node_count":2}

src/v/storage/probe.cc

dotnwat · 2025-02-04T04:45:55Z

src/v/cluster/metadata_cache.cc

+std::optional<double>
+metadata_cache::get_default_min_cleanable_dirty_ratio() const {
+    return config::shard_local_cfg().min_cleanable_dirty_ratio();
+}


i'm wondering if this is something that we want to expose as a topic config right now, or we keep it internal for a while?

Is there a reason you would like to keep this config internal for the time being?

I could see an argument for releasing all of min.cleanable.dirty.ratio/min.compaction.lag.ms/max.compaction.lag.ms as configs all at once, but I also think there is use for min.cleanable.dirty.ratio as a standalone property for now.

Is this your reasoning as well?

Is there a reason you would like to keep this config internal for the time being?

just that no one is asking for it

There have been some internal discussions around the benefits of min.cleanable.dirty.ratio reducing read/write amplification, but fair point.

got it, makes sense. presumably you could bail out of the loop early based on some condition. for example, the while loop already does this in the while condition. maybe there isn't a natural way to do that, i'm not sure.

Yeah, we can just check again for the existence of the ntp in the ntp_by_compaction_heuristic map, shift the list and continue without compacting so we eventually hit the "front" of the list again. Sub-optimal but least intrusive change.

dotnwat · 2025-02-04T04:55:22Z

src/v/storage/log_manager.cc

@@ -260,29 +293,28 @@ log_manager::housekeeping_scan(model::timestamp collection_threshold) {
        co_await compaction_map->initialize(compaction_mem_bytes);
        _compaction_hash_key_map = std::move(compaction_map);
    }
-    while (!_logs_list.empty()
-           && is_not_set(_logs_list.front().flags, bflags::compacted)) {


i don't see us checking/changing the compacted flags now. is that changed?

alternatively, did you consider just sorting _logs_list? you could leave this while loop unchanged, and before it, iterate over the btree, use get() to access the intrusive hook, and then in O(1) move the entries to the head of the list.

This is a good idea, thanks. Wasn't sure of the best way to manage both of the housekeeping meta as well as the sorted ntps. This sounds like a good solution!

your changes look ok too, but we've had a bunch of concurrency bugs related to that intrusive list in the past. i think it'd be nice to just sort it how we want, and otherwise leave it untouched. wdyt?

i was under the impression that it woudl be fine to compact any partition in that loop, we were only interested in which order compaction occurred

With this PR, the changes (as they stand) do two things:

The housekeeping scan no longer indiscriminately compacts every partitions, but instead only compacts partitions with a dirty_ratio above min.cleanable.dirty.ratio (if it is set at the cluster or topic level).

The ordering of compaction for partitions is decided using the dirty_ratio as a heuristic.

So, we aren't looking to compact every partition in that loop.

Sorry, it looks like my original comment got deleted accidentally, for context I wrote:

I agree with your proposed changes, I'm just wondering how the while loop would remain unchanged when we no longer are looking to compact every log in _logs_list (i.e, the logic of is_not_set() for the front entry of _logs_list returning false no longer applies.)

yeh i think we could bail out of the loop (or skip items)?

asimms41 · 2025-02-07T16:24:52Z

src/v/config/configuration.cc

+  , min_cleanable_dirty_ratio(
+      *this,
+      "min_cleanable_dirty_ratio",
+      "The minimum ratio between between the number of bytes in \"dirty\" "


Suggested change

"The minimum ratio between between the number of bytes in \"dirty\" "

"The minimum ratio between the number of bytes in \"dirty\" "

asimms41 · 2025-02-07T16:25:44Z

src/v/config/configuration.cc

+      "min_cleanable_dirty_ratio",
+      "The minimum ratio between between the number of bytes in \"dirty\" "
+      "segments and the total number of bytes in closed segments that must be "
+      "reached before a partition's log is considered eligible for compaction, "


Suggested change

"reached before a partition's log is considered eligible for compaction, "

"reached before a partition's log is eligible for compaction "

asimms41 · 2025-02-07T16:26:46Z

tests/rptest/tests/describe_topics_test.py

+                config_type="DOUBLE",
+                value="-1",
+                doc_string=
+                "The minimum ratio between between the number of bytes in \"dirty\" "


Suggested change

"The minimum ratio between between the number of bytes in \"dirty\" "

"The minimum ratio between the number of bytes in \"dirty\" "

asimms41 · 2025-02-07T16:27:20Z

tests/rptest/tests/describe_topics_test.py

+                doc_string=
+                "The minimum ratio between between the number of bytes in \"dirty\" "
+                "segments and the total number of bytes in closed segments that must be "
+                "reached before a partition's log is considered eligible for compaction, "


Suggested change

"reached before a partition's log is considered eligible for compaction, "

"reached before a partition's log is eligible for compaction, "

Suggested change

"reached before a partition's log is considered eligible for compaction, "

"reached before a partition's log is considered eligible for compaction "

WillemKauf added 6 commits January 30, 2025 21:46

storage: add dirty/closed_segment_bytes to probe

967e908

storage: add dirty_ratio test to storage_e2e_test.cc

11db330

rptest: add dirty ratio testing to log_compaction_test.py

43e704e

config: add min_cleanable_dirty_ratio to configuration

6779898

WillemKauf requested a review from a team as a code owner January 31, 2025 17:48

github-actions bot added the area/redpanda label Jan 31, 2025

WillemKauf force-pushed the min_cleanable_dirty_ratio branch 2 times, most recently from db600d4 to 78b2928 Compare January 31, 2025 21:02

WillemKauf added 5 commits February 1, 2025 14:42

cluster: add min_cleanable_dirty_ratio to topic_properties

a7cdceb

cluster: add min_cleanable_dirty_ratio to ntp_config

4126d49

kafka: add min.cleanable.dirty.ratio to topic config handlers

8675d9e

storage: make dirty_ratio a pure virtual function in log

a67c4bf

storage: schedule compaction by dirty_ratio

7a6aab3

WillemKauf force-pushed the min_cleanable_dirty_ratio branch from 78b2928 to 7a6aab3 Compare February 1, 2025 19:43

WillemKauf changed the title ~~storage: schedule compaction by dirty_ratio~~ storage: add min.cleanable.dirty.ratio, schedule compaction by dirty_ratio Feb 1, 2025

WillemKauf changed the title ~~storage: add min.cleanable.dirty.ratio, schedule compaction by dirty_ratio~~ [CORE-8161] storage: add min.cleanable.dirty.ratio, schedule compaction by dirty_ratio Feb 1, 2025

dotnwat reviewed Feb 4, 2025

View reviewed changes

asimms41 reviewed Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE-8161] `storage`: add `min.cleanable.dirty.ratio`, schedule compaction by `dirty_ratio` #24991

[CORE-8161] `storage`: add `min.cleanable.dirty.ratio`, schedule compaction by `dirty_ratio` #24991

WillemKauf commented Jan 31, 2025 •

edited

Loading

vbotbuildovich commented Feb 1, 2025 •

edited

Loading

vbotbuildovich commented Feb 1, 2025 •

edited

Loading

vbotbuildovich commented Feb 1, 2025

dotnwat Feb 4, 2025

WillemKauf Feb 7, 2025

dotnwat Feb 7, 2025

WillemKauf Feb 7, 2025

dotnwat Feb 7, 2025

WillemKauf Feb 7, 2025

dotnwat Feb 4, 2025

WillemKauf Feb 4, 2025

dotnwat Feb 5, 2025

dotnwat Feb 7, 2025

WillemKauf Feb 7, 2025

WillemKauf Feb 7, 2025

dotnwat Feb 7, 2025

asimms41 Feb 7, 2025

asimms41 Feb 7, 2025 •

edited

Loading

asimms41 Feb 7, 2025

asimms41 Feb 7, 2025

	"The minimum ratio between between the number of bytes in \"dirty\" "
	"The minimum ratio between the number of bytes in \"dirty\" "

	"reached before a partition's log is considered eligible for compaction, "
	"reached before a partition's log is eligible for compaction "

[CORE-8161] storage: add min.cleanable.dirty.ratio, schedule compaction by dirty_ratio #24991

Are you sure you want to change the base?

[CORE-8161] storage: add min.cleanable.dirty.ratio, schedule compaction by dirty_ratio #24991

Conversation

WillemKauf commented Jan 31, 2025 • edited Loading

Backports Required

Release Notes

Improvements

vbotbuildovich commented Feb 1, 2025 • edited Loading

Retry command for Build#61464

vbotbuildovich commented Feb 1, 2025 • edited Loading

CI test results

vbotbuildovich commented Feb 1, 2025

Retry command for Build#61480

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

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

asimms41 Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[CORE-8161] `storage`: add `min.cleanable.dirty.ratio`, schedule compaction by `dirty_ratio` #24991

[CORE-8161] `storage`: add `min.cleanable.dirty.ratio`, schedule compaction by `dirty_ratio` #24991

WillemKauf commented Jan 31, 2025 •

edited

Loading

vbotbuildovich commented Feb 1, 2025 •

edited

Loading

vbotbuildovich commented Feb 1, 2025 •

edited

Loading

asimms41 Feb 7, 2025 •

edited

Loading