-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
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. |
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. |
preclude_last_level_data_seconds > 0
preclude_last_level_data_seconds > 0
8a7a288
to
c38b294
Compare
@hx235 has updated the pull request. You must reimport the pull request before landing. |
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. |
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'm not a compaction expert so I don't want to review changes to compaction preconditions.
I did confirm this is an old bug, with your unit test failing in 8.8.fb, the earliest I tried.
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) |
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.
LGTM
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 |
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.
convert?
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.
Fixed
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
bool safe_to_penultimate_level = | |
compaction_->WithinPenultimateLevelOutputRange(ikey_for_range_check); |
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 |
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.
Output to L1?
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.
Fixed
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.