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

protectedts: add target field to pts record #74211

Merged
merged 1 commit into from
Jan 4, 2022

Conversation

adityamaru
Copy link
Contributor

@adityamaru adityamaru commented Dec 22, 2021

Protected timestamp records are moving away from
the notion of protecting spans, and instead will operate
on objects. Objects will be defined as:

  • Cluster
  • Tenant
  • Schema objects (database and table)

This change deprecates the Spans field on ptpb.Record.
Additionally, it adds a oneof target field that reflects
which of the above objects the record corresponds to.
This information will be needed by the SQLTranslator/Reconciler
in future work to emit SpanConfigurations based on the type
of object the job is protecting.

Informs: #73727

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru adityamaru changed the title protectedts,migrations,clusterversion: add schema_object_ids to support new protected timestamps protectedts: add target field to pts record Dec 22, 2021
@adityamaru
Copy link
Contributor Author

Wanted to start getting some feedback on the skeleton work while we are still iterating on the RFC.

// DeprecatedSpans are the spans which this Record protects.
repeated roachpb.Span deprecated_spans = 7 [(gogoproto.nullable) = false];

// Target holds information about the objects on which the protected timestamp
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Reword to avoid "objects"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

pkg/kv/kvserver/protectedts/ptpb/protectedts.proto Outdated Show resolved Hide resolved
pkg/kv/kvserver/protectedts/ptpb/protectedts.proto Outdated Show resolved Hide resolved
pkg/kv/kvserver/protectedts/ptpb/protectedts.proto Outdated Show resolved Hide resolved
Copy link
Contributor

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

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

The proto changes LGTM, and it makes sense to add a corresponding column to the protected timestamp system table in each tenant. I'll leave it to you what you want to do with this PR (either merge here, some side branch, or just leave open); perhaps others would prefer to see the corresponding RFC out for public review first.

pkg/kv/kvserver/protectedts/ptpb/protectedts.proto Outdated Show resolved Hide resolved
pkg/kv/kvserver/protectedts/ptpb/protectedts.proto Outdated Show resolved Hide resolved
Protected timestamp records are moving away from
the notion of protecting spans, and instead will operate
on objects. Objects will be defined as:

- Cluster
- Tenant
- Schema objects (database and table)

This change deprecates the Spans field on `ptpb.Record`.
Additionally, it adds a `oneof` target field that reflects
which of the above objects the record corresponds to.
This information will be needed by the SQLTranslator/Reconciler
in future work to emit SpanConfigurations based on the type
of object the job is protecting.

Informs: cockroachdb#73727

Release note: None
@adityamaru
Copy link
Contributor Author

I'm leaning towards checking atleast the protobuf changes in. I think the move to protect schema objects is pretty uncontroversial. I'll leave the remaining PRs as drafts until the RFC is generally accepted.

@adityamaru adityamaru marked this pull request as ready for review December 23, 2021 21:11
@adityamaru adityamaru requested a review from a team December 23, 2021 21:11
@adityamaru adityamaru requested review from a team as code owners December 23, 2021 21:11
@adityamaru adityamaru requested review from dt and removed request for a team December 23, 2021 21:11
Copy link
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 13 of 15 files at r1, 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @irfansharif)

@adityamaru
Copy link
Contributor Author

Bazel failure is changefeedccl package timing out under race.

TFTR!

bors r=irfansharif,arulajmani

@craig
Copy link
Contributor

craig bot commented Jan 4, 2022

Build succeeded:

@craig craig bot merged commit 798a3e5 into cockroachdb:master Jan 4, 2022
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

4 participants