-
Notifications
You must be signed in to change notification settings - Fork 705
storage: add _num_adjacent_segments_compacted to probe
#26180
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.
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
Conversation
src/v/storage/disk_log_impl.cc
Outdated
| segment_to_remove, "compact_adjacent_segments"); | ||
| } | ||
|
|
||
| _probe->segments_adjacent_merged(segments.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wondering if this method of counting is the most intuitive. for example, if we have 4 segments reduced to 1 then we'd have 2 + 2 + 2 = 6? Which I suppose is literally correct, but it also feels like it over counts? not really sure...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't have a strong opinion on this when adding the metric- you can either halve the metric to know that 3 segments have been removed by adjacent compaction, or acknowledge that 6 segments have, in total, partaken in adjacent compaction. If we extend this way of counting to the implementation for adjacent segment compaction beyond two segments, the multiplication factor will always be < 2.
If you have a strong preference here, I'm happy to adjust the counting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My 2 cents is I think a metric for the number of segments compacted away (half this metric) is more intuitive and communicates the same information-- how many adjacent segment compaction operations have happened. Given the current implementation of adjacent segment compaction any of the following immediately tells you the others: number of adjacent segment compactions, number of segments that have participated in adjacent segment compaction, number of segments compacted away by adjacent segment compaction, number of segments produced by adjacent segment compaction. All four of those are interesting and wouldn't be directly related to each other if we generalize adjacent segment compaction to be m:n.
Also, is it worth having a bytes measurement too? That way we have compaction metrics reflecting "fixed costs" like making new files and shuffling metadata, and metrics reflecting "variable costs" like how many bytes we have to write, how many bytes we compacted away, how much reindexing has to happen, and how much savings there is to readers who read compacted data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Altered.
CI test resultstest results on build#66111
test results on build#66205
|
0ec8e07 to
2b2d6ba
Compare
storage: add _num_adjacent_segments_merged to probestorage: add _num_adjacent_segments_compacted to probe
To improve observability of adjacent segment compaction.
2b2d6ba to
05f5ef7
Compare
|
/ci-repeat 1 |
|
/backport v25.1.x |
|
/backport v24.3.x |
|
Failed to create a backport PR to v24.3.x branch. I tried: |
To improve observability of adjacent segment compaction. This counter represents the total number of segments that have been compacted away via adjacent segment compaction.
Backports Required
Release Notes
Improvements
storage_log_adjacent_segments_compactedmetric for better observability into adjacent segment compaction.