spanconfig: replace roachpb.SpanConfigEntry in package spanconfig #76213

arulajmani · 2022-02-08T00:09:02Z

See individual commits for details.

Polished up and carved out from #76138.

In the next patch, we'll modify the existing span configuration RPC arguments to something similar in the draft PR above. Then, we'll make spanconfig.Target a union over system targets and spans (instead of the alias for roachpb.Spans as in this PR). We'll also add all the encoding and decoding methods for system targets that we've seen in prior draft patches.

cockroach-teamcity · 2022-02-08T00:09:11Z

This change is

irfansharif

These new types look great! I think framing the changes to this package in terms of spanconfig.{Target,Record} and in the future generalizing spanconfig.Target to also encompass the "special" targets is very clear IMO -- I dig it. I've left mostly minor comments below, with a recommendation that we abandon the third commit. I also have a question about the continued existence of roachpb.SpanConfigEntry, but nothing worth holding this PR on.

pkg/ccl/kvccl/kvtenantccl/connector.go

irfansharif · 2022-02-08T16:38:29Z

pkg/ccl/kvccl/kvtenantccl/connector.go

 ) error {
+	spansToDelete := make([]roachpb.Span, 0, len(toDelete))


Moving this conversions from []spanconfig.Target to []roachpb.Span and []spanconfig.Record to []roachpb.SpanConfigEntry into stand-alone helpers would help. Maybe even for going the other way? Can be used in pkg/server/node.go. Also I think a fair bit of code will be simplified if we had constructors to go between the individual roachpb.SpanConfigEntry and spanconfig.Record types, and spanconfig.Target and roachpb.Span types (I see we already have spanconfig.MakeSpanTarget, which is exactly the kind of thing we want, just applying that pattern to more types this PR is introducing).

All that said, I think the need for it points to something deeper: How are you imagining the roachpb.UpdateSpanConfigsRequest would look like in the future? Would it continue to be in terms of roachpb.SpanConfigEntry? Something else?

All that said, I think the need for it points to something deeper: How are you imagining the roachpb.UpdateSpanConfigsRequest would look like in the future? Would it continue to be in terms of roachpb.SpanConfigEntry? Something else?

I still think it'll be in terms of roachpb.SpanConfigEntry, but that type itself will change to accommodate this target concept. Check out #76219 for how that type will look.

Also I think a fair bit of code will be simplified if we had constructors to go between the individual roachpb.SpanConfigEntry and spanconfig.Record types, and spanconfig.Target and roachpb.Span types (I see we already have spanconfig.MakeSpanTarget, which is exactly the kind of thing we want, just applying that pattern to more types this PR is introducing).

I actually add exactly this in a later commit (check out the PR above)! I'm deferring the span -> Target and Target -> span changes to the PR above as we'll change the request proto there.

pkg/ccl/migrationccl/migrationsccl/seed_tenant_span_configs_external_test.go

pkg/spanconfig/spanconfigtestutils/recorder.go

pkg/spanconfig/spanconfigstore/store_test.go

pkg/spanconfig/spanconfigstore/store.go

irfansharif · 2022-02-08T19:05:25Z

pkg/spanconfig/spanconfigstore/spanconfigstore.go

+}
+
+// copy returns a copy of the spanConfigStore.
+func (s *spanConfigStore) copy(ctx context.Context) *spanConfigStore {


I'm not going to hold up the merge over it, but I don't think this commit is a positive change. Thanks for separating it out into a stand-alone commit -- I think we should drop it. We have three variants now, and one in the future.

spanconfig.Store;

spanconfigstore.Store;

spanconfigstore.spanConfigStore;

(in the future) spanconfigstore.systemSpanConfigStore

This kind of spread, if anything, makes it difficult to talk about things meaninfully. Looking at the tests, I don't think the separation is also helping much. I understand the intent here was to make room for the stand-alone spanconfigstore.systemSpanConfigStore, but that too I don't think needs to be a wholly distinct type when instead it could be a set of fields on spanconfigstore.Store and appropriately named methods. The interface we care about for all the three types are spanconfig.StoreWriter (with it's diff-ing contract), primarily, and I think it's possible to test each component by interfacing through just the StoreWriter interface. Want to test spanconfigstore.systemSpanConfigStore? Just populate using the right kind of span.Targets. What to test spanconfigstore.systemSpanConfigStore? Do the same thing. Want to test the top-level spanconfigstore.Store? Again, just do the same thing.

Generally IMO when it's possible to test everything we care about through the high-level interface (which is the case here), I think we should. Peeking into the internals instead makes for test code that's going to be subject to more change over time than necessary.

Thanks for separating it out into a stand-alone commit -- I think we should drop it.

Capturing some of our offline conversations, having an internal struct that works on spans and keeps track of spans <-> span config mappings is a nice separation. It allows us to test behaviour that pertains to this type at the right level such as the randomized test that we have. That isn't to say most of our testing won't be at the interface level, which is the way it is today.

This separation into internal types also reduces what the spanconfigstore.Store has to worry about -- in the final construction, it'll only worry about disambiguating updates and hydrating configurations, which is kinda neat. This mostly comes from a preference for not having it "do too much".

Thanks for not holding up the patch for this. Like I mentioned in our conversation about this offline when you asked me to pull this into its own commit, I'm more than happy to revisit this once everything has landed.

arulajmani

TFTR! Will bors on green.

edit -- missed a few comments above because they were over-written in a subsequent commit. Will merge on green and once those are resolved.

pkg/ccl/kvccl/kvtenantccl/connector.go

pkg/ccl/migrationccl/migrationsccl/seed_tenant_span_configs_external_test.go

pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go

pkg/spanconfig/spanconfigtestutils/recorder.go

pkg/spanconfig/target.go

arulajmani · 2022-02-08T22:58:35Z

pkg/ccl/kvccl/kvtenantccl/connector.go

 ) error {
+	spansToDelete := make([]roachpb.Span, 0, len(toDelete))


All that said, I think the need for it points to something deeper: How are you imagining the roachpb.UpdateSpanConfigsRequest would look like in the future? Would it continue to be in terms of roachpb.SpanConfigEntry? Something else?

I still think it'll be in terms of roachpb.SpanConfigEntry, but that type itself will change to accommodate this target concept. Check out #76219 for how that type will look.

Also I think a fair bit of code will be simplified if we had constructors to go between the individual roachpb.SpanConfigEntry and spanconfig.Record types, and spanconfig.Target and roachpb.Span types (I see we already have spanconfig.MakeSpanTarget, which is exactly the kind of thing we want, just applying that pattern to more types this PR is introducing).

I actually add exactly this in a later commit (check out the PR above)! I'm deferring the span -> Target and Target -> span changes to the PR above as we'll change the request proto there.

pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go

arulajmani · 2022-02-08T23:17:39Z

pkg/spanconfig/spanconfigstore/spanconfigstore.go

+}
+
+// copy returns a copy of the spanConfigStore.
+func (s *spanConfigStore) copy(ctx context.Context) *spanConfigStore {


Thanks for separating it out into a stand-alone commit -- I think we should drop it.

Capturing some of our offline conversations, having an internal struct that works on spans and keeps track of spans <-> span config mappings is a nice separation. It allows us to test behaviour that pertains to this type at the right level such as the randomized test that we have. That isn't to say most of our testing won't be at the interface level, which is the way it is today.

This separation into internal types also reduces what the spanconfigstore.Store has to worry about -- in the final construction, it'll only worry about disambiguating updates and hydrating configurations, which is kinda neat. This mostly comes from a preference for not having it "do too much".

Thanks for not holding up the patch for this. Like I mentioned in our conversation about this offline when you asked me to pull this into its own commit, I'm more than happy to revisit this once everything has landed.

arulajmani · 2022-02-09T02:18:55Z

It's green! TFTR, and thanks for brainstorming a bunch of this stuff to get to this point. I echo your sentiments around the new types!

bors r+

craig · 2022-02-09T08:17:29Z

Merge conflict.

Revert the RPCs we added for system span configurations; we'll instead modify the existing RPCs to deal with system span configurations in a future patch. This reverts commit 5177417. Release note (<category, see below>): <what> <show> <why>

This patch removes the usage of roachpb.SpanConfigEntry from the spanconfig package. Going forward, we'll only use roachpb.SpanConfigEntry in RPCs. We want to decouple types that we use inside the package with RPCs in preperation for system span configurations. We instead introduce a new spanconfig.Record type to tie together a spanconfig.Target and a spanconfig.Record. For now, we only allow targeting spans. Once we introduce system span configurations, we'll make be able to make room for system targets as well. System targets will allow tenants to target their entire keyspace and the host tenant to target particular secondary tenants/the entire cluster. Crucially, spanconfig.Target contains encoding and decoding methods which we make use of when writing to and reading from system.span_configurations. Ripping out roachpb.SpanConfigEntry ended up touching most components in the package. Release note: None

This commit pulls out what used to be the spanconfig.Store logic into as separate spanconfigstore.spanConfigStore struct to exclusively track span <-> config pairs. This also makes room for adding another internal struct to track system span configs in a future patch. Release note: None

We'll instead use roachpb.SpanConfig to represent system span configs as well; in the future, we'll just wrap it in a go type an do some validation. Release note: None

arulajmani · 2022-02-09T15:07:35Z

bors r+

craig · 2022-02-09T16:05:34Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-02-09T18:16:27Z

Build succeeded:

GitHub CI (Cockroach)

arulajmani requested review from ajwerner, nvanbenschoten, irfansharif and adityamaru February 8, 2022 00:09

arulajmani requested a review from a team as a code owner February 8, 2022 00:09

arulajmani requested a review from a team February 8, 2022 00:09

arulajmani force-pushed the protectedts-revert branch from 3dc2c0a to 6509ace Compare February 8, 2022 03:14

arulajmani mentioned this pull request Feb 8, 2022

spanconfig: modify existing RPCs to work with system span configurations #76219

Merged

arulajmani force-pushed the protectedts-revert branch 4 times, most recently from 343cf2b to 0052d3b Compare February 8, 2022 19:08

irfansharif approved these changes Feb 8, 2022

View reviewed changes

arulajmani force-pushed the protectedts-revert branch from 0052d3b to d58c31e Compare February 9, 2022 00:05

arulajmani commented Feb 9, 2022

View reviewed changes

arulajmani force-pushed the protectedts-revert branch 3 times, most recently from a1d4538 to f4acab8 Compare February 9, 2022 01:11

arulajmani added 4 commits February 9, 2022 10:00

roachpb: remove system span config protos

f38d556

We'll instead use roachpb.SpanConfig to represent system span configs as well; in the future, we'll just wrap it in a go type an do some validation. Release note: None

arulajmani force-pushed the protectedts-revert branch from f4acab8 to f38d556 Compare February 9, 2022 15:07

craig bot merged commit 90629ae into cockroachdb:master Feb 9, 2022

cockroach-teamcity mentioned this pull request Feb 9, 2022

spanconfig: replace roachpb.SpanConfigEntry in package spanconfig cockroachdb/docs#12989

Closed

arulajmani mentioned this pull request Feb 11, 2022

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

Closed

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spanconfig: replace roachpb.SpanConfigEntry in package spanconfig #76213

spanconfig: replace roachpb.SpanConfigEntry in package spanconfig #76213

arulajmani commented Feb 8, 2022

cockroach-teamcity commented Feb 8, 2022

irfansharif left a comment

irfansharif Feb 8, 2022

arulajmani Feb 8, 2022

irfansharif Feb 8, 2022

arulajmani Feb 8, 2022

arulajmani left a comment •

edited

Loading

arulajmani Feb 8, 2022

arulajmani Feb 8, 2022

arulajmani commented Feb 9, 2022

craig bot commented Feb 9, 2022

arulajmani commented Feb 9, 2022

craig bot commented Feb 9, 2022

craig bot commented Feb 9, 2022

		) error {
		spansToDelete := make([]roachpb.Span, 0, len(toDelete))

spanconfig: replace roachpb.SpanConfigEntry in package spanconfig #76213

spanconfig: replace roachpb.SpanConfigEntry in package spanconfig #76213

Conversation

arulajmani commented Feb 8, 2022

cockroach-teamcity commented Feb 8, 2022

irfansharif left a comment

Choose a reason for hiding this comment

irfansharif Feb 8, 2022

Choose a reason for hiding this comment

arulajmani Feb 8, 2022

Choose a reason for hiding this comment

irfansharif Feb 8, 2022

Choose a reason for hiding this comment

arulajmani Feb 8, 2022

Choose a reason for hiding this comment

arulajmani left a comment • edited Loading

Choose a reason for hiding this comment

arulajmani Feb 8, 2022

Choose a reason for hiding this comment

arulajmani Feb 8, 2022

Choose a reason for hiding this comment

arulajmani commented Feb 9, 2022

craig bot commented Feb 9, 2022

arulajmani commented Feb 9, 2022

craig bot commented Feb 9, 2022

craig bot commented Feb 9, 2022

arulajmani left a comment •

edited

Loading