Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: add tombstones_removed metric to probe #24145

Merged
merged 1 commit into from
Nov 19, 2024
Merged

storage: add tombstones_removed metric to probe #24145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/v/storage/probe.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,12 @@ void probe::setup_metrics(const model::ntp& ntp) {
[this] { return _compaction_removed_bytes; },
sm::description("Number of bytes removed by a compaction operation"),
labels),
sm::make_counter(
"tombstones_removed",
[this] { return _tombstones_removed; },
sm::description("Number of tombstone records removed by compaction "
"due to the delete.retention.ms setting."),
labels),
},
{},
{sm::shard_label, partition_label});
Expand Down
3 changes: 3 additions & 0 deletions src/v/storage/probe.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ class probe {
_cached_batches_read += batches;
}

void add_removed_tombstone() { ++_tombstones_removed; }

void batch_parse_error() { ++_batch_parse_errors; }

void setup_metrics(const model::ntp&);
Expand Down Expand Up @@ -136,6 +138,7 @@ class probe {
uint32_t _batch_parse_errors = 0;
uint32_t _batch_write_errors = 0;
double _compaction_ratio = 1.0;
uint64_t _tombstones_removed = 0;

ssize_t _compaction_removed_bytes = 0;

Expand Down
2 changes: 2 additions & 0 deletions src/v/storage/segment_deduplication_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ ss::future<index_state> deduplicate_segment(
auto copy_reducer = internal::copy_data_segment_reducer(
[&map,
&may_have_tombstone_records,
&probe,
segment_last_offset = seg->offsets().get_committed_offset(),
past_tombstone_delete_horizon,
compaction_placeholder_enabled](
Expand All @@ -213,6 +214,7 @@ ss::future<index_state> deduplicate_segment(

// Deal with tombstone record removal
if (r.is_tombstone() && past_tombstone_delete_horizon) {
probe.add_removed_tombstone();
return ss::make_ready_future<bool>(false);
}

Expand Down
35 changes: 18 additions & 17 deletions src/v/storage/segment_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,26 +402,27 @@ ss::future<storage::index_state> do_copy_segment_data(
seg->reader().filename(),
tmpname);
bool may_have_tombstone_records = false;
auto should_keep = [compacted_list = std::move(compacted_offsets),
past_tombstone_delete_horizon,
&may_have_tombstone_records](
const model::record_batch& b,
const model::record& r,
bool) {
// Deal with tombstone record removal
if (r.is_tombstone() && past_tombstone_delete_horizon) {
return ss::make_ready_future<bool>(false);
}
auto should_keep =
[compacted_list = std::move(compacted_offsets),
past_tombstone_delete_horizon,
&may_have_tombstone_records,
&pb](const model::record_batch& b, const model::record& r, bool) {
// Deal with tombstone record removal
if (r.is_tombstone() && past_tombstone_delete_horizon) {
pb.add_removed_tombstone();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not sure how likely it is to be problematic, but something to consider is maybe we should only update this if the compaction succeeds

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline, we will live with the overcounting for now in the edge cases of compaction failing past this point (partition shutdown/movement)

return ss::make_ready_future<bool>(false);
}

const auto o = b.base_offset() + model::offset_delta(r.offset_delta());
const auto keep = compacted_list.contains(o);
const auto o = b.base_offset()
+ model::offset_delta(r.offset_delta());
const auto keep = compacted_list.contains(o);

if (r.is_tombstone() && keep) {
may_have_tombstone_records = true;
}
if (r.is_tombstone() && keep) {
may_have_tombstone_records = true;
}

return ss::make_ready_future<bool>(keep);
};
return ss::make_ready_future<bool>(keep);
};

model::offset segment_last_offset{};
if (likely(feature_table.local().is_active(
Expand Down
Loading