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.

Sign up for GitHub

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

Jump to bottom

kvserver: emit MVCC range tombstones over rangefeeds #76443

Closed
wants to merge 2 commits into from
Closed

kvserver: emit MVCC range tombstones over rangefeeds #76443

wants to merge 2 commits into from

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Feb 11, 2022 •
edited
Loading

storage: disable range keys in iterators when unsupported

This patch falls back to IterKeyTypePointsOnly if the Pebble database
is not at FormatRangeKeys yet, to avoid a Pebble panic. These
databases can't contain range keys by definition, so this has no
practical effect.

Release note: None

kvserver: emit MVCC range tombstones over rangefeeds

This patch adds MVCC range tombstone support to range feeds. Whenever a
DeleteRange request with UseExperimentalRangeTombstone succeeds, a new
MVCCDeleteRangeOp logical op type is recorded and emitted across the
range feed as an RangeFeedDeleteRange event.

MVCC range tombstones are under active development, and highly
experimental. They will only be used (and emitted) after checking
storage.CanUseExperimentalMVCCRangeTombstones, which requires a
version gate and environment variable to be enabled.

Changefeeds will emit an error for these events. We do not expect to see
these in online spans with changefeeds, since they are initially only
planned for use with schema GC and import rollbacks.

The rangefeed client library has been extended with support for these
events, but no existing callers handle them for the same reason as
changefeeds. Initial scans do not emit regular tombstones, and thus not
range tombstones either, but catchup scans will emit them if
encountered.

This only has rudimentary integration testing, as MVCC range tombstones
are still experimental and lack e.g. persistence and replication -- more
comprehensive tests will be implemented later.

Resolves #70433.

Release note: None

@erikgrinaker erikgrinaker self-assigned this Feb 11, 2022
@erikgrinaker erikgrinaker requested review from a team as code owners February 11, 2022 19:50
@cockroach-teamcity
Copy link
Member

This change is Reviewable

erikgrinaker
erikgrinaker commented Feb 12, 2022
View reviewed changes
pkg/ccl/streamingccl/streamproducer/event_stream.go
Comment on lines 382 to +389
// TODO(yevgeniy): Handle SSTs.
// TODO(erikgrinaker): Handle DeleteRange events (MVCC range tombstones).
Copy link
Contributor Author

@erikgrinaker erikgrinaker Feb 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@miretskiy @dt Do we need to address these TODOs before stability in order to test streaming replication with 22.1? That is, add handling for AddSSTable and MVCC range tombstones in streaming replication.

Copy link
Contributor

Choose a reason for hiding this comment

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

We decided that we will test what we can -- even if some of those ops are not ready yet.
So, these TODOs are fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. We have pending PRs/issues for the necessary APIs to ingest these on the target cluster, and plan to get these in for 22.1 in experimental form. We'll be maturing these APIs during stability and the 22.1 cycle. So if we get the streaming ingestion code in place before stability as well (i.e. simply make the appropriate API calls) then we should be able to do full end-to-end testing of streaming replication sometime during the 22.1 cycle.

@erikgrinaker erikgrinaker force-pushed the rangefeed-range-keys branch 2 times, most recently from 38019ab to afe3afb Compare February 12, 2022 15:51
miretskiy
miretskiy reviewed Feb 14, 2022
View reviewed changes
Copy link
Contributor

@miretskiy miretskiy left a comment

Choose a reason for hiding this comment

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

few nits questions mostly. I'm sure other reviewers will have more comments.

pkg/ccl/streamingccl/streamproducer/event_stream.go
Comment on lines 382 to +389
// TODO(yevgeniy): Handle SSTs.
// TODO(erikgrinaker): Handle DeleteRange events (MVCC range tombstones).
Copy link
Contributor

Choose a reason for hiding this comment

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

We decided that we will test what we can -- even if some of those ops are not ready yet.
So, these TODOs are fine for now.

pkg/kv/kvserver/rangefeed/catchup_scan.go Show resolved Hide resolved
pkg/kv/kvserver/rangefeed/catchup_scan.go Show resolved Hide resolved
pkg/roachpb/data.go Outdated Show resolved Hide resolved
@erikgrinaker erikgrinaker force-pushed the rangefeed-range-keys branch 2 times, most recently from d6106c7 to edb3d8c Compare February 14, 2022 22:08
aliher1911
aliher1911 approved these changes Feb 18, 2022
View reviewed changes
Copy link
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.

LGTM

@erikgrinaker erikgrinaker force-pushed the rangefeed-range-keys branch 4 times, most recently from 438be2a to 583af44 Compare February 22, 2022 14:03
@erikgrinaker
storage: disable range keys in iterators when unsupported
e2ff5e3 
This patch falls back to `IterKeyTypePointsOnly` if the Pebble database
is not at `FormatRangeKeys` yet, to avoid a Pebble panic. These
databases can't contain range keys by definition, so this has no
practical effect.

Release note: None
@erikgrinaker
kvserver: emit MVCC range tombstones over rangefeeds
68f91c1 
This patch adds MVCC range tombstone support to range feeds. Whenever an
MVCC range tombstone is written, e.g. a `DeleteRange` request with
`UseExperimentalRangeTombstone`, a new `MVCCDeleteRangeOp` logical op
type is recorded and emitted across the range feed as an
`RangeFeedDeleteRange` event.

MVCC range tombstones are under active development, and highly
experimental. They will only be used (and emitted) after checking
`storage.CanUseExperimentalMVCCRangeTombstones`, which requires a
version gate and environment variable to be enabled.

Changefeeds will emit an error for these events. We do not expect to see
these in online spans with changefeeds, since they are initially only
planned for use with schema GC and import rollbacks.

The rangefeed client library has been extended with support for these
events, but no existing callers handle them for the same reason as
changefeeds. Initial scans do not emit regular tombstones, and thus not
range tombstones either, but catchup scans will emit them if
encountered.

This only has rudimentary integration testing, as MVCC range tombstones
are still experimental and lack e.g. persistence and replication -- more
comprehensive tests will be implemented later.

Release note: None
@erikgrinaker erikgrinaker marked this pull request as draft February 23, 2022 09:33
@erikgrinaker
Copy link
Contributor Author

Closing in favor of #82718.

@erikgrinaker erikgrinaker deleted the rangefeed-range-keys 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

@miretskiy miretskiy miretskiy left review comments

@aliher1911 aliher1911 aliher1911 approved these changes

@dt dt Awaiting requested review from dt

@stevendanna stevendanna Awaiting requested review from stevendanna

Assignees

@erikgrinaker erikgrinaker

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kvserver: C2C handling of DeleteRange aka MVCC range tombstones
4 participants
@erikgrinaker @cockroach-teamcity @aliher1911 @miretskiy