cli,sql: improve the UX for viewing and updating zone configurations #28612

knz · 2018-08-15T08:06:47Z

Summary of visible changes:

new statement forms:
- ALTER ... CONFIGURE ZONE USING num_replicas = 123, ... to edit a zone config. The compact constraint syntax is still supported, e.g. USING constraints = '[+foo]'.
- ALTER ... CONFIGURE ZONE USING DEFAULT to reset a zone config to inherited defaults
- ALTER ... CONFIGURE ZONE DISCARD to remove a zone config
compatibility forms with previous versions of CockroachDB, probably should not be documented but will ease transition:
- ALTER ... CONFIGURE ZONE = '...yaml...' to set multiple zone parameters at once using YAML
- ALTER ... CONFIGURE ZONE = '' ensures the zone config exists but is otherwise a no-op (it does not reset it to defaults if it already existed)
- ALTER ... CONFIGURE ZONE = NULL to remove a zone config
the ALTER ... CONFIGURE ZONE statement forms now can be prepared on pgwire and accept placeholders.
crdb_internal.zones and SHOW ZONE CONFIGURATION(s) have a new column config_sql in addition to the existing config_yaml and config_protobuf. This new columns reports the ALTER syntax that reproduces the zone config with the new syntax.
cockroach zone sub-commands are now deprecated.
cockroach zone rm now prints no output unless there is an error.

cockroach-teamcity · 2018-08-15T08:06:53Z

This change is

knz · 2018-08-15T08:07:13Z

@benesch this is what I have so far. You mentioned I can do something with something called "sources" to not have to rewrite the import paths? how does that work?

benesch · 2018-08-15T16:07:29Z

Yes, you would just do what we did with e.g. go-bindata: https://github.com/cockroachdb/cockroach/blob/master/Gopkg.toml#L55

You just list a constraint in Gopkg.toml that tells Dep to use a different repository for that import path. Then you don't need to rewrite import paths anywhere.

What you've done here looks fine to me too though!

knz · 2018-08-20T09:03:20Z

yes that worked, thanks

knz · 2018-08-20T12:40:06Z

@a-robinson @benesch can you start reviewing the first commits in this PR while I work on the 2nd part (expressivity of the SQL syntax)

benesch

Looks good so far. Thanks for taking this on, @knz!

I do fear that you're causing unnecessary pain by removing EXPERIMENTAL. Can't we spread that pain over two versions? Is replacing it with an optional grammar rule

opt_experimental: EXPERIMENTAL | / * empty */

somehow intractable? That way the 2.0 client would continue to work against 2.1 with no extra effort, as would our roachtests.

Reviewed 3 of 3 files at r1, 28 of 29 files at r2, 3 of 3 files at r3, 10 of 10 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cli/cli_test.go, line 528 at r2 (raw file):

	// num_replicas: 1
	// constraints: [+zone=us-east-1a, +ssd]
	// "

I'm not a huge fan of the new formatting. The old formatting isn't great, but I'm worried you're breaking backwards compatibility here for little benefit. Someone who wants to parse this YAML will now need to strip out the " and the extra line.

pkg/sql/set_zone_config.go, line 52 at r4 (raw file):

	typ := yamlConfig.ResolvedType()
	switch typ {

totally optional nit

switch typ := yamlConfig.ResolvedType(); switch typ {

pkg/sql/set_zone_config.go, line 166 at r4 (raw file):

		); err != nil {
			return err
		}

I haven't fully verified whether moving this stanza up and removing the reprising in the func itself is equivalent to the old code. @a-robinson should double-check this.

pkg/sql/show_zone_config.go, line 123 at r4 (raw file):

// generateZoneConfigIntrospectionValues creates a result row
// suitable for populating crdb_internal.zones or SHOW ZONE CONFIG.
// The values are populates into the `values` first argument.

"populated"

knz

Thanks for the early feedback.

I am very strongly against accomnodating EXPERIMENTAL. This is experimental after all which means "no guarantees". I do not want to create a precedent for us starting to accommodate backward compatibility for experimental syntax -- that's defeating the point of having the experimental bit in the first place.

There was a mistake made here, which was to start relying on an experimental feature as a client/server API. This was a a mistake and the approach I am taking in this PR is the price we have to pay today for this past mistake.

The grammar accommodation you propose, while technically apparently minor, would have a much higher price because it would set a nasty precedent.

Will address the other comments in a next iteration.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cli/cli_test.go, line 528 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I'm not a huge fan of the new formatting. The old formatting isn't great, but I'm worried you're breaking backwards compatibility here for little benefit. Someone who wants to parse this YAML will now need to strip out the " and the extra line.

--format=raw is suitable for automation.

When the output is a terminal, the result can still be copy-pasted.

pkg/sql/set_zone_config.go, line 166 at r4 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I haven't fully verified whether moving this stanza up and removing the reprising in the func itself is equivalent to the old code. @a-robinson should double-check this.

It seems to not be equivalent according to the tests :)

I need to work on this a little more.

benesch

I am very strongly against accomnodating EXPERIMENTAL. This is experimental after all which means "no guarantees". I do not want to create a precedent for us starting to accommodate backward compatibility for experimental syntax -- that's defeating the point of having the experimental bit in the first place.

I see your point, but here we're introducing a unforced backwards incompatibility for the sole purpose of preserving the sanctity of EXPERIMENTAL. That's extremely unfortunate. I see EXPERIMENTAL as a license to change, not a mandate to change.

There was a mistake made here, which was to start relying on an experimental feature as a client/server API. This was a a mistake and the approach I am taking in this PR is the price we have to pay today for this past mistake.

Partitioning wouldn't have landed without this approach. Perhaps we need a different keyword for statements that are not suitable for external users but that we're willing to at least go through a deprecation cycle with for our own CLI's sake?

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cli/cli_test.go, line 528 at r2 (raw file):

Previously, knz (kena) wrote…

--format=raw is suitable for automation.

When the output is a terminal, the result can still be copy-pasted.

I'm still not convinced that's reason enough to change the default output format.

knz · 2018-08-20T19:26:03Z

Perhaps we need a different keyword for statements that are not suitable for external users but that we're willing to at least go through a deprecation cycle with for our own CLI's sake?

There's something even better: no keyword, but mandate some session variable is set to use the feature.

benesch · 2018-08-20T19:27:49Z

There's something even better: no keyword, but mandate some session variable is set to use the feature.

Ack. That is a nice solution.

knz

Reviewable status: complete! 1 of 0 LGTMs obtained

pkg/cli/zone.go, line 101 at r5 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

nit: getCLISpecifierAndZoneConf (Cli vs CLI)

Done.

pkg/cli/zone.go, line 108 at r5 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

nit: ditto

Done.

pkg/sql/crdb_internal.go, line 1645 at r6 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Can this be null?

Yes; added a comment.

pkg/sql/parser/sql.y, line 1346 at r7 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I'm a little bit concerned that USING DEFAULT is not a common case and will be confusing to explain. In the tests I used ALTER ... EXPERIMENTAL CONFIGURE ZONE '' as a hack to install a zone config that changed no fields from the parent zone config. Would a user ever want to do that? If you're installing a zone config, it's almost always so you can override some field from the parent.

I guess the one case is where you're planning to change some default in, say, a database config, and you want to prevent that default from applying to one or two tables in that database. So you run ALTER ... CONFIGURE ZONE USING DEFAULT on those two tables. But the wording here doesn't do a good job of indicating what's going on. And you could just as well run ALTER ... CONFIGURE ZONE USING param_i_want_to_change = value_i_want_to_keep.

Put another way: how will we explain to users the difference between DISCARD and USING DEFAULT?

You're right; it's hard to explain and also unjustifiable for users until we do cascading zone configs.

I'm hiding them away from docs.

pkg/sql/sem/tree/zone.go, line 64 at r5 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

tiny tiny nit: "represents a", not "represents an"

Done.

