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: use 'NONVOTERS' as keyword for 'RELOCATE', not 'NON_VOTERS' #73802

Merged
merged 3 commits into from
Dec 14, 2021

Conversation

knz
Copy link
Contributor

@knz knz commented Dec 14, 2021

Informs #73315.
Fixes #63219.

The idiom for positional keywords in SQL is to either use words
separated by spaces (e.g. NOT NULL), or to concatenate the words
together (ISERROR, NOLOGIN, LINESTRING).

Release note (sql change): In the experimental RELOCATE syntax
forms, the positional keyword that indicates that the statement
should move non-voter replicas is now spelled NONVOTERS, instead
of NON_VOTERS previously.

Release note (sql change): The inline help for the ALTER
statements now mentions the RELOCATE syntax.

The idiom for positional keywords in SQL is to either use words
separated by spaces (e.g. NOT NULL), or to concatenate the words
together (ISERROR, NOLOGIN, LINESTRING).

Release note (sql change): In the experimental RELOCATE syntax
forms, the positional keyword that indicates that the statement
should move non-voter replicas is now spelled `NONVOTERS`, instead
of `NON_VOTERS` previously.
@knz knz requested review from lunevalex and a team December 14, 2021 17:38
@knz knz requested a review from a team as a code owner December 14, 2021 17:38
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Release note (sql change): The inline help for the ALTER
statements now mentions the RELOCATE syntax.
@knz knz force-pushed the 20211214-relocate-simplify branch from de70504 to 393268c Compare December 14, 2021 17:45
@rafiss
Copy link
Collaborator

rafiss commented Dec 14, 2021

thank you, seems like this will resolve #63219

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@rafiss rafiss linked an issue Dec 14, 2021 that may be closed by this pull request
There is no need for separate non-terminals for the 'LEASE' variants of
RELOCATE. This commit deletes them.

Release note: None
@knz knz force-pushed the 20211214-relocate-simplify branch from 393268c to d416008 Compare December 14, 2021 19:00
@knz
Copy link
Contributor Author

knz commented Dec 14, 2021

TFYR!

bors r=rafiss,RaduBerinde

Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

do we have to worry about backwards compat?

@rafiss
Copy link
Collaborator

rafiss commented Dec 14, 2021

ordinarily yes, but this is both an experimental feature, and also only intended for internal use.

knz added a commit to knz/cockroach that referenced this pull request Dec 14, 2021
a hiccup was introduced in cockroachdb#73802.

Release note: None
@knz knz mentioned this pull request Dec 14, 2021
@craig
Copy link
Contributor

craig bot commented Dec 14, 2021

Build succeeded:

@craig craig bot merged commit 7bf398f into cockroachdb:master Dec 14, 2021
craig bot pushed a commit that referenced this pull request Dec 14, 2021
73773: roachtest: add knex test r=rafiss a=otan

These tests *will* fail on master as knex does not handle the PK/FK name
rename correctly. However, this commit alone makes it backportable to
v21.2, so this is being presented as is.

Release note: None

73790: bench/rttanalysis: stop printing text trace, write jaeger r=ajwerner a=ajwerner

This commit changes the behavior on failure of the rttanalysis tests.
Before it would print a textual trace. That was pretty much useless and
it was very loud. Now it'll write the jaeger trace json to a file in the
logging directory. Hopefully that'll be more useful to inspect the difference
between the expectation and the observed result.

Release note: None

73812: sql: fix some help texts r=otan a=knz

(first 3 commits from #73802.) 
a hiccup was introduced in #73802, but I don't want to cancel the CI / bors build to put the fix in, hence a new PR.

Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
@knz knz deleted the 20211214-relocate-simplify branch December 14, 2021 20:36
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.

sql,kv: SQL syntax for the RELOCATE statement is non-idiomatic
6 participants