Skip to content
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

storage/engine: fix DeleteRange+ApplyBatchRepr #14416

Conversation

petermattis
Copy link
Collaborator

@petermattis petermattis commented Mar 28, 2017

Added support for DeleteRange to DBBatchInserter. This missing support
was causing test failures and various data corruption when enabling
COCKROACH_ENABLE_FAST_CLEAR_RANGE after #14138 started using
ApplyBatchRepr on batches containing DeleteRange operations.

See #14391


This change is Reviewable

@tamird
Copy link
Contributor

tamird commented Mar 28, 2017

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


pkg/storage/engine/db.cc, line 513 at r1 (raw file):

  }
  // NB: we don't support DeleteRangeCF yet which causes us to pick up
  // the default implementation that returns an error.

if the default implementation returns an error, how come we saw data corruption?


Comments from Reviewable

Added support for DeleteRange to DBBatchInserter. This missing support
was causing test failures and various data corruption when enabling
COCKROACH_ENABLE_FAST_CLEAR_RANGE after cockroachdb#14138 started using
ApplyBatchRepr on batches containing DeleteRange operations.

See cockroachdb#14391
@petermattis petermattis force-pushed the pmattis/delete-range-apply-batch-repr branch from 6b62283 to 0ffaf1a Compare March 28, 2017 14:37
@petermattis
Copy link
Collaborator Author

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


pkg/storage/engine/db.cc, line 513 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

if the default implementation returns an error, how come we saw data corruption?

I was ignoring an error. Now fixed.

PS Data corruption == batch was only partially applied.


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Mar 28, 2017

:lgtm:


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@petermattis petermattis merged commit c052e6b into cockroachdb:master Mar 28, 2017
@petermattis petermattis deleted the pmattis/delete-range-apply-batch-repr branch March 28, 2017 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants