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

multi-tenant: Small cleanup of tenant testing #86877

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

ajstorm
Copy link
Collaborator

@ajstorm ajstorm commented Aug 25, 2022

This commit addresses two small problems with tenant testing:

  • We were mistakenly not passing along the testing knobs from the server to the
    tenant.
  • We were printing out the "this test is using a tenant" message before we were
    guaranteed to be using a tenant.

To resolve these issues we now:

  • Pass along the testing knobs from the server to the tenant.
  • Print out the tenant message only when we're guranteed to be running the test
    in a tenant.

Release justification: Testing only change
Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajstorm ajstorm force-pushed the ajstorm-tenant-test-knobs branch 2 times, most recently from a28653f to acbbffe Compare August 26, 2022 20:04
@ajstorm ajstorm force-pushed the ajstorm-tenant-test-knobs branch 4 times, most recently from 4ac7c27 to 6641964 Compare September 23, 2022 20:45
@ajstorm ajstorm marked this pull request as ready for review September 23, 2022 20:46
@ajstorm ajstorm requested review from a team as code owners September 23, 2022 20:46
@ajstorm ajstorm requested review from srosenberg, knz and irfansharif and removed request for a team and srosenberg September 23, 2022 20:46
@ajstorm ajstorm marked this pull request as draft September 23, 2022 20:49
@ajstorm
Copy link
Collaborator Author

ajstorm commented Sep 23, 2022

Hold off on reviewing for now. Looks like someone else beat me to nearly the exact same change.

This commit addresses two problems with tenant testing:

- We were mistakenly not passing along the testing knobs from the server to the
  tenant.
- We were printing out the "this test is using a tenant" message before we were
  guaranteed to be using a tenant.

To resolve these issues we now:

- Pass along the testing knobs from the server to the tenant with the exception
  of the Server testing knobs, which mainly apply to the system tenant.
- Print out the tenant message only when we're guranteed to be running the test
  in a tenant.
- Also has a small fix to disable tenant testing when testing the span config
  override.

Release justification: Testing only change
Release note: None
@ajstorm ajstorm marked this pull request as ready for review September 23, 2022 21:44
@ajstorm
Copy link
Collaborator Author

ajstorm commented Sep 23, 2022

This should be ready for review now. Adding @stevendanna to reviewers because I'm proposing we roll back his recent change.

Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

LGTM! Only reason I didn't make this the default initially was to avoid any surprises for others, but I think this is better given that it looks like all the tests pass without issue.

@ajstorm
Copy link
Collaborator Author

ajstorm commented Sep 28, 2022

TFTR @stevendanna!

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Sep 28, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants