spanconfigkvsubscriber: teach the KVSubscriber about system span configs #76942

arulajmani · 2022-02-23T19:26:16Z

See individual commits for details.

Simplify test and unify file name with others in the package. Release note: None

cockroach-teamcity · 2022-02-23T19:26:29Z

This change is

irfansharif · 2022-02-23T19:29:42Z

pkg/spanconfig/spanconfigkvsubscriber/spanconfigdecoder_test.go

+	}
+
+	for i, systemTarget := range []spanconfig.SystemTarget{
+		// Tenant targeting its logical cluster.


Drop the "cluster" verbiage here and throughout.

irfansharif · 2022-02-23T19:36:59Z

pkg/spanconfig/systemtarget.go

+		return keys.EverythingSpan
+	}
+	switch st.systemTargetType {
+	case SystemTargetTypeEntireKeyspace:


Why do we need both this and st.targetsEntireKeyspace above?

irfansharif · 2022-02-23T19:38:20Z

pkg/spanconfig/systemtarget.go

+			EndKey: k.PrefixEnd(),
+		}
+	case SystemTargetTypeAllTenantKeyspaceTargetsSet:
+		panic("read only target")


This panic feels a bit misplaced/confusing. Not apparent that we're doing any writes here in this API.

irfansharif · 2022-02-23T19:41:21Z

pkg/spanconfig/spanconfigkvsubscriber/datadriven_test.go

