[v23.2.x] compaction: do not adjacent compact non self compacted segements #15372
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
and bumps the log level to info.
Adjacent compaction works by concatenating compaction indexes of adjacent segments. When a segment has transactional batches, we prematurely close the compaction index during segment append because we do not know upfront whether a batch will be committed/aborted. We recompute the index at the time of self compaction as it guarantees all the batches are either committed/aborted. Adjacent compacting a non self compacted index can result in incomplete concatenated index, this fix avoids it.
bharathv
commented
Dec 8, 2023
| @@ -527,6 +533,22 @@ ss::future<compaction_result> disk_log_impl::compact_adjacent_segments( | |||
| std::vector<ss::lw_shared_ptr<segment>> segments; | |||
| std::copy(range.first, range.second, std::back_inserter(segments)); | |||
|
|
|||
| bool all_segments_self_compacted = std::all_of( | |||
There was a problem hiding this comment.
Note for reviewers, we already check for this condition in do_compact()
// loop until we compact segment or reached end of segments set
while (!_segs.empty()) {
const auto end_it = std::prev(_segs.end());
auto seg_it = std::find_if(
_segs.begin(),
end_it,
[offsets_compactible](ss::lw_shared_ptr<segment>& s) {
return !s->has_appender() && s->is_compacted_segment()
&& !s->finished_self_compaction(). <===
&& offsets_compactible(*s);
});
// nothing to compact
if (seg_it == end_it) {
break;
}
but that is not tight enough as segments may be appended to _segs after the loop breaks and non self compacted segment could be appended to _segs which is then considered for adjacent segment compaction. This is the reason why the bug was exposed in when rolling in a tight loop along with compaction tests.
bharathv
force-pushed
the
compaction_fix_232x
branch
from
c225812
to
d07e9f9
Compare
bharathv
changed the title
andrwng
reviewed
Dec 8, 2023
| const auto unstable = std::any_of( | ||
| range.first, range.second, [&cfg](ss::lw_shared_ptr<segment>& seg) { | ||
| return seg->has_appender() || !seg->has_compactible_offsets(cfg); | ||
| return !seg->finished_self_compaction() || seg->has_appender() |
Comment on lines
59
to
61
| config::shard_local_cfg().log_disable_housekeeping_for_tests.set_value( | ||
| true); | ||
| config::shard_local_cfg().log_segment_ms_min.set_value(1ms); |
There was a problem hiding this comment.
You should either reset these manually at the end of the test or use a scoped_config to set these, otherwise they'll bleed into other tests
There was a problem hiding this comment.
oo, good point, fixed
rolling and running house keeping in a tight loop provides additional test coverage for more interesting compaction test cases.
bharathv
force-pushed
the
compaction_fix_232x
branch
from
d07e9f9
to
bfc9574
Compare
|
unrelated failure: |
|
@bharathv did you discover if this exists in upstream or not? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adjacent compaction works by concatenating compaction indexes of
adjacent segments. When a segment has transactional batches, we
prematurely close the compaction index during segment append because
we do not know upfront whether a batch will be committed/aborted. We
recompute the index at the time of self compaction as it guarantees all
the batches are either committed/aborted.
Adjacent compacting a non self compacted index can result in incomplete
concatenated index, this fix avoids it.
This issue was exposed by running house keeping in a tight loop with low segment.ms but
has a very low probability of occurrence in practice.
Backports Required
Release Notes
Bug Fixes