This is part 1/3 of the general project to improve the UX of zone configurations in CockroachDB 2.1. This patch does the following: - it removes the EXPERIMENTAL keyword from the grammar for statements that view or change zone configurations. - it adds missing SQL contextual help strings. - it removes the "skip doc" parser annotation so that syntax diagrams can be generated. - it modifies the `cockroach zone` CLI utility to avoid using the EXPERIMENTAL keyword. For compatibility with pre-2.1 nodes, the code falls back to using EXPERIMENTAL if the syntax without EXPERIMENTAL is not recognized. Release note (cli change): `cockroach zone rm` will now produce no output unless an error occurs.

This is part 2/3 of of the general project to improve the UX of zone configurations in CockroachDB 2.1. This patch does the following: - it creates a new column `config_sql` in `crdb_internal.zones` and the output of SHOW ZONE CONFIGURATION. This contains a copy of the allowed ALTER syntax which will be introduced in the next patch. - it cleans up and simplifies the implementation ALTER ... CONFIGURE ZONE, thereby simplifying further improvements. This patch also makes ALTER ... CONFIGURE ZONE able to accept strings that do not contain a terminating newline. This makes it possible to specify unescaped strings, for example `ALTER ... CONFIGURE ZONE 'num_replicas: 3'` where previously one would have needed to specify `e'num_replicas:3 \n'`. The behavior for deletion is preserved: a NULL values requests a removal of the zone configuration; an empty string (or containing only whitespace) is a no-op (not a deletion). Release note: None

This is part 3/3 of the general project to improve the UX of zone configurations in CockroachDB 2.1. This patch introduces the extended syntax for the ALTER ... CONFIGURE ZONE statement. The following forms are now supported: ``` -- New: remove a zone configuration. ALTER ... CONFIGURE ZONE DISCARD; -- New: reset a zone config to the inherited defaults. ALTER ... CONFIGURE ZONE USING DEFAULT; -- New: configure parameters using SQL expressions. -- Setting `constraints` and `lease_preferences` still uses YAML. ALTER ... CONFIGURE ZONE USING num_replicas = 1, gc.ttlseconds = 3600, constraints = '[+attr=ssd]'; -- Preserved for backward compatibility: -- set multiple variables at once using YAML. ALTER ... CONFIGURE ZONE = '...yaml...'; -- Preserved for backward compatibility; -- equivalent to ALTER ... CONFIGURE ZONE DISCARD: ALTER ... CONFIGURE ZONE = NULL; ``` The disambiguation using either `=` (or, equivalently, TO) or USING is necessary to disambiguate the grammar (`ZONE foo = bar` would be parsable as either the first form with expression `(foo = bar)` or the second form with a single parameter set). Additionally, this patch ensures (and tests) that the ALTER ... CONFIGURE ZONE statements can be prepared and use placeholders. Release note (sql change): CockroachDB now recognizes `ALTER ... CONFIGURE ZONE` to add, modify, reset and remove zone configurations. Release note (sql change): The SQL statements to modify zone configurations can now use placeholders (`$1`, etc) and be prepared for multiple executions.

@benesch

Requested/suggested by @benesch: this patch makes the various `cockroach zone` sub-commands report a deprecation warning and a reference to the new SQL interface: ``` $ cockroach zone ls Command "ls" is deprecated, use SHOW ZONE CONFIGURATIONS in a SQL client instead. $ cockroach zone get .default Command "get" is deprecated, use SHOW ZONE CONFIGURATION FOR ... in a SQL client instead. $ cockroach zone set .default Command "set" is deprecated, use ALTER ... CONFIGURE ZONE in a SQL client instead. $ cockroach zone rm liveness Command "rm" is deprecated, use ALTER ... CONFIGURE ZONE DISCARD in a SQL client instead. ``` Release note (cli change): The various `cockroach zone` sub-commands are now deprecated and will be removed in a future version of CockroachDB. Clients should use the SQL interface instead via `SHOW ZONE CONFIGURATION(s)` or `ALTER ... CONFIGURE ZONE`.

knz · 2018-09-06T03:16:38Z

rebased; rfal

benesch

Reviewed 53 of 53 files at r9, 17 of 17 files at r10, 41 of 41 files at r11, 1 of 1 files at r12.
Reviewable status: complete! 1 of 0 LGTMs obtained

knz · 2018-09-06T15:46:22Z

thanks

bors r+

28612: cli,sql: improve the UX for viewing and updating zone configurations r=knz a=knz Fixes #23791. Summary of visible changes: - new statement forms: - `ALTER ... CONFIGURE ZONE USING num_replicas = 123, ...` to edit a zone config. The compact constraint syntax is still supported, e.g. `USING constraints = '[+foo]'`. - `ALTER ... CONFIGURE ZONE USING DEFAULT` to reset a zone config to inherited defaults - `ALTER ... CONFIGURE ZONE DISCARD` to remove a zone config - compatibility forms with previous versions of CockroachDB, probably should not be documented but will ease transition: - `ALTER ... CONFIGURE ZONE = '...yaml...'` to set multiple zone parameters at once using YAML - `ALTER ... CONFIGURE ZONE = ''` ensures the zone config exists but is otherwise a no-op (it does not reset it to defaults if it already existed) - `ALTER ... CONFIGURE ZONE = NULL` to remove a zone config - the `ALTER ... CONFIGURE ZONE` statement forms now can be prepared on pgwire and accept placeholders. - `crdb_internal.zones` and `SHOW ZONE CONFIGURATION(s)` have a new column `config_sql` in addition to the existing `config_yaml` and `config_protobuf`. This new columns reports the ALTER syntax that reproduces the zone config with the new syntax. - `cockroach zone` sub-commands are now deprecated. - `cockroach zone rm` now prints no output unless there is an error. Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>

craig · 2018-09-06T16:15:45Z

Build succeeded

GitHub CI (Cockroach)

29632: release-2.0: cli/zone: make the client cross-compatible with 2.1 r=knz a=knz Accompanies #28612. Release note (cli change): The command `cockroach zone` is upgraded to become compatible with CockroachDB 2.1; note however that `cockroach zone` also becomes *deprecated* in CockroachDB 2.1 in favor of SQL ALTER statements to update zone configurations. Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>

…tput The endpoint was broken by cockroachdb#28612, which changed the output schema of `SHOW ALL ZONE CONFIGURATIONS`, which was used by the endpoint. The breakage was not caught because the unit test for the endpoint was skipped due to flakiness under stress. Release note: None

Switch from using `MustBeDBytes` to `AsDBytes` to also accept strings. Also change the index of `zcBytes` to [4] due to the change of the zone config table schema in cockroachdb#28612. Release note: none

…tput The endpoint was broken by cockroachdb#28612, which changed the output schema of `SHOW ALL ZONE CONFIGURATIONS`, which was used by the endpoint. The breakage was not caught because the unit test for the endpoint was skipped due to flakiness under stress. Release note: None

@jseldess

30747: cli: Mark zone command deprecated so it doesn't show up in help text r=a-robinson a=a-robinson We had previously marked zone's subcommands as deprecated in bfc190d, which meant they didn't show up as available subcommands in the help text of `cockroach zone`. However, there was no deprecation message on the zone command itself, which made the lack of subcommands confusing, and also meant that it showed up as a command in the help text of the top level `cockroach` command. This fixes both issues. Release note: None --- cc @jseldess just as an additional reminder on top of the docs-todo label on this and #28612 that the `cockroach zone` command is being deprecated in favor of the `SHOW ZONE` and `CONFIGURE ZONE` sql syntax. If you think this is a big loss in usability, speak up before the release :) Fixes #30728 Co-authored-by: Alex Robinson <alexdwanerobinson@gmail.com>

knz requested review from a team August 15, 2018 08:06

knz force-pushed the 20180815-zone-configs branch from 83c2363 to 2448297 Compare August 20, 2018 09:03

knz requested a review from a team as a code owner August 20, 2018 12:30

knz requested review from a team August 20, 2018 12:30