@@ -46,8 +46,9 @@ import (
 //      delete [c,e)
 //      upsert [c,d):C
 //      upsert [d,e):D
+//      upsert {entire-keyspace}:_CLUSTER


Why the _? Drop the cluster verbiage. The text here is just a shorthand for a config, using any letter in the alphabet instead would avoid confusing readers that these words/underscore have special meaning. Mind doing that?

irfansharif · 2022-02-23T19:41:57Z

pkg/spanconfig/spanconfigkvsubscriber/datadriven_test.go

@@ -243,7 +245,7 @@ func TestDataDriven(t *testing.T) {
 					}

 					var spanStr string
-					if update.Equal(keys.EverythingSpan) {
+					if update.Equal(keys.EverythingSpan) || update.Key.Compare(keys.TenantTableDataMin) > 0 {


Why did we need this?

irfansharif · 2022-02-23T19:43:59Z

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs

+
+store-reader key=e
+----
+conf=D_CLUSTER


Instead of concatenating and making use of _'s, what if this syntax captured exactly what was being appended?

conf=D+X

irfansharif · 2022-02-23T19:46:00Z

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs

+# Update span configs that target tenant keyspaces and ensure handlers are
+# invoked correctly.
+update
+upsert {source=1, target=1}:_HOST


[nit] Here too I'd prefer using single letters instead and showing combination of configs through "A+B+C".

irfansharif · 2022-02-23T19:48:32Z

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs

+upsert {source=10, target=10}:_TEN
+----
+
+# M{in-ax} corresponds to the system tenant targeting itself.


Is this right? I thought the system tenant targeting itself would would only capture its own keyspace without going into the tenant's. Wasn't that the whole point of separating out the EntireKeyspaceTarget into its own thing?

arulajmani

RFAL

Dismissed @irfansharif from 2 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @irfansharif)

pkg/spanconfig/systemtarget.go, line 158 at r3 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Why do we need both this and st.targetsEntireKeyspace above?

Oops, meant to delete the thing above.

pkg/spanconfig/systemtarget.go, line 167 at r3 (raw file):

Previously, irfansharif (irfan sharif) wrote…

This panic feels a bit misplaced/confusing. Not apparent that we're doing any writes here in this API.

Switched to returning an error and swallowing it at the caller -- not sure if it's much better, but maybe it's okay considering we never expect this to happen.

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs, line 42 at r3 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Instead of concatenating and making use of _'s, what if this syntax captured exactly what was being appended?
conf=D+X

I'm not sure how we'd make the syntax capture what's being appended at this layer. For context, the "combining" happens at the level of the store -- the subscriber is agnostic to which PTS field comes from which component (span config, system span config).

Maybe all you're asking for is to switch _CLUSTER to something like +X? If that's the case, I've made the change.

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs, line 52 at r3 (raw file):

Previously, irfansharif (irfan sharif) wrote…

[nit] Here too I'd prefer using single letters instead and showing combination of configs through "A+B+C".

Done.

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs, line 57 at r3 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Is this right? I thought the system tenant targeting itself would would only capture its own keyspace without going into the tenant's. Wasn't that the whole point of separating out the EntireKeyspaceTarget into its own thing?

Switched this to only include system tenant keys.

pkg/spanconfig/spanconfigkvsubscriber/datadriven_test.go, line 49 at r3 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Why the _? Drop the cluster verbiage. The text here is just a shorthand for a config, using any letter in the alphabet instead would avoid confusing readers that these words/underscore have special meaning. Mind doing that?

Done; though in the tests we use things like +X to visually differentiate the output.

pkg/spanconfig/spanconfigkvsubscriber/datadriven_test.go, line 248 at r3 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Why did we need this?

Switched this around a bit; we need some special pretty printing for spans not constructed using our re. I moved it to PrintSpan though.

pkg/spanconfig/spanconfigkvsubscriber/spanconfigdecoder_test.go, line 98 at r3 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Drop the "cluster" verbiage here and throughout.

Done.

irfansharif · 2022-02-24T18:15:14Z

pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go

+				var err error
+				sp, err = target.GetSystemTarget().KeyspaceTargeted()
+				if err != nil {
+					// Swallow the error here, even though we never expect this to happen.


This is still a bit odd, and tightly coupled to internals of KeyspaceTargeted. I think you want a check at this level that the SystemTargetType isn't the odd one you're not expecting and fatal-ing if it is. With that, KeyspaceTargeted could panic("unimplemented") or panic("not applicable"). I should've been clearer earlier, I was just thrown off by the "read only target" message in the panic.

irfansharif · 2022-02-24T18:16:40Z

pkg/spanconfig/spanconfigkvsubscriber/spanconfigdecoder_test.go

+
+		require.Equal(t, conf, got.Config)
+		require.True(t, got.Target.IsSystemTarget())
+		require.Equal(t, systemTarget, got.Target.GetSystemTarget(), "i:%d", i)


Is "i:%d", i) debugging detritus?

irfansharif · 2022-02-24T18:17:32Z

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs

+# The system span config we set should still apply.
+store-reader key=g
+----
+conf=MISSING+X


[nit] s/MISSING/FALLBACK

irfansharif · 2022-02-24T18:20:34Z

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs

+----
+conf=MISSING+X
+
+# Update span configs that target tenant keyspaces and ensure handlers are


Move everything below this line into a separate file; the spans don't overlap with the special [alphabet, alphabet) style spans listed above, except for {source=1, target=1}: which could stay in this file.

irfansharif · 2022-02-24T18:24:50Z

pkg/spanconfig/spanconfigtestutils/utils.go

+		sp.Key.String(),
+		sp.EndKey.String(),
+	}
+	for i := range s {


This is breaking symmetry with ParseSpan, worth at least updating the comment above. Typo below "qe".

irfansharif · 2022-02-24T18:47:41Z

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs

+
+store-reader key=a
+----
+conf=A+Y


Just looked into the earlier PR that took character tags and int-casted them into protection policies. To me that at least smells weird, I don't know. If all we cared about was unique tags, it's fine to tag them using integers. When printing the configs, instead of this string concat (we're having to use tags prefixed with "+X", which feels odd), we could simply print a list of protection policies in PrintSpanConfig with "+", iterating through each PTS.

irfansharif

I'll rubber stamp because it's a testing only PR, but I think we should make the test changes I'm suggesting.

This worked but didn't have a test previously. Release note: None

This patch teaches the KVSubscriber to handle updates to system span configurations. There's not much here, other than invoking registered handlers with the correct span when a system span config is updated. This entails: - The everything span when the system span config targets the entire keyspace or targets the host tenant. - The targeted tenant's span if the system span config targets a secondary tenant. Release note: None

arulajmani

I reverted the patch I had to fix the bug in how we invoke our handlers; it turns out there's more we need to do there and I'll pull it out into its own PR.

Dismissed @irfansharif from 4 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @irfansharif)

pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go, line 263 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

This is still a bit odd, and tightly coupled to internals of KeyspaceTargeted. I think you want a check at this level that the SystemTargetType isn't the odd one you're not expecting and fatal-ing if it is. With that, KeyspaceTargeted could panic("unimplemented") or panic("not applicable"). I should've been clearer earlier, I was just thrown off by the "read only target" message in the panic.

Done.

pkg/spanconfig/spanconfigtestutils/utils.go, line 254 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

This is breaking symmetry with ParseSpan, worth at least updating the comment above. Typo below "qe".

Done.

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs, line 47 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

[nit] s/MISSING/FALLBACK

Done.

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs, line 49 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Move everything below this line into a separate file; the spans don't overlap with the special [alphabet, alphabet) style spans listed above, except for {source=1, target=1}: which could stay in this file.

Done.

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs, line 85 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Just looked into the earlier PR that took character tags and int-casted them into protection policies. To me that at least smells weird, I don't know. If all we cared about was unique tags, it's fine to tag them using integers. When printing the configs, instead of this string concat (we're having to use tags prefixed with "+X", which feels odd), we could simply print a list of protection policies in PrintSpanConfig with "+", iterating through each PTS.

The construction here allows us to store words, not just characters to tag a config. I quite like how we can use this to do neat tricks like call a tagged config FALLBACK; I'm ambivalent to to this "+X" / int cast given that. I won't hold up the merging this PR over this, considering it's a nitty detail, but happy to retroactively fix this representation.

pkg/spanconfig/spanconfigkvsubscriber/spanconfigdecoder_test.go, line 154 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Is "i:%d", i) debugging detritus?

Yeah, removed.

arulajmani · 2022-02-24T21:01:46Z

Also added a test for KeyspaceTargeted like we talked about offline.

arulajmani

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

pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs, line 85 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

The construction here allows us to store words, not just characters to tag a config. I quite like how we can use this to do neat tricks like call a tagged config FALLBACK; I'm ambivalent to to this "+X" / int cast given that. I won't hold up the merging this PR over this, considering it's a nitty detail, but happy to retroactively fix this representation.

Tacked on 4th commit on here to get around using the "+X" style tags; we now instead require tags to be a single character, or, "FALLBACK" which we special case for some readability benefits. I like this approach over the switching to integers to tag configs as that'd require unnecessary test case churn. Or maybe I'm just lazy and too used to seeing letter tags :P

Previously, we could tag configs using arbitrary words. For tests that used both span configs and system span configs, we'd end up needing to put visual delimiters when setting up the test ourselves. This patch makes it such that we can only tag configs using a single character. We then render delimiters ourselves when printing "combined" configs. For readability, we still allow setting a config called "FALLBACK". This was the only reason words, as opposed to chars, were used to tag configs. Release note: None

arulajmani · 2022-02-24T23:35:14Z

TFTR!

bors r+

craig · 2022-02-25T01:21:50Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-02-25T12:31:20Z

Build succeeded:

GitHub CI (Cockroach)

spanconfigkvsubscriber: improve test

f877e66

Simplify test and unify file name with others in the package. Release note: None

arulajmani requested review from irfansharif and adityamaru February 23, 2022 19:26

arulajmani requested a review from a team as a code owner February 23, 2022 19:26

arulajmani mentioned this pull request Feb 23, 2022

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

Closed

23 tasks

irfansharif reviewed Feb 23, 2022

View reviewed changes

arulajmani force-pushed the protectedts-kv-subscriber branch from da01fdb to 9b6898e Compare February 24, 2022 16:43

blathers-crl bot requested a review from irfansharif February 24, 2022 16:43

arulajmani commented Feb 24, 2022

View reviewed changes

arulajmani force-pushed the protectedts-kv-subscriber branch from 9b6898e to b83e5b3 Compare February 24, 2022 17:50

irfansharif reviewed Feb 24, 2022

View reviewed changes

irfansharif approved these changes Feb 24, 2022

View reviewed changes

arulajmani added 2 commits February 24, 2022 15:50

spanconfigkvsubscriber: add test for decoding system span config targets

71afae0

This worked but didn't have a test previously. Release note: None

arulajmani force-pushed the protectedts-kv-subscriber branch from b83e5b3 to 3a6a253 Compare February 24, 2022 20:59

arulajmani commented Feb 24, 2022

View reviewed changes

arulajmani force-pushed the protectedts-kv-subscriber branch from 4dca05a to f6cb1d1 Compare February 24, 2022 22:24

arulajmani force-pushed the protectedts-kv-subscriber branch from f6cb1d1 to cc87693 Compare February 24, 2022 23:33

craig bot merged commit 95b0430 into cockroachdb:master Feb 25, 2022

tbg mentioned this pull request Mar 24, 2022

perf: kv0/kv95 regression around Feb 25 [loosely coupled log truncation] #78412

Open

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spanconfigkvsubscriber: teach the KVSubscriber about system span configs #76942

spanconfigkvsubscriber: teach the KVSubscriber about system span configs #76942

arulajmani commented Feb 23, 2022

cockroach-teamcity commented Feb 23, 2022

irfansharif Feb 23, 2022

irfansharif Feb 23, 2022

irfansharif Feb 23, 2022

irfansharif Feb 23, 2022

irfansharif Feb 23, 2022

irfansharif Feb 23, 2022

irfansharif Feb 23, 2022

irfansharif Feb 23, 2022

arulajmani left a comment

irfansharif Feb 24, 2022

irfansharif Feb 24, 2022

irfansharif Feb 24, 2022

irfansharif Feb 24, 2022

irfansharif Feb 24, 2022

irfansharif Feb 24, 2022

irfansharif left a comment

arulajmani left a comment

arulajmani commented Feb 24, 2022

arulajmani left a comment

arulajmani commented Feb 24, 2022

craig bot commented Feb 25, 2022

craig bot commented Feb 25, 2022

spanconfigkvsubscriber: teach the KVSubscriber about system span configs #76942

spanconfigkvsubscriber: teach the KVSubscriber about system span configs #76942

Conversation

arulajmani commented Feb 23, 2022

cockroach-teamcity commented Feb 23, 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

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

arulajmani left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irfansharif left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

arulajmani commented Feb 24, 2022

arulajmani left a comment

Choose a reason for hiding this comment

arulajmani commented Feb 24, 2022

craig bot commented Feb 25, 2022

craig bot commented Feb 25, 2022