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

sql: emphasize "virtual cluster" in SQL syntax, keep "tenant" as alias #106110

Merged
merged 17 commits into from
Jul 6, 2023

Conversation

knz
Copy link
Contributor

@knz knz commented Jul 4, 2023

Informs #106068.
Epic: CRDB-29380

See individual commits for details.

knz added 6 commits July 4, 2023 14:15
This aliases `INCLUDE_ALL_SECONDARY_TENANTS` which becomes hidden.

Release note: None
This aliases `TENANT_NAME` which becomes hidden.

Release note: None
This aliases `TENANT` (in `RESTORE` options) which becomes hidden.

Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz force-pushed the 20230704-tenant-alias-syntax branch 2 times, most recently from 5769959 to 88938fa Compare July 4, 2023 15:07
@knz knz force-pushed the 20230704-tenant-alias-syntax branch from 88938fa to 8d1c965 Compare July 4, 2023 15:28
@knz knz added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Jul 4, 2023
@knz knz added the A-multitenancy Related to multi-tenancy label Jul 4, 2023
@knz knz force-pushed the 20230704-tenant-alias-syntax branch from 8d1c965 to 753fc23 Compare July 4, 2023 16:30
@knz knz marked this pull request as ready for review July 4, 2023 17:46
@knz knz requested review from a team as code owners July 4, 2023 17:46
@knz knz requested review from a team as code owners July 4, 2023 17:46
Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

:lgtm:

Just a few questions from me:

  • what's the rationale for removing some of the syntax from the docs diagrams?
  • what's the end vision for the code base (i.e. non-user-visible) as it relates to the word "tenant"?

Reviewed 11 of 11 files at r1, 6 of 6 files at r2, 6 of 6 files at r3, 4 of 4 files at r4, 6 of 6 files at r5, 2 of 2 files at r6, 9 of 9 files at r7, 7 of 7 files at r8, 15 of 15 files at r9, 3 of 3 files at r10, 16 of 16 files at r11, 11 of 11 files at r12, 4 of 4 files at r13, 9 of 9 files at r14, 10 of 10 files at r15, 1 of 1 files at r16, 8 of 8 files at r17, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @knz and @stevendanna)


-- commits line 4 at r1:
nit: should we rename include_all_secondary_tenants field in jobs.proto too?


pkg/multitenant/mtinfopb/info.proto line 46 at r9 (raw file):

  // DroppedName is the name the tenant had before DROP VIRTUAL
  // CLUSTER was run on the tenant. It should be empty for active or

nit: do we want to mention "tenant" here?


pkg/sql/sem/asof/as_of.go line 19 at r14 (raw file):

	"time"

	apd "github.com/cockroachdb/apd/v3"

nit: seems unnecessary.


pkg/sql/parser/testdata/show line 2008 at r11 (raw file):


parse
SHOW VIRTUAL CLUSTER ALL

nit: should we also add CLUSTER_ALL here?

Copy link
Contributor Author

@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.

what's the rationale for removing some of the syntax from the docs diagrams?

They were not meant to be there in the first place.

what's the end vision for the code base (i.e. non-user-visible) as it relates to the word "tenant"?

To be handled separately (#106069)

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @stevendanna and @yuzefovich)


-- commits line 4 at r1:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: should we rename include_all_secondary_tenants field in jobs.proto too?

This is a code change not UX change so will do as part of #106069


pkg/multitenant/mtinfopb/info.proto line 46 at r9 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: do we want to mention "tenant" here?

Let's handle this as part of #106069.


pkg/sql/parser/testdata/show line 2008 at r11 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: should we also add CLUSTER_ALL here?

CLUSTER_ALL is not visible in the syntax; it's a symbolic token in the lexer used to disambiguate the grammar.

@knz
Copy link
Contributor Author

knz commented Jul 6, 2023

TFYR!

bors r=yuzefovich

@craig
Copy link
Contributor

craig bot commented Jul 6, 2023

Build succeeded:

@craig craig bot merged commit 6319c0a into cockroachdb:master Jul 6, 2023
2 checks passed
@blathers-crl
Copy link

blathers-crl bot commented Jul 6, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 874c83b to blathers/backport-release-23.1-106110: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy 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.

3 participants