Cancel tombstone skipping during bottommost compaction #7356
Conversation
There was a problem hiding this comment.
Thanks for the fix @riversand963 ! LGTM with a couple of questions/minor comments.
db/compaction/compaction_iterator.cc
Outdated
| @@ -565,11 +563,16 @@ void CompactionIterator::NextFromInput() { | |||
| // Handle the case where we have a delete key at the bottom most level | |||
| // We can skip outputting the key iff there are no subsequent puts for this | |||
| // key | |||
| assert(compaction_->KeyNotExistsBeyondOutputLevel(ikey_.user_key, | |||
There was a problem hiding this comment.
We should probably also assert(compaction_); here.
There was a problem hiding this comment.
I ended up doing assert(!compaction_ || compaction_->KeyNot.....).
db/db_compaction_test.cc
Outdated
|
|
||
| for (int i = 0; i < kNumL0Files / 4; ++i) { | ||
| ASSERT_OK(Put("a", "value")); | ||
| ASSERT_OK(Put("c", "value")); |
There was a problem hiding this comment.
Based on i < kNumL0Files / 4, I'm assuming the loop body is supposed to generate four files, is that correct? Maybe a Flush call is missing from here?
db/db_compaction_test.cc
Outdated
|
|
||
| CompactRangeOptions cro; | ||
| Status s = db_->CompactRange(cro, nullptr, nullptr); | ||
| ASSERT_TRUE(s.IsShutdownInProgress()); |
There was a problem hiding this comment.
I'm wondering if we can somehow make sure that in this test that the new logic kicked in? (I believe you would get the same behavior with the old code as well once we'd churned through the tombstones.) Maybe instead of changing the condition of the while above, we could put a check with a sync point in the while loop's body, e.g. if (IsShuttingDown()) { TEST_SYNC_POINT(...); break; }?
riversand963
force-pushed
the
cancel-compact-skip-bottom-tombstones
branch
from
a36b377 to
c15c029
Compare
|
Oh, thanks @ltamasi for the review....I just deleted the unit test because I think it was testing something not very interesting at the cost of introducing additional test sync points which make it more tedious for future code-refactoring... |
|
Update HISTORY.md? |
riversand963
force-pushed
the
cancel-compact-skip-bottom-tombstones
branch
from
c15c029 to
fec0773
Compare
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 merged this pull request in 205e577. |
Summary: During bottommost compaction, RocksDB cannot simply drop a tombstone if this tombstone is not in the earliest snapshot. The current behavior is: RocksDB skips other internal keys (of the same user key) in the same snapshot range. In the meantime, RocksDB should check for the `shutting_down` flag. Otherwise, it is possible for a bottommost compaction that has already started running to take a long time to finish, even if the application has tried to cancel all background jobs. Pull Request resolved: facebook#7356 Test Plan: make check Reviewed By: ltamasi Differential Revision: D23663241 Pulled By: riversand963 fbshipit-source-id: 25f8e9b51bc3bfa3353cdf87557800f9d90ee0b5
During bottommost compaction, RocksDB cannot simply drop a tombstone if
this tombstone is not in the earliest snapshot. The current behavior is: RocksDB
skips other internal keys (of the same user key) in the same snapshot range. In
the meantime, RocksDB should check for the
shutting_downflag. Otherwise, itis possible for a bottommost compaction that has already started running to take
a long time to finish, even if the application has tried to cancel all background jobs.
Test plan:
make check