Check conflict at output level in CompactFiles #3926
Conversation
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
db/compaction_picker.cc
Outdated
| @@ -852,6 +852,11 @@ Status CompactionPicker::SanitizeCompactionInputFilesForAllLevels( | |||
| } | |||
| } | |||
| } | |||
| if (RangeOverlapWithCompaction(smallestkey, largestkey, output_level)) { | |||
There was a problem hiding this comment.
oops this is inside the loop, it should be able to go outside
|
@ajkr has updated the pull request. View: changes, changes since last import |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
ping |
| @@ -853,6 +853,11 @@ Status CompactionPicker::SanitizeCompactionInputFilesForAllLevels( | |||
| } | |||
| } | |||
| } | |||
| if (RangeOverlapWithCompaction(smallestkey, largestkey, output_level)) { | |||
There was a problem hiding this comment.
How is it possible that there is no overlap of input files with running compactions, but there is overlap at output level?
There was a problem hiding this comment.
It's a weird edge case, where the input files' key range is in a gap between files that are pending compaction to the same output level. The pending compaction merges the input files and can produce output files that cover the gap.
There was a problem hiding this comment.
Thanks for the clarification. In this edge case, we might end up with files in the output with overlapping key ranges. So checking for range overlap with pending compactions would detect that and not start the manual compaction.
| @@ -853,6 +853,11 @@ Status CompactionPicker::SanitizeCompactionInputFilesForAllLevels( | |||
| } | |||
| } | |||
| } | |||
| if (RangeOverlapWithCompaction(smallestkey, largestkey, output_level)) { | |||
There was a problem hiding this comment.
Thanks for the clarification. In this edge case, we might end up with files in the output with overlapping key ranges. So checking for range overlap with pending compactions would detect that and not start the manual compaction.
There was a problem hiding this comment.
@ajkr is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
* fix CompactFiles inclusion of older L0 files Summary: if we're moving any L0 files down, we need to include older L0 files since they may contain older versions of the keys being moved down. Closes facebook#2845 Differential Revision: D5773800 Pulled By: ajkr fbshipit-source-id: 9f0770a8eaaeea4c87df2e7a2a1d65bf9d7f4f7e * fix hanging after CompactFiles with L0 overlap Summary: Bug report: https://www.facebook.com/groups/rocksdb.dev/permalink/1389452781153232/ Non-empty `level0_compactions_in_progress_` was aborting `CompactFiles` after incrementing `bg_compaction_scheduled_`, and in that case we never decremented it. This blocked future compactions and prevented DB close as we wait for scheduled compactions to finish/abort during close. I eliminated `CompactFiles`'s dependency on `level0_compactions_in_progress_`. Since it takes a contiguous span of L0 files -- through the last L0 file if any L1+ files are included -- it's fine to run in parallel with other compactions involving L0. We make the same assumption in intra-L0 compaction. Closes facebook#2849 Differential Revision: D5780440 Pulled By: ajkr fbshipit-source-id: 15b15d3faf5a699aed4b82a58352d4a7bb23e027 * Check conflict at output level in CompactFiles (facebook#3926) Summary: CompactFiles checked whether the existing files conflicted with the chosen compaction. But it missed checking whether future files would conflict, i.e., when another compaction was simultaneously writing new files to the same range at the same output level. Closes facebook#3926 Differential Revision: D8218996 Pulled By: ajkr fbshipit-source-id: 21cb00a6fed4c8c62d3ed2ff810962e6bdc2fdfb
RocksDB was violating an invariant that no 2 sstables in a level overlap. It isn't quite clear what the upshot of this violation is. At the very least it would cause the overlapping tables to be compacted together. It seems possible that it could lead to missing writes, but I haven't been able to verify that. Fixes cockroachdb#26693 Release note: None
RocksDB was violating an invariant that no 2 sstables in a level overlap. It isn't quite clear what the upshot of this violation is. At the very least it would cause the overlapping tables to be compacted together. It seems possible that it could lead to missing writes, but I haven't been able to verify that. Fixes cockroachdb#26693 Release note: None
26755: release-2.0: Bump RocksDB pointer to grab facebook/rocksdb#3926 r=benesch,a-robinson a=petermattis RocksDB was violating an invariant that no 2 sstables in a level overlap. It isn't quite clear what the upshot of this violation is. At the very least it would cause the overlapping tables to be compacted together. It seems possible that it could lead to missing writes, but I haven't been able to verify that. Fixes #26693 Release note: None Co-authored-by: Peter Mattis <[email protected]>
26753: storage: Add extra event to allocator rebalancing r=a-robinson a=a-robinson Helps make the output of simulated allocator runs less confusing, since otherwise it's not clear why we're considering removal from the range and why the replicas being considered for removal includes one that isn't even a real member of the range. Release note: None Would have made looking at the simulated allocator output from https://forum.cockroachlabs.com/t/how-to-enable-leaseholder-load-balancing/1732/3 a little more pleasant. 26754: Bump RocksDB pointer to grab facebook/rocksdb#3926 r=benesch,a-robinson a=petermattis RocksDB was violating an invariant that no 2 sstables in a level overlap. It isn't quite clear what the upshot of this violation is. At the very least it would cause the overlapping tables to be compacted together. It seems possible that it could lead to missing writes, but I haven't been able to verify that. Fixes #26693 Release note: None Co-authored-by: Alex Robinson <[email protected]> Co-authored-by: Peter Mattis <[email protected]>
CompactFiles checked whether the existing files conflicted with the chosen compaction. But it missed checking whether future files would conflict, i.e., when another compaction was simultaneously writing new files to the same range at the same output level.
Test Plan: