{backup,changefeed,streaming}ccl: start populating pts target #74248

adityamaru · 2021-12-23T19:08:43Z

This change touches all jobs that create a protected timestamp
record before calling Protect. Previously, the created record
would contain the spans that the record was going to protect.

With this change, the record will also populate the Target
field on ptpb.Record with the object it is going to protect.
The Target field is a proto message defined in ptpb.Target
and can be one of:

Cluster
Tenants
Schema object (database or table)

For backups, this target field is determined based on the targets
passed in by the user via the BACKUP <target> query.

For changefeeds, this target is the group of tables on which the
changefeed is being started + system.descriptors table.

For the streaming job, this target is the tenant that is being
streamed.

This change does not touch any test files that create
a raw ptpb.Record for testing purposes. That will come in a follow
up PR where we actually teach Protect to validate and make use
of this Target field. A test for how backup chooses its target
will also come in a follow up PR once Protect actually writes
the encoded protobuf target field to the underlying system table.

Informs: #73727

Release note: None

cockroach-teamcity · 2021-12-23T19:08:50Z

This change is

adityamaru · 2021-12-31T16:35:36Z

First two commits from #74281.

adityamaru · 2022-01-05T19:02:18Z

This is RFAL! I have a test for the backup target selection in a follow-up PR over in c7dde69. I also moved the ptpb.Target message outside the ptpb.Record since in a future PR we need to encode this protobuf to bytes before we write to the target column in system.pts_records.

miretskiy · 2022-01-05T19:10:22Z

pkg/ccl/changefeedccl/changefeed.go

@@ -64,12 +66,26 @@ func createProtectedTimestampRecord(
 	progress.ProtectedTimestampRecord = uuid.MakeV4()
 	log.VEventf(ctx, 2, "creating protected timestamp %v at %v",
 		progress.ProtectedTimestampRecord, resolved)
-	spansToProtect := makeSpansToProtect(codec, targets)
+	deprecatedSpansToProtect := makeSpansToProtect(codec, targets)


we need both targets and spans?

Until we run the migration in #74281 we want to continue protecting spans so that jobs started in a mixed version state do not fail. Once the migration is complete, the Protect method in the protected ts Storage interface will stop writing these passed in spans to the underlying system.pts_records table (this will come as a follow up PR). So all jobs started after the migration might pass in a record with the spans field set, but this will not be persisted.

The idea is that GC for 22.1 will continue to respect both spans protected by the old subsystem, and targets protected by the new subsystem since this simplifies the migration in a mixed version state. In 22.2 with some elbow grease, we should be able to stop populating the spans field in the record entirely.

miretskiy · 2022-01-05T19:12:19Z

pkg/kv/kvserver/protectedts/ptpb/protectedts.proto

@@ -89,28 +89,6 @@ message Metadata {

 // Record corresponds to a protected timestamp.
 message Record {
-  message SchemaObjectsTarget {


is it safe to change proto like this? we have serialized records stored in protected keys, don't we?

These fields were added only a couple days ago in #74211, since this is unreleased code I think it is safe?

If nobody generated and serialized those, then it's fine (i'm thinking roachtests).

miretskiy

from changefeed side.

irfansharif

LGTM as an outsider to backup/changefeed/streaming.

pkg/ccl/backupccl/schedule_pts_chaining.go

pkg/kv/kvserver/protectedts/ptpb/protectedts.go

adityamaru · 2022-01-05T20:38:06Z

@gh-casper adding you for the streaming diff, thanks!

miretskiy · 2022-01-05T21:17:43Z

Also, <img class="emoji" title=":lgtm:" alt=":lgtm:" align="absmiddle" src="https://reviewable.io/lgtm.png" height="20" width="61"/> for the streaming stuff.

…

On Wed, Jan 5, 2022 at 3:38 PM Aditya Maru ***@***.***> wrote: @gh-casper <https://github.com/gh-casper> adding you for the streaming diff, thanks! — Reply to this email directly, view it on GitHub <#74248 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANA4FVGJF4HUYDM57VFSTP3UUST3XANCNFSM5KVK5VIA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because your review was requested.Message ID: ***@***.***>

This change touches all jobs that create a protected timestamp record before calling `Protect`. Previously, the created record would contain the spans that the record was going to protect. With this change, the record will also populate the `Target` field on `ptpb.Record` with the object it is going to protect. The `Target` field is a proto message defined in `ptpb.Target` and can be one of: - Cluster - Tenants - Schema object (database or table) For backups, this target field is determined based on the targets passed in by the user via the `BACKUP <target>` query. For changefeeds, this target is the group of tables on which the changefeed is being started + `system.descriptors` table. For the streaming job, this target is the tenant that is being streamed. This change does not touch any test files that create a raw `ptpb.Record` for testing purposes. That will come in a follow up PR where we actually teach `Protect` to validate and make use of this `Target` field. A test for how backup chooses its target will also come in a follow up PR once Protect actually writes the encoded protobuf target field to the underlying system table. Informs: cockroachdb#73727 Release note: None

adityamaru · 2022-01-06T20:07:44Z

@dt friendly ping for the backup stuff, I'll be adding a test in a follow-up PR once we start persisting these targets in the system tables.

dt · 2022-01-07T01:41:50Z

pkg/ccl/backupccl/backup_planning.go

+		for _, tenant := range backupManifest.Tenants {
+			tenantID = append(tenantID, roachpb.MakeTenantID(tenant.ID))
+		}
+		return ptpb.MakeTenantsTarget(tenantID)


Currently BACKUP syntax doesn't allow backing up table 245 and tenant 123, but that is a valid backup job. It looks like however that is not possible with the ptpb.Target? i.e. tenants target and schema object targets are mutually exclusive?

Not blocking, since it's unreachable with current syntax, just wondering.

It's not possible today yeah, we could in the future switch the Record to hold a repeated Target instead of a single target to accommodate for this.

adityamaru · 2022-01-07T02:25:27Z

TFTR!

bors r+

craig · 2022-01-07T03:23:29Z

Build succeeded:

GitHub CI (Cockroach)

adityamaru mentioned this pull request Dec 23, 2021

protectedts: rework protected timestamp storage for multi-tenant model #73727

Closed

23 tasks

adityamaru force-pushed the populate-pts-record-target branch 4 times, most recently from c7dde69 to 3d7425b Compare December 28, 2021 17:42

adityamaru requested review from irfansharif, dt and arulajmani December 31, 2021 16:35

adityamaru mentioned this pull request Dec 31, 2021

ptstorage: change Protect and GetRecord to work with target column #74297

Merged

adityamaru force-pushed the populate-pts-record-target branch from 3d7425b to ed08564 Compare January 5, 2022 18:53

adityamaru marked this pull request as ready for review January 5, 2022 19:02

adityamaru requested a review from a team January 5, 2022 19:02

adityamaru requested review from a team as code owners January 5, 2022 19:02

adityamaru requested review from miretskiy and removed request for a team January 5, 2022 19:02

miretskiy reviewed Jan 5, 2022

View reviewed changes

miretskiy approved these changes Jan 5, 2022

View reviewed changes

irfansharif approved these changes Jan 5, 2022

View reviewed changes

pkg/ccl/backupccl/schedule_pts_chaining.go Outdated Show resolved Hide resolved

pkg/kv/kvserver/protectedts/ptpb/protectedts.go Outdated Show resolved Hide resolved

adityamaru requested a review from gh-casper January 5, 2022 20:37

adityamaru removed request for a team and gh-casper January 6, 2022 02:23

adityamaru force-pushed the populate-pts-record-target branch from ed08564 to 2528c38 Compare January 6, 2022 02:37

dt reviewed Jan 7, 2022

View reviewed changes

dt approved these changes Jan 7, 2022

View reviewed changes

craig bot merged commit d191f64 into cockroachdb:master Jan 7, 2022

adityamaru deleted the populate-pts-record-target branch January 7, 2022 03:53

rafiss mentioned this pull request Aug 5, 2024

protectedts: MakeSchemaObjectsTarget should protect the schema object's metadata #127345

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

{backup,changefeed,streaming}ccl: start populating pts target #74248

{backup,changefeed,streaming}ccl: start populating pts target #74248

adityamaru commented Dec 23, 2021 •

edited

Loading

cockroach-teamcity commented Dec 23, 2021

adityamaru commented Dec 31, 2021

adityamaru commented Jan 5, 2022

miretskiy Jan 5, 2022

adityamaru Jan 5, 2022

miretskiy Jan 5, 2022

adityamaru Jan 5, 2022

miretskiy Jan 5, 2022

miretskiy left a comment •

edited by petermattis

Loading

irfansharif left a comment

adityamaru commented Jan 5, 2022

miretskiy commented Jan 5, 2022 via email •

edited by petermattis

Loading

adityamaru commented Jan 6, 2022

dt Jan 7, 2022

adityamaru Jan 7, 2022

adityamaru commented Jan 7, 2022

craig bot commented Jan 7, 2022

{backup,changefeed,streaming}ccl: start populating pts target #74248

{backup,changefeed,streaming}ccl: start populating pts target #74248

Conversation

adityamaru commented Dec 23, 2021 • edited Loading

cockroach-teamcity commented Dec 23, 2021

adityamaru commented Dec 31, 2021

adityamaru commented Jan 5, 2022

miretskiy Jan 5, 2022

Choose a reason for hiding this comment

adityamaru Jan 5, 2022

Choose a reason for hiding this comment

miretskiy Jan 5, 2022

Choose a reason for hiding this comment

adityamaru Jan 5, 2022

Choose a reason for hiding this comment

miretskiy Jan 5, 2022

Choose a reason for hiding this comment

miretskiy left a comment • edited by petermattis Loading

Choose a reason for hiding this comment

irfansharif left a comment

Choose a reason for hiding this comment

adityamaru commented Jan 5, 2022

miretskiy commented Jan 5, 2022 via email • edited by petermattis Loading

adityamaru commented Jan 6, 2022

dt Jan 7, 2022

Choose a reason for hiding this comment

adityamaru Jan 7, 2022

Choose a reason for hiding this comment

adityamaru commented Jan 7, 2022

craig bot commented Jan 7, 2022

adityamaru commented Dec 23, 2021 •

edited

Loading

miretskiy left a comment •

edited by petermattis

Loading

miretskiy commented Jan 5, 2022 via email •

edited by petermattis

Loading