Allow DB reopen with reduced options.num_levels #2740
Conversation
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
db/version_builder.cc
Outdated
| return false; | ||
| } | ||
| // Don't need to check deleted files. | ||
| level_iter = levels_.erase(level_iter); |
There was a problem hiding this comment.
I'm not a big fan of deleting entries from a data structure while iterating it, no matter it is supported or not, because it creates a portable issue if someone tries to swap a data structure. I suggest we do it in two paths, or not removing these empty levels at all.
|
@siying tested with |
|
@yiwu-arbug updated the pull request - view changes - changes since last import |
|
Is 27MB the largest you can find? I think there must be much larger manifest files. |
|
@yiwu-arbug updated the pull request - view changes - changes since last import |
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Updated the code to use a separate map for invalid levels, to avoid regression. Tested with the same ldb command to dump a 39MB manifest (which is the largest among several prod servers I checked). |
|
ping |
|
ping @siying |
db/version_builder.cc
Outdated
| invalid_levels_[level].insert(number); | ||
| } else { | ||
| has_invalid_levels_ = true; | ||
| } |
There was a problem hiding this comment.
What's the purpose of complicating the logic to have invalid_levels_ and has_invalid_levels_ ? Can we have the verification logic all in the end?
There was a problem hiding this comment.
invalid_levels_ is split from levels_ to avoid regression in case there isn't files above num_levels_. has_invalid_levels_ can be avoid, but it is handy to remember whether there are erase of non-existing files or addition of existing file, on invalid levels. Let me add more comments.
|
@yiwu-arbug updated the pull request - view changes - changes since last import |
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
db/version_builder.cc
Outdated
| invalid_levels_[level].insert(number); | ||
| } else { | ||
| has_invalid_levels_ = true; | ||
| } |
|
@yiwu-arbug updated the pull request - view changes - changes since last import |
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@yiwu-arbug updated the pull request - view changes - changes since last import |
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary:
Allow user to reduce number of levels in LSM by issue a full CompactRange() and put the result in a lower level, and then reopen DB with reduced options.num_levels. Previous this will fail on reopen on when recovery replaying the previous MANIFEST and found a historical file was on a higher level than the new options.num_levels. The workaround was after CompactRange(), reopen the DB with old num_levels, which will create a new MANIFEST, and then reopen the DB again with new num_levels.
This patch relax the check of levels during recovery. It allows DB to open if there was a historical file on level > options.num_levels, but was also deleted.
Test Plan:
See the new test. Also make sure existing tests pass.