systemspanconfig: introduce Target and encoding/decoding methods #75641

arulajmani · 2022-01-28T04:07:11Z

This patch introduces a new systemspanconfig.Target type which is
intended as in interemediate representation for system span config
targets throughout the spanconfig package. It ties together a
targeter tenant ID with a targetee tenant ID (along with validation
to ensure secondary tenants don't target other tenants).

We also reserve a SystemSpanConfigSpan at the end of the System
range. This allows us to assign special meaning to key-prefixes carved
out of this range (when stored in system.span_configurations).
By ensuring this span is carved at the end of the system range, we can
ensure that the host tenant does not install span configurations to this
span directly.

We define special key prefixes for system span configurations in this
range and establish a mapping between a Target <-> span using Encode
and Decode methods. This is useful when we store sysytem span
configurations in system.span_configurations given its schema. Ditto
for reading from the table.

We'll make use of the Target in upcoming PRs, specifically in the
KVAccessor, KVSubscriber, and (the upcoming)
SystemSpanConfigStore.

The KVAccessor will construct Targets using tenant information from
the context as the targeter tenant. It'll use the
roachpb.SystemSpanConfigTargets supplied to it by RPCs (See #75615).
The encoding/decoding methods will be used when writing to/reading from
system.span_configurations.

While the existing roachpb.SystemSpanConfigTarget is sufficient
for our use in the KVAccessor (which runs on SQL pods), the richer
structure is motivated by the desire to also use this type in the
KVSubscriber (when receiving system span config events) and
SystemSpanConfigStore (which is an in-memory representation of the
system span config state). Down in KV we want to distinguish between a
tenant installed system span config on its entire keyspace vs. a host
tenant installed system span config over a tenants keyspace.

Release note: None

cockroach-teamcity · 2022-01-28T04:07:18Z

This change is

arulajmani · 2022-01-28T18:47:47Z

@irfansharif and I have been talking a bit more about how we can change some of the types/protos in the span config package to reduce the number of types exposed outside the package while still maintaining explicit internal types for SystemSpanConfigs and SystemSpanConfigTargets. The general idea would be to introduce union types over {roachpb.SpanConfig, roachpb.SystemSpanConfig} and {roachpb.SystemSpanConfigTarget, roachpb.Span}. The hope is with this change we'd reduce the number of RPCs on the KVAccessor and make (what the RFC called) the SystemSpanConfigStore an internal struct as opposed to a public interface.

I plan on doing this in a separate patch, but if we go this route, roachpb.SystemSpanConfigTarget would end up looking very similar to the systemspanconfig.Target, and it would subsume the encoding/decoding methods this PR adds.

We also agreed that it would make sense to review the business logic of the KVAccessor and Target encoding/decoding separate to the type change. The hope is it'll make review iterations faster + targeted (for both me and the reviewers); however it's worth keeping in mind that some of this stuff might shift in the near term.

adityamaru · 2022-01-31T23:03:22Z

pkg/spanconfig/systemspanconfig/target.go

+type Target struct {
+	// TargeterTenantID is the ID of the tenant that specified the system span
+	// configuration.
+	TargeterTenantID roachpb.TenantID


We had discussed considering SourceTenantID and TargetTenantID to be more intuitive names for these fields. I'm okay leaving things as they are since the comments are explanatory but wanted to check if you had considered that?

adityamaru · 2022-01-31T23:05:12Z

pkg/spanconfig/systemspanconfig/target.go

+		targeteeTenantID = targeterTenantID
+	}
+
+	t := Target{


does it make sense to just call return MakeTarget at this point?

adityamaru · 2022-01-31T23:22:24Z

pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go

@@ -218,7 +222,7 @@ func (s *SQLTranslator) generateSpanConfigurationsForNamedZone(
 		})
 		spans = append(spans, roachpb.Span{
 			Key:    keys.TimeseriesSpan.EndKey,
-			EndKey: keys.SystemMax,
+			EndKey: keys.SystemSpanConfigSpan.Key,


discussed briefly, but does it make sense to include keys.SystemConfigSpan.EndKey to keys.SystemMax just incase we ever have any keys in that hole?

adityamaru · 2022-01-31T23:30:56Z

pkg/spanconfig/spanconfigreconciler/reconciler.go

 			Key:    keys.EverythingSpan.Key,
+			EndKey: keys.SystemSpanConfigSpan.Key,
+		})
+		spans = append(spans, roachpb.Span{


Maybe add a comment here that we intentionally elide the SystemSpanConfigSpan. My hunch is that this will change once we build out the SystemSpanConfigStore etc?

This patch introduces a new `systemspanconfig.Target` type which is intended as in interemediate representation for system span config targets throughout the `spanconfig` package. It ties together a targeter tenant ID with a targetee tenant ID (along with validation to ensure secondary tenants don't target other tenants). We also reserve a `SystemSpanConfigSpan` at the end of the System range. This allows us to assign special meaning to key-prefixes carved out of this range (when stored in `system.span_configurations`). By ensuring this span is carved at the end of the system range, we can ensure that the host tenant does not install span configurations to this span directly. We define special key prefixes for system span configurations in this range and establish a mapping between a Target <-> span using `Encode` and `Decode` methods. This is useful when we store sysytem span configurations in `system.span_configurations` given its schema. Ditto for reading from the table. We'll make use of the `Target` in upcoming PRs, specifically in the `KVAccessor`, `KVSubscriber`, and (the upcoming) `SystemSpanConfigStore`. The `KVAccessor` will construct `Targets` using tenant information from the context as the targeter tenant. It'll use the `roachpb.SystemSpanConfigTargets` supplied to it by RPCs (See cockroachdb#75615). The encoding/decoding methods will be used when writing to/reading from `system.span_configurations`. While the existing `roachpb.SystemSpanConfigTarget` is sufficient for our use in the `KVAccessor` (which runs on SQL pods), the richer structure is motivated by the desire to also use this type in the `KVSubscriber` (when receiving system span config events) and `SystemSpanConfigStore` (which is an in-memory representation of the system span config state). Down in KV we want to distinguish between a tenant installed system span config on its entire keyspace vs. a host tenant installed system span config over a tenants keyspace. Release note: None

arulajmani

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

pkg/spanconfig/spanconfigreconciler/reconciler.go, line 285 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

Maybe add a comment here that we intentionally elide the SystemSpanConfigSpan. My hunch is that this will change once we build out the SystemSpanConfigStore etc?

I don't think this will change even when we build out the SystemSpanConfigStore because we never want to install/ get configs at these keyspans directly; KV stores things here by translating SystemSpanConfigTargets, but at the layer of the reconciler, we shouldn't be doing that translation.

I'll add a comment to this effect though.

pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 225 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

discussed briefly, but does it make sense to include keys.SystemConfigSpan.EndKey to keys.SystemMax just incase we ever have any keys in that hole?

Talked offline, but considering the SystemSpanConfigSpan is at the end of the System keyrange, and anything that comes between SystemSpanConfigSpan.EndKey and `SystemMax' will still be prefixed by this span, I think it's fair to say we'll never have any keys in that hole.

pkg/spanconfig/systemspanconfig/target.go, line 24 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

We had discussed considering SourceTenantID and TargetTenantID to be more intuitive names for these fields. I'm okay leaving things as they are since the comments are explanatory but wanted to check if you had considered that?

Switched!

pkg/spanconfig/systemspanconfig/target.go, line 56 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

does it make sense to just call return MakeTarget at this point?

Done.

irfansharif · 2022-02-01T15:48:51Z

Thanks for capturing the discussion on future typing changes we're looking to explore. That only came up after having looked at this PR and it's relation to the protos we recently introduced in #74765, the motivation being that we're 2x-ing the number of symbols for a lot of interfaces in the package -- one to deal with the SystemConfigSpan{,Target,Entry} types and one for just the SpanConfig{,Entry} type. We ought to consider this more fully I think, and now's a great time to do it. The affected interfaces/impls IIUC are:

one of spanconfig.Translator or spanconfig.Reconciler
spanconfig.KVAccessor (already being worked on in spanconfigkvaccessor: implement system span config RPCS #75769)
spanconfig.KVSubscriber
spanconfig.Store (or like the RFC puts forth, a systemspanconfig.Store)

Given we're open to unifying the typing story, I would encourage you to work through the implications discussed here first: #75641 (comment). I suspect it:

has implications for how these things are going to be encoded in system.span_configurations, something this PR concerns itself with;
has implications on how these types would end up looking, also something this PR does;
would make reviewing spanconfigkvaccessor: implement system span config RPCS #75769 and this PR much easier if we made that effort up front, without hopefully adding much iteration time.

That said, if the end-state is clearer to others, and therefore feel more comfortable with this PR's incremental step, I won't hold up the merge train.

adityamaru

Sans the typing discussions above this looks good to me. I'll leave it up to you on how these PRs should be ordered.

arulajmani · 2022-02-09T22:31:58Z

Closing this in favour of #76219

arulajmani requested review from ajwerner, nvanbenschoten, irfansharif and adityamaru January 28, 2022 04:07

arulajmani requested a review from a team as a code owner January 28, 2022 04:07

arulajmani marked this pull request as draft January 28, 2022 19:55

arulajmani force-pushed the protectedts-systemspanconfig-encoder-decoder branch from 5012f9b to 4947d17 Compare January 28, 2022 21:19

arulajmani marked this pull request as ready for review January 28, 2022 21:19

arulajmani force-pushed the protectedts-systemspanconfig-encoder-decoder branch from 4947d17 to f987775 Compare January 28, 2022 22:33

adityamaru reviewed Feb 1, 2022

View reviewed changes

arulajmani mentioned this pull request Feb 1, 2022

spanconfigkvaccessor: implement system span config RPCS #75769

Closed

arulajmani force-pushed the protectedts-systemspanconfig-encoder-decoder branch from f987775 to 63b4f2f Compare February 1, 2022 15:04

arulajmani commented Feb 1, 2022

View reviewed changes

arulajmani requested a review from adityamaru February 1, 2022 15:05

arulajmani mentioned this pull request Feb 1, 2022

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

Closed

23 tasks

adityamaru approved these changes Feb 2, 2022

View reviewed changes

arulajmani closed this Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

systemspanconfig: introduce Target and encoding/decoding methods #75641

systemspanconfig: introduce Target and encoding/decoding methods #75641

arulajmani commented Jan 28, 2022 •

edited

Loading

cockroach-teamcity commented Jan 28, 2022

arulajmani commented Jan 28, 2022

adityamaru Jan 31, 2022

adityamaru Jan 31, 2022

adityamaru Jan 31, 2022

adityamaru Jan 31, 2022

arulajmani left a comment

irfansharif commented Feb 1, 2022

adityamaru left a comment

arulajmani commented Feb 9, 2022

systemspanconfig: introduce Target and encoding/decoding methods #75641

systemspanconfig: introduce Target and encoding/decoding methods #75641

Conversation

arulajmani commented Jan 28, 2022 • edited Loading

cockroach-teamcity commented Jan 28, 2022

arulajmani commented Jan 28, 2022

adityamaru Jan 31, 2022

Choose a reason for hiding this comment

adityamaru Jan 31, 2022

Choose a reason for hiding this comment

adityamaru Jan 31, 2022

Choose a reason for hiding this comment

adityamaru Jan 31, 2022

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

irfansharif commented Feb 1, 2022

adityamaru left a comment

Choose a reason for hiding this comment

arulajmani commented Feb 9, 2022

arulajmani commented Jan 28, 2022 •

edited

Loading