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

externalconn/testutils: fix a mistake in SetSQLDBForUser #110005

Merged
merged 4 commits into from
Sep 5, 2023

Conversation

knz
Copy link
Contributor

@knz knz commented Sep 5, 2023

All commits but the last are from #110004 (review only the last commit here)

Prior to this patch, this method was incorrectly swapping a SQL
connection to the secondary tenant for one to the system tenant.

This patch fixes it.

Epic: CRDB-18499

knz added 3 commits September 5, 2023 11:44
Before this patch, the DefaultZoneConfig field was taken from the
global default. This is incorrect. This patch fixes it.

Note that this patch is not complete - we also need to update
the non-test version of tenant configs, by communicating
the default config from the KV layer to the SQL service
over the network (using tenant connector).

Release note: None
Release note: None
@knz knz requested review from a team as code owners September 5, 2023 10:17
@knz knz requested review from rharding6373, srosenberg and renatolabs and removed request for a team September 5, 2023 10:17
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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.

Sometimes for fixes like this I think it is useful to describe why/how this was working before.

We might want to take a second look at spanconfigtestcluster since I think this code was copied from there.

@knz knz mentioned this pull request Sep 5, 2023
Prior to this patch, this method was incorrectly swapping a SQL
connection to the secondary tenant for one to the system tenant.

This patch fixes it.

Note: the problem was "invisible" because this incorrect redirect was
the first thing that ever happened in any data file to a secondary
tenant. So really all the "multi-tenant" testdata files were really
exercising the system tenant.

Release note: None
@knz
Copy link
Contributor Author

knz commented Sep 5, 2023

Sometimes for fixes like this I think it is useful to describe why/how this was working before.

Done. (added to commit message)

We might want to take a second look at spanconfigtestcluster since I think this code was copied from there.

Done - it's unaffected because it does not contain a SetSQLDBForUser method.

@knz knz force-pushed the 20230905-externalconn-fix branch from e16128a to 547a048 Compare September 5, 2023 10:56
@knz
Copy link
Contributor Author

knz commented Sep 5, 2023

TFYR!

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Sep 5, 2023

Build succeeded:

@craig craig bot merged commit 2b71df6 into cockroachdb:master Sep 5, 2023
@knz knz deleted the 20230905-externalconn-fix branch September 5, 2023 11:48
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