ptstorage: change Protect and GetRecord to work with target column#74297
ptstorage: change Protect and GetRecord to work with target column#74297craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
First three commits from #74248. |
935fc5d to
46106ca
Compare
|
First commit is from #74248. edit: rebased |
b0ac63e to
3f16f9e
Compare
|
@ajwerner @arulajmani friendly ping for a review on this! |
ajwerner
left a comment
There was a problem hiding this comment.
cursory pass this looks fine
Reviewed 6 of 19 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, @irfansharif, and @msbutler)
pkg/kv/kvserver/protectedts/ptstorage/storage.go, line 146 at r1 (raw file):
} // TODO(during review): To avoid breaking protected timestamps before the
How hard is it to put in a cluster setting or testing knob that we pair with the version gate in all the places we check it that we default to false? With that, we wouldn't break anything and we can test the new logic by enabling that cluster setting. Then, before we release, we get rid of the cluster setting or testing knob.
arulajmani
left a comment
There was a problem hiding this comment.
Reviewed 6 of 19 files at r1, all commit messages.
Reviewable status:
pkg/kv/kvserver/protectedts/ptstorage/sql.go, line 126 at r1 (raw file):
` getRecordsWithoutTargetQueryBase = `
nit: might be worth commenting that the Target column was added in a migration which necessitates these with and without target versions of queries.
pkg/kv/kvserver/protectedts/ptstorage/storage.go, line 83 at r1 (raw file):
return errors.Wrap(err, "failed to marshal spans") } it, err := p.ex.QueryIteratorEx(ctx, "protectedts-protect", txn,
nit: s/protectedts-protected/protectedts-deprecated-protect/
pkg/kv/kvserver/protectedts/ptstorage/storage.go, line 142 at r1 (raw file):
meta = []byte{} } if !p.settings.Version.IsActive(ctx, clusterversion.AlterSystemProtectedTimestampAddColumn) {
Might be worth adding a comment about this here as well and a TODO to remove this in v22.2.
pkg/kv/kvserver/protectedts/ptstorage/storage.go, line 146 at r1 (raw file):
Previously, ajwerner wrote…
How hard is it to put in a cluster setting or testing knob that we pair with the version gate in all the places we check it that we default to false? With that, we wouldn't break anything and we can test the new logic by enabling that cluster setting. Then, before we release, we get rid of the cluster setting or testing knob.
We could also run some of the tests that are supposed to work before and after theAlterSystemProtectedTimestampAddColumn migration with the testing knob set to both values for 22.2.
pkg/kv/kvserver/protectedts/ptstorage/storage.go, line 213 at r1 (raw file):
} } else { row, err = p.ex.QueryRowEx(ctx, "protectedts-GetRecord", txn,
nit: Similar to how you did for Protect, you could pull this logic out in a deprecated method as well. That way, when we come back to this to cleanup, it'll be easier.
3f16f9e to
7775d4c
Compare
adityamaru
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/protectedts/ptstorage/storage.go, line 142 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Might be worth adding a comment about this here as well and a TODO to remove this in v22.2.
Done.
pkg/kv/kvserver/protectedts/ptstorage/storage.go, line 146 at r1 (raw file):
Done.
Adding a testing knob that currently defaults to false. All tests that want to run with the new system have to override this knob. Prior to the release, we will default this knob to true or rename it toDisablePTSForMultiTenantand have all the tests that want to run with the old system override it.
|
I'm not sure what happened to reviewable, but I have addressed all the comments so this if RFAL. |
| it, err := p.ex.QueryIteratorEx(ctx, "protectedts-GetRecords", txn, | ||
| sessiondata.InternalExecutorOverride{User: security.NodeUserName()}, | ||
| getRecordsQuery) | ||
| getRecordsWithoutTargetQuery) |
There was a problem hiding this comment.
this is wrong, it should be getRecordsQuery. I'm just waiting for CI to shake out all failures before repushing.
7775d4c to
6f76387
Compare
arulajmani
left a comment
There was a problem hiding this comment.
Couple of minor comments, but otherwise
Reviewed 1 of 19 files at r1, 15 of 24 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:
pkg/kv/kvserver/protectedts/ptprovider/provider.go, line 47 at r3 (raw file):
// New creates a new protectedts.Provider. func New(cfg Config, knobs *protectedts.TestingKnobs) (protectedts.Provider, error) {
Any reason to not put these on the Config struct itself?
pkg/kv/kvserver/protectedts/ptstorage/storage.go, line 156 at r3 (raw file):
// TODO(adityamaru): Delete in 22.2 once we exclusively protect `target`s. if !p.settings.Version.IsActive(ctx, clusterversion.AlterSystemProtectedTimestampAddColumn) || !p.knobs.EnableProtectedTimestampForMultiTenant {
nit: maybe pull this logic into a useDeprecatedProtectedTSStorage or something -- we use it in a couple of places and it's slightly more readable than this disjunction.
pkg/ccl/backupccl/backup_test.go, line 6759 at r3 (raw file):
dir, dirCleanupFn := testutils.TempDir(t) defer dirCleanupFn() clusterArgs := base.TestClusterArgs{}
revert?
pkg/kv/kvserver/protectedts/ptstorage/storage_test.go, line 324 at r3 (raw file):
// If set to true, the test will be run on a cluster version prior to // AlterSystemProtectedTimestampAddColumn, thereby testing the "old" protected
Maybe reference the testing knob instead?
Protected timestamp records now apply on a `ptpb.Target` instead of `roachpb.Spans`. This change modifies the `Protect` and `GetRecord` methods of the ptstorage interface. Protect will persist the `ptpb.Target` specified on the passed in `ptpb.Record` as an encoded protocol buffer in the `target` column, and `GetRecord` will read the `target` column when constructing a `ptpb.Record` to return to the caller. If we are running in a mixed version state where the migration that adds the `target` column to the system.pts_records table has not run, we use the existing logic to protect Spans. This logic has wholesale been moved to `deprecatedProtect` method. Once the migration has run, all calls to protect must have a non-nil `ptpb.Target` field on the record, as we will no longer be persisting the `DeprecatedSpans` field in the underlying system table. Post migration, we do not use the `kv.protectedts.max_spans` to enforce any limits, but continue to use `kv.protectedts.max_bytes` to enforce a ceiling on the total size of encoded targets we can store in the subsystem. Most tests in `ptstorage_test` continue to run in both a pre and post migration state to ensure that `Storage` is handling both span backed and target backed records correctly. This can be cleaned up in a future version when we get rid of spans from the pts record entirely. All other PTS tests in the `kvserver` package run on a version prior to the pts migration since the remainder of the new pts system is yet to be implemented. These tests will be revisited as and when we develop those pieces of the subsystem. Informs: cockroachdb#73727 Release note: None
6f76387 to
6ea1c5e
Compare
adityamaru
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/protectedts/ptprovider/provider.go, line 47 at r3 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Any reason to not put these on the
Configstruct itself?
good point, done.
pkg/kv/kvserver/protectedts/ptstorage/storage.go, line 156 at r3 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: maybe pull this logic into a
useDeprecatedProtectedTSStorageor something -- we use it in a couple of places and it's slightly more readable than this disjunction.
i like it, thanks!
pkg/ccl/backupccl/backup_test.go, line 6759 at r3 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
revert?
Done.
pkg/kv/kvserver/protectedts/ptstorage/storage_test.go, line 324 at r3 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Maybe reference the testing knob instead?
yup stale comment, done.
|
@ajwerner let me know if you'd like to take another pass at this, thanks! |
arulajmani
left a comment
There was a problem hiding this comment.
Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status:
|
borsing to cleanup some of the PRs on top of this will keep a lookout for additional comments TFTR! bors r=arulajmani |
|
Build succeeded: |
Protected timestamp records now apply on a
ptpb.Targetinsteadof
roachpb.Spans. This change modifies theProtectandGetRecordmethods of the ptstorage interface. Protect will persist the
ptpb.Targetspecified on the passed inptpb.Recordas an encodedprotocol buffer in the
targetcolumn, andGetRecordwill read thetargetcolumn when constructing aptpb.Recordto return to the caller.If we are running in a mixed version state where the migration that adds
the
targetcolumn to the system.pts_records table has not run,we use the existing logic to protect Spans. This logic has wholesale
been moved to
deprecatedProtectmethod.Once the migration has run, all calls to protect must
have a non-nil
ptpb.Targetfield on the record, as we will no longerbe persisting the
DeprecatedSpansfield in the underlying system table.Post migration, we do not use the
kv.protectedts.max_spansto enforceany limits, but continue to use
kv.protectedts.max_bytesto enforce aceiling on the total size of encoded targets we can store in the subsystem.
Most tests in
ptstorage_testcontinue to run in both a pre and post migrationstate to ensure that
Storageis handling both span backed and target backedrecords correctly. This can be cleaned up in a future version when we get
rid of spans from the pts record entirely. All other PTS tests in the
kvserverpackage run on a version prior to the pts migration since the remainder of the
new pts system is yet to be implemented. These tests will be revisited as and
when we develop those pieces of the subsystem.
Informs: #73727
Release note: None