`storage`: compaction enhancements #23380

WillemKauf · 2024-09-19T02:20:31Z

PR Summary

Minor tombstone additions/tweaks:

Replace internal::get_tombstone_delete_horizon() with internal::is_past_tombstone_delete_horizon() for clean-up of logic and reduced comparisons.
The addition of the segment::may_have_tombstone_records() bitflag.
The addition of the may_have_removable_tombstones() function.

A number of optimizations to compaction, both in general and for tombstones:

Per the comment here, the behaviour of sliding window compaction is changed, such that newly added/closed segments are ignored until the current "round" of sliding window compaction cleanly compacts all segments down to the start of the log. This allows for sliding window compaction to continually produce clean segments, and avoid a situation where clean segments are not being produced due to a high ingress rate or key cardinality (this situation would prevent timely tombstone removal). _last_compaction_window_offset must reach the base offset of the first segment in the log before new segments will be considered in the compaction window.
Per the comment here, segments that are cleanly compacted or have been through a round of window compaction are considered in the sliding window range. However, it would be a no-op to actually perform window compaction over these segments. Self-compaction is performed to remove tombstones on segments that may contain them, and all cleanly compacted segments are removed before sliding window compaction occurs. This allows for timely tombstone removal by avoiding the situation in which a partition which is no longer being produced to can still trigger tombstone removal.
Cleanly compacted segments do not need to have their keys added to the compaction offset map, since the deduplication process considers unindexed keys to be valid records to keep. By not indexing these segments, the compaction process can use less memory and cleanly compact down to the start of the log faster.

Backports Required

Release Notes

Improvements

A number of optimizations to local storage compaction.

Replace `internal::get_tombstone_delete_horizon()` with `internal::is_past_tombstone_delete_horizon()`. This change makes the code a little bit cleaner, and potentially reduces the number of operations in `should_keep()` related functions, as the current timestamp only needs to be evaluated once for every tombstone record in a segment with a clean compaction timestamp. Also remove `should_remove_tombstone_record()`, since its logic is now trivial.

WillemKauf · 2024-09-19T02:26:34Z

Force push to:

format with clang-format-18. I really need to update my default clang-format version.

vbotbuildovich · 2024-09-19T04:58:51Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54716#0192085b-a525-4008-93ae-ef3c61748c59

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54716#0192085b-a528-4008-9518-61df56f770e7

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54786#01920d19-c098-4a03-9a7d-58fde00f7d0c

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/54996#0192226e-4d2f-44ef-a40e-14c2c198216d

src/v/storage/segment.h

src/v/storage/disk_log_impl.cc

WillemKauf · 2024-09-19T12:42:42Z

src/v/storage/segment_utils.cc

