Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upccl/storageccl/engineccl: properly handle intents which straddle sstables #31290
Conversation
petermattis
requested a review
from cockroachdb/sql-bulk-prs
as a
code owner
Oct 11, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
This change is |
petermattis
requested review from
benesch and
danhhz
Oct 11, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
petermattis
Oct 11, 2018
Contributor
See #28358. I think this reproduces the scenario described in that bug. No comments yet, but the code is relatively straightforward. Getting this out for review as I'm going to be offline for the rest of the night.
|
See #28358. I think this reproduces the scenario described in that bug. No comments yet, but the code is relatively straightforward. Getting this out for review as I'm going to be offline for the rest of the night. |
petermattis
changed the title from
[WIP]
to
ccl/storageccl/engineccl: add TestMVCCIncrementalIteratorIntentStraddlesSStables
Oct 12, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
petermattis
Oct 12, 2018
Contributor
Added some comments to the tests and made it detect the success vs failure condition. The test currently fails (as expected), and passes if I disable TimeBoundTblPropCollector::AddUserKey so that time bounds are not added to any sstable. I'll get to the fix after breakfast.
|
Added some comments to the tests and made it detect the success vs failure condition. The test currently fails (as expected), and passes if I disable |
petermattis
requested a review
from cockroachdb/core-prs
as a
code owner
Oct 12, 2018
petermattis
changed the title from
ccl/storageccl/engineccl: add TestMVCCIncrementalIteratorIntentStraddlesSStables
to
ccl/storageccl/engineccl: properly handle intents which straddle sstables
Oct 12, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
petermattis
Oct 12, 2018
Contributor
Implemented the suggested fix to TimeBoundTblPropCollector and the test now passes. I think this PR is good to go. PTAL.
|
Implemented the suggested fix to |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
bors r=danhhz |
petermattis
referenced this pull request
Oct 12, 2018
Merged
release-2.1: ccl/storageccl/engineccl: properly handle intents which straddle sstables #31316
bot
pushed a commit
that referenced
this pull request
Oct 12, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
craig
bot
commented
Oct 12, 2018
Build succeeded |
petermattis commentedOct 11, 2018
•
edited
An intent which straddles an sstable can lead an incremental iterator to
incorrectly ignore an sstable. In order to fix this, when an intent
straddles an sstable (i.e. the metadata key is the last key in the
sstable) we need to include the intent's timestamp in the timestamp
bounds. We don't need to do this for interior intents because we'll
already be including the intent's timestamp as it is contained in the
next key following the intent. Add
TestMVCCIncrementalIteratorIntentStraddlesSStableswhich demonstratesthe problem.
Fixes #28358
Release note (bug fix): Fix a rare scenario where a backup could
incorrectly include a key for a transaction which was aborted.