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: don't panic if reader doesn't exist yet #99318

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

stevendanna
Copy link
Collaborator

@stevendanna stevendanna commented Mar 22, 2023

While shared-process tenant servers are not likely to make requests
before the capability reader exists, the limiter factory looks up the
relevant tenant ID from the start key of the range descriptor and it
isn't unlikely that we'll see requests against tenant ranges before we
have a capability reader available.

Epic: none

Release note: None

Release justification: Low risk fix to avoid panics in tests.

@stevendanna stevendanna requested a review from a team as a code owner March 22, 2023 23:28
@blathers-crl
Copy link

blathers-crl bot commented Mar 22, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

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

@ajstorm ajstorm left a comment

Choose a reason for hiding this comment

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

LGTM

@ecwall
Copy link
Contributor

ecwall commented Mar 23, 2023

This looks fine, but I am worried that these functions are diverging. They are all pretty much just checking a bool(s), but have slight differences already.

Copy link
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

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

Should we add a comment inline as well, in case future readers wonder why this function doesn't panic like the others?

@knz
Copy link
Contributor

knz commented Mar 23, 2023

Maybe update commit message and PR description to replace "in memory tenants" by "shared process tenant servers".

While shared-process tenant servers are not likely to make requests
before the capability reader exists, the limiter factory looks up the
relevant tenant ID from the start key of the range descriptor and it
isn't unlikely that we'll see requests against tenant ranges before we
have a capability reader available.

Epic: none

Release note: None

Release justification: Low risk fix to avoid panics in tests.
@stevendanna
Copy link
Collaborator Author

Comments and commit message updated. Will merge once CI is green.

@ecwall

This looks fine, but I am worried that these functions are diverging. They are all pretty much just checking a bool(s), but have slight differences already.

I kinda wonder if what we want here is a HasCapability(ctx, tenID, cap) and HasCapability(ctx, tenID, cap, authorizer.ReturnDefaultIfUnavailable()). Not going to do that in this PR, but happy to chat about how this should evolve.

@stevendanna
Copy link
Collaborator Author

Test failure is slow quiesce.

bors r=knz,ajstorm,arulajmani

@craig
Copy link
Contributor

craig bot commented Mar 23, 2023

Build succeeded:

@craig craig bot merged commit da20f04 into cockroachdb:master Mar 23, 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.

6 participants