-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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 delete triggered compaction for single level universal #7224
Conversation
} | ||
if (compact) { | ||
FileMetaData* f = vstorage_->LevelFiles(0)[loop]; |
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 am confused by this code. Looking at how sorted_runs_ is built, it appears that the value for loop could be greater than the depth of LevelFiles[0]. Won't this run off the end of the LevelFiles(0)[] array?
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.
This code is only executed in case of vstorage_->num_levels() == 1
. In that case, sorted_runs_
has one entry for each entry in vstorage.LevelFiles(0)
, and nothing else since all other levels must be empty.
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. Should we also (in a later PR) convert the assertion in MarkFilesBeingCompacted()
to a runtime check for release builds?
} | ||
if (compact) { | ||
FileMetaData* f = vstorage_->LevelFiles(0)[loop]; |
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.
This code is only executed in case of vstorage_->num_levels() == 1
. In that case, sorted_runs_
has one entry for each entry in vstorage.LevelFiles(0)
, and nothing else since all other levels must be empty.
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.
@anand1976 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@anand1976 merged this pull request in f308da5. |
…7224) Summary: Delete triggered compaction (DTC) for universal compaction style with ```num_levels = 1``` has been disabled for sometime due to a data correctness bug. This PR re-enables it with a bug fix. A file marked for compaction can be picked, along with all L0 files after it as the compaction input. We stop adding files to the input once we encounter a file already being compacted (the original bug failed to check the compaction status of the files). Tests: Add unit tests to ```compaction_picker_test.cc``` Pull Request resolved: facebook#7224 Reviewed By: ajkr Differential Revision: D23031845 Pulled By: anand1976 fbshipit-source-id: 9de3cab5f9774cede666c2c48d309a7d9b88a505
Delete triggered compaction (DTC) for universal compaction style with
num_levels = 1
has been disabled for sometime due to a data correctness bug. This PR re-enables it with a bug fix. A file marked for compaction can be picked, along with all L0 files after it as the compaction input. We stop adding files to the input once we encounter a file already being compacted (the original bug failed to check the compaction status of the files).Tests:
Add unit tests to
compaction_picker_test.cc