Skip to content

batcheval: add range tombstone support for DeleteRange#77762

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
erikgrinaker:mvcc-range-tombstones-deleterange
Jun 9, 2022
Merged

batcheval: add range tombstone support for DeleteRange#77762
craig[bot] merged 1 commit intocockroachdb:masterfrom
erikgrinaker:mvcc-range-tombstones-deleterange

Conversation

@erikgrinaker
Copy link
Copy Markdown
Contributor

@erikgrinaker erikgrinaker commented Mar 14, 2022 •
edited
Loading

This patch adds the parameter UseExperimentalRangeTombstone for
DeleteRange, which deletes the span using an MVCC range tombstone.
The new version gate MVCCRangeTombstones must be checked before using
it. storage.ExperimentalMVCCDeleteRangeUsingTombstone() is added to
carry out the actual deletion.

This is a bare-bones implementation to allow writing range keys via the
KV API for testing and development purposes. It has significant
shortcomings, and will be fleshed out at a later time.

Touches #70415.
Replaces #76203.

Release note: None

@erikgrinaker erikgrinaker requested a review from aliher1911 March 14, 2022 13:12
@erikgrinaker erikgrinaker requested a review from a team as a code owner March 14, 2022 13:12
@erikgrinaker erikgrinaker self-assigned this Mar 14, 2022
@erikgrinaker erikgrinaker requested a review from a team as a code owner March 14, 2022 13:12
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Mar 14, 2022

This change is Reviewable

@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-deleterange branch 2 times, most recently from 171e3cd to 0c93f12 Compare March 16, 2022 12:45
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones branch from 19a02e0 to 430743b Compare March 18, 2022 15:23
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-deleterange branch 2 times, most recently from fc6b90f to d2c629b Compare March 18, 2022 15:45
This was referenced Mar 18, 2022
Merged
Merged
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones branch 2 times, most recently from b050b4a to bb5aa54 Compare March 24, 2022 11:39
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-deleterange branch 3 times, most recently from e06f38c to a4abc6c Compare March 26, 2022 14:17
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones branch from bb5aa54 to ba8eb78 Compare March 30, 2022 12:13
aliher1911
aliher1911 approved these changes Mar 31, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@aliher1911 aliher1911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is GH description correct regarding which commits belong to this PR? It looks only the last one is relevant.

Some nits as I think I already went through this code before.

@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones branch from ba8eb78 to 34e6ab0 Compare April 2, 2022 17:10
@erikgrinaker
Copy link
Copy Markdown
Contributor Author

erikgrinaker commented Apr 2, 2022 •
edited
Loading

Is GH description correct regarding which commits belong to this PR? It looks only the last one is relevant.

Ah, it's because I rebased the base branch mvcc-range-tombstones onto master, so the old commits that are no longer in the base branch are then considered part of this branch instead. It'll be resolved once I rebase all of the PRs to the new base branch. Updated the PR description to be agnostic to the number of prior commits.

@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-deleterange branch from a4abc6c to 5d5aecb Compare April 3, 2022 16:31
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones branch from 34e6ab0 to 4798ae2 Compare April 4, 2022 13:43
nicktrav
nicktrav reviewed Apr 5, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 20 of 20 files at r4, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @erikgrinaker)

pkg/storage/testdata/mvcc_histories/range_tombstone_mutations, line 1 at r4 (raw file):

# Set up some point keys, point/range tombstones, and intents.

nit: diagram would be useful here.

