Compaction with timestamp: input boundaries #6645
Conversation
riversand963
changed the title
riversand963
force-pushed
the
ts-compatible-compaction
branch
from
c605b86
to
c838bcc
Compare
riversand963
changed the title
riversand963
changed the title
riversand963
changed the title
riversand963
changed the title
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
force-pushed
the
ts-compatible-compaction
branch
from
c838bcc
to
924c370
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.
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 has updated the pull request. Re-import the pull request |
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.
|
cc @hliu18 |
riversand963
force-pushed
the
ts-compatible-compaction
branch
from
35562fc
to
56be440
Compare
|
@riversand963 has updated the pull request. Re-import the pull request |
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.
There was a problem hiding this comment.
Thanks for the patch @riversand963 ! Looks good, just some minor comments (please see below). Also, one general question: I suppose we'll need changes in other places like e.g. CompactionPicker, CompactionIterator, FileIndexer etc.; will those be handled in a separate PR?
| } | ||
| ASSERT_OK(Flush()); | ||
| // Wait for compaction to finish | ||
| ASSERT_OK(dbfull()->TEST_WaitForCompact()); |
There was a problem hiding this comment.
So we're relying on automatic compactions here, right? I'm wondering if it would be better if we called CompactRange with for the range consisting only of key 99 here.
There was a problem hiding this comment.
I think they will go through the same code path in LevelCompactionBuilder, thus should be the same.
Summary: During compaction, timestamp should also be taken into accound when computing overlapping ranges. If not, point lookup and range scan will return incorrect results. Test Plan: make check
riversand963
force-pushed
the
ts-compatible-compaction
branch
from
56be440
to
7b0155e
Compare
|
@riversand963 has updated the pull request. Re-import the pull request |
Thanks @ltamasi for the detailed review. |
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 0c05624. |
Summary: The subcompaction boundary picking logic does not currently guarantee that all user keys that differ only by timestamp get processed by the same subcompaction. This can cause issues with the `CompactionIterator` state machine: for instance, one subcompaction that processes a subset of such KVs might drop a tombstone based on the KVs it sees, while in reality the tombstone might not have been eligible to be optimized out. (See also #6645, which adjusted the way compaction inputs are picked for the same reason.) Pull Request resolved: #8393 Test Plan: Ran `make check` and the crash test script with timestamps enabled. Reviewed By: jay-zhuang Differential Revision: D29071635 Pulled By: ltamasi fbshipit-source-id: f6c72442122b4e581871e096fabe3876a9e8a5a6
Towards making compaction logic compatible with user timestamp.
When computing boundaries and overlapping ranges for inputs of compaction, We need to compare SSTs by user key without timestamp.
Test plan (devserver):
Several individual tests: