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

release-21.2: changefeedccl: resurrect manual protected timestamps #77589

Conversation

samiskin
Copy link
Contributor

@samiskin samiskin commented Mar 10, 2022

Backport 1/1 commits from #76605.
Backport 1/1 commits from #77606.

The changes from the master version are in their own commits, mostly around fixing tests. I also had to backport a small change from #75897 where I split out isBehind into its own function.

Release justification: new feature that is desired by doordash that is gated behind a cluster setting

/cc @cockroachdb/release


Previously changefeeds only laid down protected timestamp records to
protect against either an ongoing backfill or the changefeed lagging
behind. This is insufficient in cases such as if the gcttl is very
short, recurring errors retry the changefeed for too long, or in
upcoming work to enable serverless to shut down idle changefeeds.

This PR removes the manual PTS protection on backfills and begins an
async routine on the changeFrontier that updates the protected timestamp
record to the current highwater mark.

Fixes #76247

Release note (enterprise change): changefeeds running on tables with a
low gcttl will function more reliably due to protected timestamps being
maintained for the changefeed targets at the resolved timestamp of the
changefeed. The frequency at which the protected timestamp is updated
to the resolved timestamp can be configured through the
changefeed.protect_timestamp_interval cluster setting. If the
changefeed lags too far behind such that storage of old data becomes an
issue, cancelling the changefeed will release the protected timestamps
and allow garbage collection to resume. If
protect_data_from_gc_on_pause is unset, pausing the changefeed will
release the existing protected timestamp record.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@samiskin samiskin force-pushed the backport-changefeed-active-protected-ts branch from 11d326c to 03d512f Compare March 10, 2022 15:54
@samiskin samiskin changed the title Backport changefeed active protected ts release-21.2: changefeedccl: resurrect manual protected timestamps Mar 10, 2022
Previously changefeeds only laid down protected timestamp records to
protect against either an ongoing backfill or the changefeed lagging
behind.  This is insufficient in cases such as if the gcttl is very
short, recurring errors retry the changefeed for too long, or in
upcoming work to enable serverless to shut down idle changefeeds.

This PR removes the manual PTS protection on backfills and begins an
async routine on the changeFrontier that updates the protected timestamp
record to the current highwater mark.

Fixes cockroachdb#76247

Release note (enterprise change): changefeeds running on tables with a
low gcttl will function more reliably due to protected timestamps being
maintained for the changefeed targets at the resolved timestamp of the
changefeed.  The frequency at which the protected timestamp is updated
to the resolved timestamp can be configured through the
`changefeed.protect_timestamp_interval` cluster setting. If the
changefeed lags too far behind such that storage of old data becomes an
issue, cancelling the changefeed will release the protected timestamps
and allow garbage collection to resume. If
`protect_data_from_gc_on_pause` is unset, pausing the changefeed will
release the existing protected timestamp record.
@samiskin samiskin force-pushed the backport-changefeed-active-protected-ts branch from 03d512f to 8272030 Compare March 10, 2022 16:00
@samiskin samiskin marked this pull request as ready for review March 10, 2022 16:04
@samiskin samiskin requested a review from a team as a code owner March 10, 2022 16:04
@samiskin samiskin requested review from gh-casper and miretskiy and removed request for a team March 10, 2022 16:04
@samiskin
Copy link
Contributor Author

@miretskiy There are 4 commits, with the 2 that aren't cherry-picks being the actual changes from the master versions of the PRs

// ActiveProtectedTimestamps enables always having protected timestamps laid
// down that are periodically advanced to the highwater mark.
var ActiveProtectedTimestamps = settings.RegisterBoolSetting(
"changefeed.active_protected_timestamps",
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename to changefeed.active_protected_timestamps.enabled

// water) will have a protected timestamp.
return nil
if cp.ProtectedTimestampRecord == uuid.Nil {
// This case shouldn't occur when ActiveProtectedTimestamps is enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the comment should be updated; this event can easily happen if Active PTS was off before, and now it's on (btw, this will definitely happen if you're upgrading cluster).

I think flipping this flag, even multiple times, is probably okay.

var ActiveProtectedTimestamps = settings.RegisterBoolSetting(
"changefeed.active_protected_timestamps",
"if set, rather than only protecting changefeed targets from garbage collection during backfills, data will always be protected up to the changefeed's frontier",
false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to add this flag into retiredSettings in registry.go (on master).

@samiskin samiskin force-pushed the backport-changefeed-active-protected-ts branch 2 times, most recently from 0d5d5bb to 10fa208 Compare March 10, 2022 18:06
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.

None yet

3 participants