knz changed the title ~~[DNM] deps: fork go-yaml~~ cli,sql: improve the UX for viewing and updating zone configurations Aug 20, 2018

knz force-pushed the 20180815-zone-configs branch from 8da3758 to 0fec9de Compare August 20, 2018 12:33

knz force-pushed the 20180815-zone-configs branch 2 times, most recently from e558ccc to 1e09ccc Compare August 20, 2018 15:08

knz requested a review from a team August 20, 2018 15:08

knz closed this Aug 20, 2018

knz deleted the 20180815-zone-configs branch August 20, 2018 16:02

knz restored the 20180815-zone-configs branch August 20, 2018 16:02

knz reopened this Aug 20, 2018

knz force-pushed the 20180815-zone-configs branch from 1e09ccc to d1ad8e0 Compare August 20, 2018 17:47

knz requested a review from a team as a code owner August 20, 2018 17:47

benesch suggested changes Aug 20, 2018

View reviewed changes

knz commented Aug 20, 2018

View reviewed changes

benesch reviewed Aug 20, 2018

View reviewed changes

knz force-pushed the 20180815-zone-configs branch from 8260562 to c48cfee Compare September 5, 2018 21:34

knz commented Sep 5, 2018

View reviewed changes

knz added 4 commits September 6, 2018 00:30

knz force-pushed the 20180815-zone-configs branch from c48cfee to bfc190d Compare September 5, 2018 22:30

benesch approved these changes Sep 6, 2018

View reviewed changes

knz mentioned this pull request Sep 6, 2018

release-2.0: cli/zone: make the client cross-compatible with 2.1 #29632

Merged

knz added the docs-todo label Sep 6, 2018

craig bot merged commit bfc190d into cockroachdb:master Sep 6, 2018

tbg mentioned this pull request Sep 6, 2018

roachtest: drop/tpcc/w=100,nodes=9 failed on master #29665

Closed

This was referenced Sep 12, 2018

roachtest: clearrange failed on release-2.1 #30045

Closed

release-2.1: table caching and zone config improvements #30173

Merged

knz mentioned this pull request Sep 21, 2018

sql: TestGetZoneConfig failed under stress #29461

Closed

a-robinson mentioned this pull request Sep 27, 2018

cli: Mark zone command deprecated so it doesn't show up in help text #30747

Merged

jseldess mentioned this pull request Sep 28, 2018

Zone config manipulation via SQL cockroachdb/docs#2965

Closed

knz deleted the 20180815-zone-configs branch February 14, 2019 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli,sql: improve the UX for viewing and updating zone configurations #28612

cli,sql: improve the UX for viewing and updating zone configurations #28612

knz commented Aug 15, 2018 •

edited

Loading

cockroach-teamcity commented Aug 15, 2018

knz commented Aug 15, 2018

benesch commented Aug 15, 2018

knz commented Aug 20, 2018

knz commented Aug 20, 2018

benesch left a comment

knz left a comment

benesch left a comment

knz commented Aug 20, 2018

benesch commented Aug 20, 2018

knz left a comment

knz commented Sep 6, 2018

benesch left a comment

knz commented Sep 6, 2018

craig bot commented Sep 6, 2018

cli,sql: improve the UX for viewing and updating zone configurations #28612

cli,sql: improve the UX for viewing and updating zone configurations #28612

Conversation

knz commented Aug 15, 2018 • edited Loading

cockroach-teamcity commented Aug 15, 2018

knz commented Aug 15, 2018

benesch commented Aug 15, 2018

knz commented Aug 20, 2018

knz commented Aug 20, 2018

benesch left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

benesch left a comment

Choose a reason for hiding this comment

knz commented Aug 20, 2018

benesch commented Aug 20, 2018

knz left a comment

Choose a reason for hiding this comment

knz commented Sep 6, 2018

benesch left a comment

Choose a reason for hiding this comment

knz commented Sep 6, 2018

craig bot commented Sep 6, 2018

Build succeeded

knz commented Aug 15, 2018 •

edited

Loading