sctest: unskip multitenant for schema change tests #160557
Merged
+63
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
Member
|
This change is |
rafiss
requested review from
fqazi,
nameisbhaskar and
williamchoe3
and removed request for
a team
fqazi
requested changes
Jan 6, 2026
Collaborator
fqazi
left a comment
fqazi
left a comment
There was a problem hiding this comment.
End to end side effects looks unhappy, with the switch to multi-tenant. Could we be using the wrong interface there?
@fqazi reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nameisbhaskar and @williamchoe3).
Collaborator
Author
|
Fixed -- We also had to pass in the SQLCodec so that the schema TestDeps accesss the tenant correctly. |
fqazi
approved these changes
Jan 6, 2026
Collaborator
fqazi
left a comment
fqazi
left a comment
There was a problem hiding this comment.
@fqazi reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:
Collaborator
Author
|
bors r+ |
Collaborator
Author
|
bors r- |
Contributor
|
Canceled. |
Collaborator
Author
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Jan 6, 2026
160557: sctest: unskip multitenant for schema change tests r=rafiss a=rafiss This fixes external process tenant startup to propagate version settings from the parent, allowing tenants to start when the cluster is running at an older version (e.g., MinSupported). We also had to pass in the SQLCodec so that the schema TestDeps accesss the tenant correctly. Note that MultiRegionTestClusterFactory already allowed multitenant testing. fixes #142814 fixes #109457 Release note: None Co-authored-by: Rafi Shamim <[email protected]>
Contributor
|
Build failed: |
Collaborator
Author
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Jan 7, 2026
160557: sctest: unskip multitenant for schema change tests r=rafiss a=rafiss This fixes external process tenant startup to propagate version settings from the parent, allowing tenants to start when the cluster is running at an older version (e.g., MinSupported). We also had to pass in the SQLCodec so that the schema TestDeps accesss the tenant correctly. Note that MultiRegionTestClusterFactory already allowed multitenant testing. fixes #142814 fixes #109457 Release note: None Co-authored-by: Rafi Shamim <[email protected]>
Contributor
|
Build failed: |
This fixes external process tenant startup to propagate version settings from the parent, allowing tenants to start when the cluster is running at an older version (e.g., MinSupported). We also had to pass in the SQLCodec so that the schema TestDeps accesss the tenant correctly. Note that MultiRegionTestClusterFactory already allowed multitenant testing. Release note: None
…bIsIdle The test passed without other changes. Release note: None
Collaborator
Author
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Jan 7, 2026
153361: sql: remove some EXPLAIN [ANALYZE] fields in non-VERBOSE mode r=yuzefovich a=mgartner The output of `EXPLAIN [ANALYZE]` in non-`VERBOSE` mode is now more succinct. In the header above the query plan the following changes have been made: 1. "isolation level" is only shown if it is not "serializable". 2. "priority" is only shown if it is not "normal". 3. "quality of service" is only shown if it is not "regular". 4. "kv cpu time" is not shown. 5. "sql cpu time" is not shown. 6. "DistSQL network usage" is only show if it is greater than 0. 7. "maximum memory usage" is not shown. 8. "cumulative time spent in KV" is not shown. 9. "vectorized" is only shown if it is "false". 10. "buffered writes enabled" is not shown. In each node of the query plan the following changes have been made: 1. "KV pairs read" is not shown. 2. "KV bytes read" is not shown. 3. "KV gRPC calls" is not shown. 4. "estimated max memory allocated" is not shown. 5. "estimated max sql temp disk usage" is not shown. 6. "actual row count" is directly above "estimated row count". Here's an example of what `EXPLAIN ANALYZE` looked like before: ``` planning time: 243µs execution time: 632µs distribution: local vectorized: true plan type: generic, reused cumulative time spent in KV: 331µs maximum memory usage: 20 KiB DistSQL network usage: 0 B (0 messages) regions: us-east1 sql cpu time: 20µs estimated RUs consumed: 0 isolation level: serializable priority: normal quality of service: regular • scan sql nodes: n1 kv nodes: n1 regions: us-east1 actual row count: 0 KV time: 331µs KV rows decoded: 0 KV bytes read: 0 B KV gRPC calls: 1 estimated max memory allocated: 20 KiB sql cpu time: 20µs missing stats table: t@t_pkey spans: [/0 - /0] ``` And here's the output with the same query after the changes in this commit: ``` planning time: 325µs execution time: 974µs distribution: local plan type: generic, reused regions: us-east1 estimated RUs consumed: 0 • scan sql nodes: n1 kv nodes: n1 regions: us-east1 KV time: 610µs KV rows decoded: 0 sql cpu time: 29µs actual row count: 0 missing stats table: t@t_pkey spans: [/0 - /0] ``` Epic: None Release note (sql change): The output of `EXPLAIN [ANALYZE]` in non-`VERBOSE` mode is now more succinct. 160206: sql: clean up dependency graph a bit r=yuzefovich a=yuzefovich This PR contains a few commits that break a few dependencies to allow more parallelism during builds. Informs: #79357. Epic: None Release note: None 160557: sctest: unskip multitenant for schema change tests r=rafiss a=rafiss This fixes external process tenant startup to propagate version settings from the parent, allowing tenants to start when the cluster is running at an older version (e.g., MinSupported). We also had to pass in the SQLCodec so that the schema TestDeps accesss the tenant correctly. Note that MultiRegionTestClusterFactory already allowed multitenant testing. fixes #142814 fixes #109457 Release note: None Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Rafi Shamim <[email protected]>
Contributor
|
Build failed (retrying...): |
Contributor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes external process tenant startup to propagate
version settings from the parent, allowing tenants to start
when the cluster is running at an older version
(e.g., MinSupported).
We also had to pass in the SQLCodec so that the schema TestDeps
accesss the tenant correctly.
Note that MultiRegionTestClusterFactory already allowed
multitenant testing.
fixes #142814
fixes #109457
Release note: None