-    if (s->finished_self_compaction() || !s->has_compactible_offsets(cfg)) {
+    if (
+      (s->finished_self_compaction() || !s->has_compactible_offsets(cfg))
+      && !may_have_removable_tombstones(s, cfg)) {


Very important for allowing self compaction to remove tombstones, even if the segment has already been through a round of self compaction.

WillemKauf · 2024-09-19T12:43:29Z

src/v/storage/disk_log_impl.cc

-        if (seg->finished_self_compaction()) {
-            continue;
-        }
+


The removed code here was a superfluous check that we perform in self_compact_segment() anyways. move continue logic after call by checking for result.did_compact() == false.

WillemKauf · 2024-09-19T12:43:47Z

src/v/storage/disk_log_impl.h

@@ -197,6 +197,11 @@ class disk_log_impl final : public log {
        _last_compaction_window_start_offset = o;
    }

+    const std::optional<model::offset>&
+    get_last_compaction_window_start_offset() const {


For testing.

src/v/storage/segment_deduplication_utils.cc

src/v/storage/segment.h

src/v/storage/segment_utils.cc

src/v/storage/disk_log_impl.cc

andrwng · 2024-09-19T16:56:07Z

src/v/storage/disk_log_impl.cc

+        for (const auto& seg : segs) {
+            if (internal::may_have_removable_tombstones(seg, cfg)) {
+                filtered_buf.emplace_back(seg);


I'm not a fan of how this means the returned range is no longer contiguous segments. Can we put the onus for checking this on callers?

Changed behaviour of find_sliding_range() to once again return a contiguous range of segments- however, this range may include segments already marked as being cleanly compacted/having finished window compaction, and the onus is on the caller to filter them as they see fit.

andrwng · 2024-09-19T17:05:52Z

src/v/storage/disk_log_impl.cc

+        if (
+           _last_compaction_window_start_offset.has_value()
+           && (seg->offsets().get_base_offset()
+                >= _last_compaction_window_start_offset.value())) {
+            // Force clean segment production by compacting down to the
+            // start of the log before considering new segments in the
+            // compaction window.
+            break;


It seems possible that segments could have been removed such that the first segment falls above _last_compaction_window_start_offset. In that case, does this always return empty and we get stuck never compacting?

Maybe we should reset _last_compaction_window_start_offset at the top of this method. Then we wouldn't need to worry about resetting it at compaction time

Good callout. Added a new sanity check to the top of find_sliding_range() to reset the _last_compaction_window_start_offset if it is <= _segs.front()->offsets().get_base_offset().

However, the code which resets it in sliding_window_compact is still required- we want to compare idx_start_offset to the base offset of the first segment in our sliding window range (which is not necessarily the front segment in the log) as an indicator of whether or not we can reset the start offset and allow new segments into the range.

src/v/storage/disk_log_impl.cc

src/v/storage/segment_deduplication_utils.cc

andrwng · 2024-09-19T17:13:48Z

src/v/storage/segment_deduplication_utils.cc

@@ -116,6 +116,25 @@ ss::future<model::offset> build_offset_map(
            cfg.asrc->check();
        }
        auto seg = *iter;
+        if (seg->index().has_clean_compact_timestamp()) {


nit: just making sure, is there anywhere in code comments that describes what it means to be clean compacted?

For future changes for sliding window compaction scheduling, it is helpful to have a flag that indicates whether a segment may contain tombstone records or not. Add a new `bool` field, `may_have_tombstone_records` to the `index_state`, and mark/unmark its value during segment deduplication and data copying. This field is `true` by default, which can lead to false-positives. A segment is only considered to not have tombstone records after proven by de-duplication/ segment data copying in the compaction process.

Adds a helper function that indicates whether or not a segment may have tombstones eligible for deletion. This can return false-positives, since any segment that has not yet been through the compaction process is assumed to potentially have tombstones until proven otherwise.

WillemKauf · 2024-09-20T00:40:06Z

src/v/storage/disk_log_impl.cc

-    //
-    // If there are new segments that have not been compacted, we can't make
-    // this claim, and compact everything again.
-    if (


It may look a bit scary to remove this chunk of code, but this is functionally performing the same "clean-compacted" check we now perform here (since, logically, is_clean_compacted is defined this way here.)

We want to leave these segments in the compaction range so that they can be self-compacted in the case that they contain tombstones, and then filter them out afterwards before proceeding to sliding window compaction.

This commit does two things, which seemed easier to combine into one commit: 1. The behavior of sliding window compaction is changed, such that newly added/closed segments are ignored until the current "round" of sliding window compaction cleanly compacts all segments down to the start of the range. This allows for sliding window compaction to avoid a situation where clean segments are not being produced due to a high ingress rate or key cardinality (this situation would prevent timely tombstone removal). `_last_compaction_window_offset` must reach the base offset of the first segment in the currently active window before new segments will be considered for compaction. 2. Segments that are cleanly compacted or have been through a round of window compaction are considered in the sliding window range. However, it would be a no-op to actually perform window compaction over these segments. Self-compaction is performed to remove tombstones on segments that may contain them, and all cleanly compacted segments are removed before sliding window compaction occurs. This allows for timely tombstone removal by avoiding the situation in which a partition which is no longer being produced to can still trigger tombstone removal. Both of these changes improve the rate at which tombstone removal occurs, and help prevent clean segment/tombstone removal "starvation".

Cleanly compacted segments do not need to have their keys added to the compaction offset map, since the deduplication process considers unindexed keys to be valid records to keep. By not indexing these segments, the compaction process can use less memory and cleanly compact down to the start of the log faster.

WillemKauf · 2024-09-24T03:55:26Z

Force push to:

Fix logic bug in self_compact_segment(), which erroneously ignored a segment's has_compactible_offsets() result if it had removable tombstones when considering its eligibility for self compaction.

https://github.com/redpanda-data/redpanda/compare/3ac5b645768b4fc74ede70977e5f7b64d744a49c..262600310ec7dde3f8735f9055a33e2a8280a390#diff-c79a67494746e1dac357cbfc721f6718275cc322e24c34dfc8f8f6cdcae546d9R742-R743

WillemKauf requested review from dotnwat and andrwng September 19, 2024 02:20

github-actions bot added the area/redpanda label Sep 19, 2024

WillemKauf force-pushed the compaction_enhancements branch from e3fc594 to 2b3857e Compare September 19, 2024 02:25

WillemKauf commented Sep 19, 2024

View reviewed changes

andrwng reviewed Sep 19, 2024

View reviewed changes

WillemKauf added 2 commits September 19, 2024 16:12

WillemKauf force-pushed the compaction_enhancements branch from 2b3857e to 7249b56 Compare September 20, 2024 00:29

WillemKauf commented Sep 20, 2024

View reviewed changes

WillemKauf changed the title ~~storage: compaction enhancements~~ storage: compaction enhancements Sep 20, 2024

WillemKauf force-pushed the compaction_enhancements branch from 7249b56 to 3ac5b64 Compare September 20, 2024 03:52

WillemKauf requested a review from andrwng September 20, 2024 16:35

andrwng previously approved these changes Sep 23, 2024

View reviewed changes

WillemKauf added 2 commits September 23, 2024 23:51

WillemKauf dismissed andrwng’s stale review via 2626003 September 24, 2024 03:53

WillemKauf force-pushed the compaction_enhancements branch from 3ac5b64 to 2626003 Compare September 24, 2024 03:53

WillemKauf requested a review from andrwng September 24, 2024 16:58

andrwng approved these changes Sep 24, 2024

View reviewed changes

WillemKauf merged commit d57dc7c into redpanda-data:dev Sep 24, 2024
17 checks passed

WillemKauf mentioned this pull request Oct 7, 2024

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

Merged

7 tasks

WillemKauf mentioned this pull request Nov 26, 2024

storage: reset compaction window start offset when offset map isn't built #24323

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`storage`: compaction enhancements #23380

`storage`: compaction enhancements #23380

WillemKauf commented Sep 19, 2024 •

edited

Loading

WillemKauf commented Sep 19, 2024

vbotbuildovich commented Sep 19, 2024 •

edited

Loading

WillemKauf Sep 19, 2024

WillemKauf Sep 19, 2024

WillemKauf Sep 19, 2024

andrwng Sep 19, 2024

WillemKauf Sep 20, 2024

andrwng Sep 19, 2024

andrwng Sep 19, 2024

WillemKauf Sep 20, 2024 •

edited

Loading

andrwng Sep 19, 2024

WillemKauf Sep 20, 2024

WillemKauf Sep 20, 2024 •

edited

Loading

WillemKauf commented Sep 24, 2024 •

edited

Loading

storage: compaction enhancements #23380

storage: compaction enhancements #23380

Conversation

WillemKauf commented Sep 19, 2024 • edited Loading

PR Summary

Minor tombstone additions/tweaks:

A number of optimizations to compaction, both in general and for tombstones:

Backports Required

Release Notes

Improvements

WillemKauf commented Sep 19, 2024

vbotbuildovich commented Sep 19, 2024 • edited Loading

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

WillemKauf Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillemKauf Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

WillemKauf commented Sep 24, 2024 • edited Loading

`storage`: compaction enhancements #23380

`storage`: compaction enhancements #23380

WillemKauf commented Sep 19, 2024 •

edited

Loading

vbotbuildovich commented Sep 19, 2024 •

edited

Loading

WillemKauf Sep 20, 2024 •

edited

Loading

WillemKauf Sep 20, 2024 •

edited

Loading

WillemKauf commented Sep 24, 2024 •

edited

Loading