-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
spanconfigsqltranslator: populate protected_timestamps in SpanConfig #74803
Conversation
First two commits are from #74737. |
84786e3
to
1118d9e
Compare
pkg/roachpb/span_config.proto
Outdated
// ProtectedTimestamps captures all the protected timestamp records that apply | ||
// to the range. The timestamp values represent the timestamps after which | ||
// data will be protected from GC, if the protection succeeds. | ||
repeated util.hlc.Timestamp protected_timestamps = 10 [(gogoproto.nullable) = false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is subject to change in #74765
1118d9e
to
2e87a4b
Compare
pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go
Outdated
Show resolved
Hide resolved
pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go
Outdated
Show resolved
Hide resolved
pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go
Outdated
Show resolved
Hide resolved
2e87a4b
to
733eb94
Compare
733eb94
to
57f3502
Compare
@arulajmani @irfansharif this should be RFAL! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM mod comments below.
pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go
Outdated
Show resolved
Hide resolved
pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go
Outdated
Show resolved
Hide resolved
pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go
Outdated
Show resolved
Hide resolved
34db9a9
to
29a4a03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 22 files at r7, 19 of 22 files at r8, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @dt, and @irfansharif)
pkg/clusterversion/cockroach_versions.go, line 386 at r8 (raw file):
Version: roachpb.Version{Major: 21, Minor: 2, Internal: 48}, }, {
To confirm, it's okay for us to do this now because we haven't released any betas?
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 376 at r8 (raw file):
// Copy the ProtectionPolicies that apply to the table's SpanConfig onto its // SubzoneSpanConfig. subzoneSpanConfig.GCPolicy.ProtectionPolicies = tableSpanConfig.GCPolicy.ProtectionPolicies[:]
nit: would it be clearer if you pulled the right hand side of the assignment in a separate variable and used it to assign the ProtectionPolicies wherever you need to?
Yes, but also because we don't support upgrades from alphas/betas to prod clusters. Re-ordering these versions is safe any time before the actual release itself (not even the branch cut). |
pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go
Outdated
Show resolved
Hide resolved
@@ -11,6 +11,8 @@ SELECT node_id, name FROM crdb_internal.leases ORDER BY name | |||
0 eventlog | |||
0 jobs | |||
0 locations | |||
0 protected_ts_meta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, what's this about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a 100% sure, but a little digging led me to #69008 where we switched to leasing all system descriptors except a few in a deny list. The PTS tables are not in this deny list so I guess we expect to see them here?
pkg/spanconfig/spanconfigsqltranslator/protectedts_state_reader.go
Outdated
Show resolved
Hide resolved
29a4a03
to
50619f0
Compare
Discussed offline, going to keep it as is for now since it avoids an extra copy. |
This change teaches the SQLTranslator to hydrate the SpanConfigs for a table with protected timestamps that apply to that table. Concretely, this change initializes a spanconfig.ProtectedTimestampStateReader in the txn in which the translation is taking place, thereby providing a transactional view of the system.protected_ts_records table. After generating the span configurations based on the zone configurations that apply to the table, we hydrate the newly introduced protected_timestamps field on each span configuration with all the protected timestamps that apply to this table. This includes protected timestamp records that directly target this table, as well as records targetting the table's parent database. This information is obtained from the ProtectedTimestampStateReader mentioned above. Additionally, this change modifies StartTenant to allow secondary tenants to interact with the protected timestamp subsystem using a "real" protectedts.Provider provided the migration EnableProtectedTimestampsForTenant has run. For testing purposes, this change teaches the data driven framework of two additional commands protect and release. Informs: cockroachdb#73727 Release note: None
50619f0
to
7c63710
Compare
Bazel timeout seems unrelated. bors r=irfansharif,arulajmani |
Build succeeded: |
This change teaches the SQLTranslator to hydrate the SpanConfigs
for a table with protected timestamps that apply to that table.
Concretely, this change initializes a spanconfig.ProtectedTimestampStateReader
in the txn in which the translation is taking place, thereby
providing a transactional view of the system.protected_ts_records
table. After generating the span configurations based on the zone
configurations that apply to the table, we hydrate the newly
introduced protected_timestamps field on each span configuration
with all the protected timestamps that apply to this table. This
includes protected timestamp records that directly target this
table, as well as records targetting the table's parent database.
This information is obtained from the ProtectedTimestampStateReader
mentioned above.
Additionally, this change modifies StartTenant to allow secondary
tenants to interact with the protected timestamp subsystem using a
"real" protectedts.Provider provided the migration
EnableProtectedTimestampsForTenant has run.
For testing purposes, this change teaches the data driven framework
of two additional commands protect and release.
Informs: #73727
Release note: None