spanconfigsqltranslator: introduce a pts table reader #74737

adityamaru · 2022-01-12T16:59:05Z

This change introduces a ProtectedTimestampTableReader
that provides a txn scoped, in-memory view of the system
table that stores protected timestamp records.

The SQLTranslator will use this table reader to generate
SpanConfigs and SystemSpanConfigs in a follow up PR.

Informs: #73727

Release note: None

cockroach-teamcity · 2022-01-12T16:59:14Z

This change is

adityamaru · 2022-01-12T16:59:42Z

First commit is from #74297.

arulajmani

Mostly looks good 💯

Reviewed 19 of 19 files at r1, 2 of 5 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @irfansharif, and @stevendanna)

pkg/spanconfig/spanconfig.go, line 247 at r2 (raw file):

// protected timestamps that apply to the different targets that can be
// protected from GC.
type ProtectedTimestampTableReader interface {

Now that we're only using this thing in the Translator, and not in the Reconciler, does this thing need to be a top level interface? Or can we contain it to the spanconfigsqltranslator package

pkg/spanconfig/spanconfigprotectedts/protectedts_table_reader.go, line 76 at r2 (raw file):

// read from the in-memory store.
type ProtectedTimestampTableReader struct {
	protections map[key][]hlc.Timestamp

Instead of this single map, you could instead have 3 separate fields for cluster, schema objects, and tenants instead. For schema objects/tenants you could use a map keyed by descriptor ID/tenantID. This would allow you to get rid of the key encoding scheme (and string allocations) above.

pkg/spanconfig/spanconfigprotectedts/protectedts_table_reader.go, line 124 at r2 (raw file):

}

func (p *ProtectedTimestampTableReader) unmarshalProtectedTimestampRecord(

Should we make this into a decoder struct similar to what we do for system.zones and system.descriptors? It might be worth doing that because we'll use this thing in the sqlwatcher as well.

pkg/spanconfig/spanconfigprotectedts/protectedts_table_reader.go, line 163 at r2 (raw file):

	ctx context.Context, ptsRecordsSystemTable string, ie sqlutil.InternalExecutor, txn *kv.Txn,
) (retErr error) {
	getProtectedTimestampRecordStmt := fmt.Sprintf(`SELECT ts, target FROM %s`, ptsRecordsSystemTable)

Can we use the GetState method on protectedts.Storage instead by threading in that interface into here?

pkg/spanconfig/spanconfigprotectedts/protectedts_table_reader_test.go, line 36 at r2 (raw file):

)

func TestProtectedTimestampTableReader(t *testing.T) {

You could convert this thing into a datadriven test -- it'll allow you to test things like different records that target the same schema object/tenant/cluster easily.

pkg/spanconfig/spanconfigprotectedts/protectedts_table_reader_test.go, line 66 at r2 (raw file):

	// Protect the cluster.
	clusterProtect := s0.Clock().Now()

nit: Instead of using Now(), you can instead create synthetic timestamps. Check out the TestBuffer in the spanconfigsqlwatcher package

pkg/spanconfig/spanconfigprotectedts/protectedts_table_reader_test.go, line 72 at r2 (raw file):

	tenantProtect := s0.Clock().Now()
	mkRecordAndProtect(tenantProtect, ptpb.MakeTenantsTarget([]roachpb.TenantID{roachpb.MakeTenantID(1),
		roachpb.MakeTenantID(2)}))

Might be worth protecting a tenant in a separate record and ensuring that all tenant protections are returned.

adityamaru

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @irfansharif, and @stevendanna)

pkg/spanconfig/spanconfig.go, line 247 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Now that we're only using this thing in the Translator, and not in the Reconciler, does this thing need to be a top level interface? Or can we contain it to the spanconfigsqltranslator package

i don't think it needs to be a top-level interface, removed and moved into the spanconfigsqltranslator package.

pkg/spanconfig/spanconfigprotectedts/protectedts_table_reader.go, line 76 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Instead of this single map, you could instead have 3 separate fields for cluster, schema objects, and tenants instead. For schema objects/tenants you could use a map keyed by descriptor ID/tenantID. This would allow you to get rid of the key encoding scheme (and string allocations) above.

good idea yeah, simplifies everything.

pkg/spanconfig/spanconfigprotectedts/protectedts_table_reader.go, line 124 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Should we make this into a decoder struct similar to what we do for system.zones and system.descriptors? It might be worth doing that because we'll use this thing in the sqlwatcher as well.

As discussed offline, i'll bring this in the PR that sets up the rangefeed in the SQLWatcher, and come back and clean this up then.

pkg/spanconfig/spanconfigprotectedts/protectedts_table_reader.go, line 163 at r2 (raw file):

Can we use the GetState method on protectedts.Storage instead by threading in that interface into here?

Very nice catch. I think we can, the SQLTranslator which will use this reader already has an ExecCfg which in turn has a ptpb.Provider. Changed to do that instead!

pkg/spanconfig/spanconfigprotectedts/protectedts_table_reader_test.go, line 36 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

You could convert this thing into a datadriven test -- it'll allow you to test things like different records that target the same schema object/tenant/cluster easily.

Done.

pkg/spanconfig/spanconfigprotectedts/protectedts_table_reader_test.go, line 72 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Might be worth protecting a tenant in a separate record and ensuring that all tenant protections are returned.

Done.

adityamaru · 2022-01-17T19:23:53Z

Rebased, and RFAL!

arulajmani

Couple of testing suggestions but otherwise

Thanks for putting in the effort to make the tests here datadriven!

Reviewed 35 of 35 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, @irfansharif, and @stevendanna)

pkg/spanconfig/spanconfigsqltranslator/protectedts_table_reader.go, line 39 at r4 (raw file):

	ctx context.Context, txn *kv.Txn, ptsProvider protectedts.Provider,
) (*ProtectedTimestampTableReader, error) {
	reader := &ProtectedTimestampTableReader{

nit: Let's add an assertion failure if the passed in txn is nil?

pkg/spanconfig/spanconfigsqltranslator/testdata/ptstablereader, line 8 at r4 (raw file):


query-sql
SELECT id FROM system.namespace WHERE name='db'

nit: Instead of the queries to system.namespace below, just comment the IDs of these objects here instead.

pkg/spanconfig/spanconfigsqltranslator/testdata/ptstablereader, line 49 at r4 (raw file):

# Write a protected timestamp on tenant 2.
protect id=6 ts=6
tenant 2

Maybe add another on this tenant for completeness?

pkg/spanconfig/spanconfigsqltranslator/testdata/ptstablereader, line 75 at r4 (raw file):

tablereader tenants
----
id=1 [ts=5]

nit: s/id/tenant_id/g (in the datadriven test where such output is printed) considering "id" is overloaded here.

pkg/spanconfig/spanconfigsqltranslator/testdata/ptstablereader, line 89 at r4 (raw file):


# Records on t1.
tablereader schemaObject=56

Let's add a test that releases either id=1 or id=2 as well, to test that records being protected/released don't affect other records that apply to a particular ID.

adityamaru

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @irfansharif, and @stevendanna)

pkg/spanconfig/spanconfigsqltranslator/protectedts_table_reader.go, line 39 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: Let's add an assertion failure if the passed in txn is nil?

Since we're passing in a state now this check happens in GetState() itself.

pkg/spanconfig/spanconfigsqltranslator/testdata/ptstablereader, line 8 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: Instead of the queries to system.namespace below, just comment the IDs of these objects here instead.

done.

pkg/spanconfig/spanconfigsqltranslator/testdata/ptstablereader, line 49 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Maybe add another on this tenant for completeness?

Done.

pkg/spanconfig/spanconfigsqltranslator/testdata/ptstablereader, line 75 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: s/id/tenant_id/g (in the datadriven test where such output is printed) considering "id" is overloaded here.

done.

pkg/spanconfig/spanconfigsqltranslator/testdata/ptstablereader, line 89 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Let's add a test that releases either id=1 or id=2 as well, to test that records being protected/released don't affect other records that apply to a particular ID.

Done.

adityamaru · 2022-01-19T00:10:51Z

@irfansharif changed the state reader to take in a ptpb.State instead as we discussed, let me know what you think.

irfansharif · 2022-01-18T20:16:38Z

pkg/spanconfig/spanconfigsqltranslator/protectedts_table_reader_test.go

+		defer spanConfigTestCluster.Cleanup()
+
+		var tenant *spanconfigtestcluster.Tenant
+		// TODO(adityamaru): When secondary tenants have a "real"


Take the opportunity to move this test code to pkg/ccl/spanconfigccl/...; when using tenant code you need a CCL binary, and need to sit under pkg/ccl.

irfansharif · 2022-01-19T16:53:41Z

pkg/spanconfig/spanconfigsqltranslator/protectedts_state_reader.go

+
+// ProtectedTimestampStateReader provides a target specific view of the
+// protected timestamp records stored in the system table.
+type ProtectedTimestampStateReader struct {


This doesn't need to be exported from this package, right?

doesn't yup, changed.

ahh can't do this because the linter doesn't like it:

exported func TestingNewProtectedTimestampStateReader returns unexported type *spanconfigsqltranslator.protectedTimestampStateReader, which can be annoying to use

Well, you don't need that method either?

yup removed it in the most recent push once i changed the pkg to non _test

irfansharif · 2022-01-19T16:54:34Z

pkg/spanconfig/spanconfigsqltranslator/protectedts_state_reader.go

+
+// TenantProtectedTimestamps represents all the Protections that apply to a
+// tenant's keyspace.
+type TenantProtectedTimestamps struct {


Ditto, we don't expect to be used outside the translator package, do we?

We need it in the testing package when comparing output so had to leave it exported.

It's an internal type, right? Would using a package internal test make more sense? s/package spanconfigsqltranslator_test/package spanconfigsqltranslator. It would let you unexport as well.

pkg/spanconfig/spanconfigsqltranslator/protectedts_state_reader.go

irfansharif · 2022-01-19T17:17:32Z

pkg/spanconfig/spanconfigsqltranslator/protectedts_state_reader_test.go

+	"github.com/stretchr/testify/require"
+)
+
+// TestProtectedTimestampStateReader is a data driven test for the


Discussed a bit offline, but for now while we're just testing this state reader type, and given that it operates off a static ptpb.State snapshot, a simple unit test would suffice. We can graft pieces of this test we've written when hooking it up to the translator as a way to conveniently write to the protected_ts table and ensure the right span configs are generated.

Changed to a unit test, kept some of the stringer methods around for readabilty, and ease of comparison since I already had them written out.

arulajmani

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @arulajmani, @irfansharif, and @stevendanna)

pkg/spanconfig/spanconfigsqltranslator/protectedts_state_reader_test.go, line 47 at r5 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

Changed to a unit test, kept some of the stringer methods around for readabilty, and ease of comparison since I already had them written out.

Not to rehash what we talked about offline, but I'm not a fan of going from the datadriven test we had here before to this unit test. We're reducing coverage at worst, and reducing readability at best. Sure, you can test these things using the Translator in a future PR, but that feels very indirect to me.

In particular, having a data driven test allows us to write multiple records to the same schema object/tenant, records that apply to various schema objects/tenants, and release records. Considering this struct responsible for transforming this state and providing accessors over it, what we had earlier provided value.

irfansharif

This does not feel much less readable to me. This is a pretty small component, and I think it's fine for our testing of it not have to use the full datadriven machinery. We're really only testing loadProtectedTimestampRecords, right? (which is <20 lines of Go code.)

irfansharif · 2022-01-19T21:16:32Z

pkg/spanconfig/spanconfigsqltranslator/protectedts_state_reader.go

+
+// TenantProtectedTimestamps represents all the Protections that apply to a
+// tenant's keyspace.
+type TenantProtectedTimestamps struct {


It's an internal type, right? Would using a package internal test make more sense? s/package spanconfigsqltranslator_test/package spanconfigsqltranslator. It would let you unexport as well.

pkg/spanconfig/spanconfigsqltranslator/protectedts_state_reader_test.go

pkg/spanconfig/spanconfigsqltranslator/protectedts_state_reader.go

This change introduces a `ProtectedTimestampStateReader` that provides an in-memory view of the system table that stores protected timestamp records. The `SQLTranslator` will use this table reader to generate SpanConfigs and SystemSpanConfigs in a follow up PR. Informs: cockroachdb#73727 Release note: None

adityamaru · 2022-01-20T03:02:33Z

bazel ORM failures are unrelated

TFTRs!

bors r+

craig · 2022-01-20T07:47:09Z

Build succeeded:

GitHub CI (Cockroach)

adityamaru requested review from arulajmani, irfansharif and a team January 12, 2022 16:59

adityamaru requested review from a team as code owners January 12, 2022 16:59

adityamaru requested review from stevendanna and removed request for a team January 12, 2022 16:59

adityamaru requested review from a team and removed request for a team January 12, 2022 16:59

This was referenced Jan 12, 2022

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

Closed

spanconfigsqltranslator: populate protected_timestamps in SpanConfig #74803

Merged

arulajmani reviewed Jan 13, 2022

View reviewed changes

adityamaru force-pushed the full-translate branch from 080e7ae to 5318673 Compare January 14, 2022 20:39

adityamaru commented Jan 14, 2022

View reviewed changes

adityamaru changed the title ~~spanconfigprotectedts: introduce a pts table reader~~ spanconfigsqltranslator: introduce a pts table reader Jan 14, 2022

adityamaru force-pushed the full-translate branch from 5318673 to 580a386 Compare January 14, 2022 20:45

adityamaru requested a review from arulajmani January 14, 2022 20:46

adityamaru force-pushed the full-translate branch from 580a386 to 1715deb Compare January 17, 2022 19:23

adityamaru removed the request for review from a team January 17, 2022 19:24

arulajmani approved these changes Jan 18, 2022

View reviewed changes

adityamaru force-pushed the full-translate branch from 1715deb to 1864096 Compare January 19, 2022 00:09

adityamaru commented Jan 19, 2022

View reviewed changes

irfansharif reviewed Jan 19, 2022

View reviewed changes

adityamaru force-pushed the full-translate branch from 1864096 to dd14c03 Compare January 19, 2022 19:23

blathers-crl bot requested a review from irfansharif January 19, 2022 19:23

adityamaru force-pushed the full-translate branch from dd14c03 to 8ac46f1 Compare January 19, 2022 19:25

arulajmani reviewed Jan 19, 2022

View reviewed changes

irfansharif approved these changes Jan 19, 2022

View reviewed changes

adityamaru force-pushed the full-translate branch from 8ac46f1 to f6cfcd8 Compare January 20, 2022 00:07

irfansharif reviewed Jan 20, 2022

View reviewed changes

pkg/spanconfig/spanconfigsqltranslator/protectedts_state_reader.go Outdated Show resolved Hide resolved

adityamaru force-pushed the full-translate branch from f6cfcd8 to 67170ed Compare January 20, 2022 01:38

craig bot merged commit 506412f into cockroachdb:master Jan 20, 2022

adityamaru deleted the full-translate branch January 20, 2022 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spanconfigsqltranslator: introduce a pts table reader #74737

spanconfigsqltranslator: introduce a pts table reader #74737

adityamaru commented Jan 12, 2022

cockroach-teamcity commented Jan 12, 2022

adityamaru commented Jan 12, 2022

arulajmani left a comment

adityamaru left a comment

adityamaru commented Jan 17, 2022

arulajmani left a comment

adityamaru left a comment

adityamaru commented Jan 19, 2022

irfansharif Jan 18, 2022

irfansharif Jan 19, 2022

adityamaru Jan 19, 2022

adityamaru Jan 19, 2022

irfansharif Jan 20, 2022 •

edited

Loading

adityamaru Jan 20, 2022

irfansharif Jan 19, 2022

adityamaru Jan 19, 2022 •

edited

Loading

irfansharif Jan 19, 2022 •

edited

Loading

irfansharif Jan 19, 2022

adityamaru Jan 19, 2022

arulajmani left a comment

irfansharif left a comment

irfansharif Jan 19, 2022 •

edited

Loading

adityamaru commented Jan 20, 2022

craig bot commented Jan 20, 2022

spanconfigsqltranslator: introduce a pts table reader #74737

spanconfigsqltranslator: introduce a pts table reader #74737

Conversation

adityamaru commented Jan 12, 2022

cockroach-teamcity commented Jan 12, 2022

adityamaru commented Jan 12, 2022

arulajmani left a comment

Choose a reason for hiding this comment

adityamaru left a comment

Choose a reason for hiding this comment

adityamaru commented Jan 17, 2022

arulajmani left a comment

Choose a reason for hiding this comment

adityamaru left a comment

Choose a reason for hiding this comment

adityamaru commented Jan 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irfansharif Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityamaru Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

irfansharif Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

irfansharif left a comment

Choose a reason for hiding this comment

irfansharif Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

adityamaru commented Jan 20, 2022

craig bot commented Jan 20, 2022

irfansharif Jan 20, 2022 •

edited

Loading

adityamaru Jan 19, 2022 •

edited

Loading

irfansharif Jan 19, 2022 •

edited

Loading

irfansharif Jan 19, 2022 •

edited

Loading