Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multitenant: add tenant end key split to MetadataSchema #99246

Merged
merged 2 commits into from
Mar 22, 2023
Merged

multitenant: add tenant end key split to MetadataSchema #99246

merged 2 commits into from
Mar 22, 2023

Conversation

ecwall
Copy link
Contributor

@ecwall ecwall commented Mar 22, 2023

Fixes #97985

Previously #95100 created tenant end key split points asynchronously which
allowed for a brief period after tenant creation where the tenant's keyspace
had an end key of /Max instead of the next tenant's start key.

This change creates that split point synchronously on tenant creation to avoid
race conditions.

Release note: None

@ecwall ecwall requested review from knz, rafiss and arulajmani March 22, 2023 16:05
@ecwall ecwall requested review from a team as code owners March 22, 2023 16:05
@ecwall ecwall requested review from a team, herkolategan and smg260 and removed request for a team March 22, 2023 16:05
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ecwall ecwall removed request for a team, herkolategan and smg260 March 22, 2023 16:06
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

nice

@ecwall ecwall added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Mar 22, 2023
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @ecwall)


pkg/sql/catalog/bootstrap/metadata.go line 279 at r1 (raw file):

		records = append(records, record{k: s})
	}
	// Strip the tenant prefix if there is one.

the "if there is one" comment that was here seems to imply that the prefix may or may not be present. after this change, should we assert somehow that the prefix is always present?

Copy link
Contributor Author

@ecwall ecwall left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @rafiss)


pkg/sql/catalog/bootstrap/metadata.go line 279 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

the "if there is one" comment that was here seems to imply that the prefix may or may not be present. after this change, should we assert somehow that the prefix is always present?

The prefix won't be present for the system tenant because it does not have a prefix. I think the comment was indicating that it will strip secondary tenants' prefixes.

@ecwall ecwall requested a review from fqazi March 22, 2023 20:13
ecwall added 2 commits March 22, 2023 16:54
Fixes #97985

Previously #95100 created tenant end key split points asynchronously which
allowed for a brief period after tenant creation where the tenant's keyspace
had an end key of /Max instead of the next tenant's start key.

This change creates that split point synchronously on tenant creation to avoid
race conditions.

Release note: None
This function was added as part of #95100 to block until the tenant's end key
split point was created asynchronously, but is no longer needed because the
tenant end key split point is created synchronously on tenant creation.

Release note: None
Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

First commit looks good!

Reviewed 3 of 5 files at r2, 9 of 9 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @rafiss)

@ecwall
Copy link
Contributor Author

ecwall commented Mar 22, 2023

bors r=knz

@craig
Copy link
Contributor

craig bot commented Mar 22, 2023

Build succeeded:

@craig craig bot merged commit f4b266d into cockroachdb:master Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multitenant: tenant range endKey incorrect on tenant creation
5 participants