From be8b021ea46012555a0cc9754d664f085d3fb448 Mon Sep 17 00:00:00 2001 From: Willem Kaufmann Date: Thu, 5 Dec 2024 19:57:35 -0500 Subject: [PATCH] `storage`: check for `may_have_tombstones` in `self_compact_segment` Segments that may have tombstone records in them should still be considered eligible for self compaction. An early return statement was missing a check for this condition. Add it so that tombstones will be properly removed for a segment eligible for removal. Also adjusts a `vassert()` to account for this case. (cherry picked from commit 1f5f3633ee14522c0962daf18500a972228c2f60) --- src/v/storage/segment_utils.cc | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/v/storage/segment_utils.cc b/src/v/storage/segment_utils.cc index 3175146726ca6..2e6355bbce979 100644 --- a/src/v/storage/segment_utils.cc +++ b/src/v/storage/segment_utils.cc @@ -724,9 +724,10 @@ ss::future self_compact_segment( "Cannot compact an active segment. cfg:{} - segment:{}", cfg, s)); } + const bool may_remove_tombstones = may_have_removable_tombstones(s, cfg); if ( !s->has_compactible_offsets(cfg) - || (s->finished_self_compaction() && !may_have_removable_tombstones(s, cfg))) { + || (s->finished_self_compaction() && !may_remove_tombstones)) { co_return compaction_result{s->size_bytes()}; } @@ -735,7 +736,11 @@ ss::future self_compact_segment( = co_await maybe_rebuild_compaction_index( s, stm_manager, cfg, read_holder, resources, pb); - if (state == compacted_index::recovery_state::already_compacted) { + const bool segment_already_compacted + = (state == compacted_index::recovery_state::already_compacted) + && !may_remove_tombstones; + + if (segment_already_compacted) { vlog( gclog.debug, "detected {} is already compacted", @@ -744,10 +749,10 @@ ss::future self_compact_segment( co_return compaction_result{s->size_bytes()}; } - vassert( - state == compacted_index::recovery_state::index_recovered, - "Unexpected state {}", - state); + const bool is_valid_index_state + = (state == compacted_index::recovery_state::index_recovered) + || (state == compacted_index::recovery_state::already_compacted); + vassert(is_valid_index_state, "Unexpected state {}", state); auto sz_before = s->size_bytes(); auto apply_offset = should_apply_delta_time_offset(feature_table);