Skip to content

Commit

Permalink
storage: check for may_have_tombstones in self_compact_segment
Browse files Browse the repository at this point in the history
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 1f5f363)
  • Loading branch information
WillemKauf committed Dec 9, 2024
1 parent 033afa3 commit be8b021
Showing 1 changed file with 11 additions and 6 deletions.
17 changes: 11 additions & 6 deletions src/v/storage/segment_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -724,9 +724,10 @@ ss::future<compaction_result> 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()};
}

Expand All @@ -735,7 +736,11 @@ ss::future<compaction_result> 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",
Expand All @@ -744,10 +749,10 @@ ss::future<compaction_result> 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);
Expand Down

0 comments on commit be8b021

Please sign in to comment.