Skip to content

kv: replace SystemSpanConfig with SystemDescriptorTableSpan / SystemZonesTableSpan#82281

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
RichardJCai:remove_system_span_config
Jun 22, 2022
Merged

kv: replace SystemSpanConfig with SystemDescriptorTableSpan / SystemZonesTableSpan#82281
craig[bot] merged 2 commits intocockroachdb:masterfrom
RichardJCai:remove_system_span_config

Conversation

@RichardJCai
Copy link
Copy Markdown
Contributor

@RichardJCai RichardJCai commented Jun 1, 2022 •
edited
Loading

SystemSpanConfig does not have to contain all tables with id <= 10.
It has been replaced with SystemDescriptorTableSpan and SystemZonesTableSpan.
These two tables are the only system tables that are unsplittable.

We cannot get rid of the concept entirely due to SystemSpanConfig components
being relied on for replication reports and opt catalog.

Release note: None

fixes #82178

@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Jun 1, 2022

This change is Reviewable

@RichardJCai RichardJCai force-pushed the remove_system_span_config branch 4 times, most recently from 1f66c22 to c32e1b6 Compare June 6, 2022 17:37
@RichardJCai RichardJCai mentioned this pull request Jun 6, 2022
Merged
@RichardJCai RichardJCai force-pushed the remove_system_span_config branch 2 times, most recently from 4fc94b2 to c679e7b Compare June 8, 2022 20:32
@RichardJCai RichardJCai changed the title kv, gossip: remove deprecated system span config kv: replace SystemSpanConfig with SystemDescriptorTableSpan / SystemZonesTableSpan Jun 8, 2022
@RichardJCai RichardJCai marked this pull request as ready for review June 8, 2022 20:37
@RichardJCai RichardJCai requested review from a team as code owners June 8, 2022 20:37
@RichardJCai RichardJCai requested a review from a team June 8, 2022 20:37
@RichardJCai RichardJCai requested a review from a team as a code owner June 8, 2022 20:37
@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Jun 8, 2022

Is this the right idea? @ajwerner @irfansharif

@RichardJCai RichardJCai force-pushed the remove_system_span_config branch from c679e7b to 921b11c Compare June 8, 2022 20:39
ajwerner
ajwerner reviewed Jun 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the right idea

irfansharif
irfansharif reviewed Jun 9, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to take a closer look at the spanconfigcompared testdata files. Honestly, that package existed to compare the span configs infra against the old thing, the old thing we've now gotten rid of. I'd be happy nuking that entire test package. Wanna do the honours?

@ajwerner
Copy link
Copy Markdown
Contributor

ajwerner commented Jun 9, 2022

Honestly, that package existed to compare the span configs infra against the old thing, the old thing we've now gotten rid of. I'd be happy nuking that entire test package. Wanna do the honours?

I'm not convinced we're ready for that. Recall that the optimizer and the replication reports still derive their expectations of what the span configs will be from the pre-existing logic.

@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Jun 9, 2022

Also anyone have an idea on why TestStoreRangeUpReplicate and TestRangeCount pass locally for me but not in CI?

@RichardJCai RichardJCai force-pushed the remove_system_span_config branch from 921b11c to 99b49ba Compare June 9, 2022 14:45
@blathers-crl blathers-crl bot requested a review from irfansharif June 9, 2022 14:45
@RichardJCai RichardJCai requested a review from a team as a code owner June 14, 2022 18:33
@RichardJCai RichardJCai requested a review from a team June 14, 2022 18:33
@RichardJCai RichardJCai force-pushed the remove_system_span_config branch 2 times, most recently from d2955df to 0f5f4b1 Compare June 14, 2022 21:33
@ajwerner
Copy link
Copy Markdown
Contributor

ajwerner commented Jun 15, 2022

cockroach/pkg/sql/catalog/bootstrap/metadata.go

Lines 184 to 186 in 90054c6

if desc.GetID() > keys.MaxSystemConfigDescID {
splits = append(splits, roachpb.RKey(ms.codec.TablePrefix(uint32(desc.GetID()))))
}

Should we get rid of this condition? It's the reason for the bazel flake, and, I suspect, the need for one of your succeeds soon

irfansharif
irfansharif approved these changes Jun 15, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great stuff!

@RichardJCai RichardJCai force-pushed the remove_system_span_config branch from 89c1071 to 2ed1967 Compare June 15, 2022 22:37
irfansharif
irfansharif reviewed Jun 15, 2022
View reviewed changes
@RichardJCai RichardJCai force-pushed the remove_system_span_config branch 6 times, most recently from 72030da to 8eab857 Compare June 17, 2022 02:17
@RichardJCai
kv: replace SystemSpanConfig with SystemDescriptorTableSpan / SystemZ…
84b294c 
…onesTableSpan

SystemSpanConfig does not have to contain all tables with id <= 10.
It has been replaced with SystemDescriptorTableSpan and SystemZonesTableSpan.
These two tables are the only system tables that are unsplittable.

We cannot get rid of the concept entirely due to SystemSpanConfig components
being relied on for replication reports and opt catalog.

Release note: None
@RichardJCai RichardJCai force-pushed the remove_system_span_config branch 2 times, most recently from dad0528 to 7295cce Compare June 21, 2022 15:41
@RichardJCai RichardJCai force-pushed the remove_system_span_config branch from 7295cce to b86fd4f Compare June 21, 2022 17:09
@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Jun 21, 2022

Alright I think I addressed everything, thanks for the reviews!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 21, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 22, 2022

Build failed (retrying...):

@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Jun 22, 2022

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 22, 2022

Already running a review

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 22, 2022

Build failed:

@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Jun 22, 2022

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 22, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 22, 2022

Build succeeded:

@craig craig bot merged commit 0050edb into cockroachdb:master Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@knz knz knz left review comments

@irfansharif irfansharif irfansharif approved these changes

@ajwerner ajwerner ajwerner approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

keys,*: eliminate the concept of a SystemConfigSpan

5 participants

@RichardJCai @cockroach-teamcity @ajwerner @knz @irfansharif