@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones branch 5 times, most recently from 2097ac5 to 3ad25e4 Compare April 11, 2022 20:36
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-deleterange branch from 5d5aecb to 53f4f4c Compare April 12, 2022 09:14
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-deleterange branch 2 times, most recently from cf525db to 413b5bc Compare May 1, 2022 15:08
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones branch from cb77b2e to 49aacac Compare May 9, 2022 08:36
@erikgrinaker erikgrinaker requested a review from a team as a May 9, 2022 08:36
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-deleterange branch from 413b5bc to bc45db8 Compare May 9, 2022 08:41
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones branch from 49aacac to e5bd560 Compare May 9, 2022 09:20
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-deleterange branch from bc45db8 to f7fea06 Compare May 9, 2022 09:24
@erikgrinaker erikgrinaker removed the request for review from a team May 28, 2022 13:16
@erikgrinaker erikgrinaker marked this pull request as draft May 28, 2022 13:17
@erikgrinaker erikgrinaker changed the base branch from mvcc-range-tombstones to master May 28, 2022 15:10
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-deleterange branch from f7fea06 to 6c3aa50 Compare May 28, 2022 15:11
This was referenced May 29, 2022
Merged
Merged
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-deleterange branch 2 times, most recently from 441ea16 to 8410a44 Compare June 4, 2022 11:14
@erikgrinaker erikgrinaker marked this pull request as ready for review June 4, 2022 11:15
@erikgrinaker
Copy link
Copy Markdown
Contributor Author

erikgrinaker commented Jun 4, 2022

@aliher1911 May want to take a quick look at this again, since there's been a few changes to range keys since it was initially submitted -- nothing major though.

@erikgrinaker erikgrinaker requested review from a team, aliher1911, itsbilal and jbowens June 4, 2022 11:16
aliher1911
aliher1911 approved these changes Jun 9, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@aliher1911 aliher1911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave it another read and it looks good.

There's something wrong with the request itself though. I can see latest version with all updates in "Files changed", but neither VS-Code view, nor Reviewable shows it. Both show diff with some older commit which rolls back changes from master massively.

@erikgrinaker
Copy link
Copy Markdown
Contributor Author

erikgrinaker commented Jun 9, 2022

I gave it another read and it looks good.

Thanks!

There's something wrong with the request itself though. I can see latest version with all updates in "Files changed", but neither VS-Code view, nor Reviewable shows it. Both show diff with some older commit which rolls back changes from master massively.

Are you sure you're using the correct base though? Remember, this used to be onto the mvcc-range-tombstones feature branch, but is now rebased onto master. May be that your local VSCode checkout and/or Reviewable gets that wrong and pulls in a bunch of stuff from the old base branch?

Using "Show full diff" in Reviewable shows the correct changes, as does comparing the branch to master in VSCode, so idk.

@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-deleterange branch from 8410a44 to 363ce8d Compare June 9, 2022 18:50
@erikgrinaker
batcheval: add MVCC range tombstone support for DeleteRange
3d71082 
This patch adds the parameter `UseExperimentalRangeTombstone` for
`DeleteRange`, which deletes the span using an MVCC range tombstone.
The new version gate `MVCCRangeTombstones` must be checked before using
it. `storage.ExperimentalMVCCDeleteRangeUsingTombstone()` is added to
carry out the actual deletion.

This is a bare-bones implementation to allow writing range keys via the
KV API for testing and development purposes. It has significant
shortcomings, and will be fleshed out at a later time.

Release note: None
@erikgrinaker erikgrinaker force-pushed the mvcc-range-tombstones-deleterange branch from 363ce8d to 3d71082 Compare June 9, 2022 19:36
@erikgrinaker
Copy link
Copy Markdown
Contributor Author

erikgrinaker commented Jun 9, 2022

TFTR!

bors r=aliher1911

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 9, 2022

Build succeeded:

@craig craig bot merged commit 4cced47 into cockroachdb:master Jun 9, 2022
@erikgrinaker erikgrinaker deleted the mvcc-range-tombstones-deleterange branch June 10, 2022 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicktrav nicktrav nicktrav left review comments

@jbowens jbowens Awaiting requested review from jbowens

@itsbilal itsbilal Awaiting requested review from itsbilal

+1 more reviewer

@aliher1911 aliher1911 aliher1911 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@erikgrinaker erikgrinaker

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erikgrinaker @cockroach-teamcity @nicktrav @aliher1911