Fix crash in CompactFiles() of conflict range under preclude_last_level_data_seconds > 0
#12628
Conversation
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
hx235
force-pushed
the
compact_file_key_placement_conflict
branch
from
83192e4
to
8a7a288
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
hx235
changed the title
preclude_last_level_data_seconds > 0preclude_last_level_data_seconds > 0
hx235
force-pushed
the
compact_file_key_placement_conflict
branch
from
8a7a288
to
c38b294
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
hx235
force-pushed
the
compact_file_key_placement_conflict
branch
from
c38b294
to
133e8a3
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
hx235
force-pushed
the
compact_file_key_placement_conflict
branch
from
133e8a3
to
e722b2b
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Potential conflict with #12633 (comment) |
db/compaction/compaction_picker.h
Outdated
| // `converted_input_files`. | ||
| // When the input parameters do not describe a valid | ||
| // compaction, the function will try to fix the input_files by adding | ||
| // necessary files. If it's not possible to conver an invalid input_files |
| // To force below two CompactFiles() in order to coerce range conflict on L1 | ||
| // upon (2) | ||
| // (1): Compact [k2] at L0 and [k3] at L1 and output to L1 and L0 | ||
| // (2): Compact [k4] at L1 and [k1, k5] at L2 and output to L1 and L2 |
There was a problem hiding this comment.
In this case, it does not seem like an actual conflict: universal with preclude feature should only output to the penultimate level the keys that are within the key range of input files from penultimate level:
rocksdb/db/compaction/compaction_iterator.cc
Lines 1305 to 1306 in 9bddac0
However, I think this change is fine since this seems to be an existing behavior.
db/db_compaction_test.cc
Outdated
|
|
||
| // To force below two CompactFiles() in order to coerce range conflict on L1 | ||
| // upon (2) | ||
| // (1): Compact [k2] at L0 and [k3] at L1 and output to L1 and L0 |
hx235
force-pushed
the
compact_file_key_placement_conflict
branch
from
e722b2b
to
a70079f
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Context/Summary:
Previously
CompactFiles()usedRangeOverlapWithCompaction()to check for conflict when sanitizing input files while later usedFilesRangeOverlapWithCompaction()to assert for no conflict. The latter function checks for more conflict scenarios than the former does, particularly the ones arising frompreclude_last_level_data_seconds > 0(i.e, compaction can output to second-to-the-last level). So we ran into assertion violation inCompactFiles()like belowThis PR make
CompactFiles()usedFilesRangeOverlapWithCompaction()and return Aborted status upon range conflict instead of crashing (during debug build) or proceed incorrectly (during non-debug build). To do so cleanly, I included a refactoring to makeFilesRangeOverlapWithCompaction()part ofSanitizeAndConvertCompactionInputFiles(), replacingRangeOverlapWithCompaction().Test:
New UT crashed before the fix and return correct status after the fix.