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

settings,*: give better names to certain settings #109074

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

knz
Copy link
Contributor

@knz knz commented Aug 19, 2023

Fixes #109046.

Now we have a framework that allows us to rename cluster settings
while preserving backward-compatibility, we can clean up the settings
whose name didn't pass the linter, and had been "grand-fathered". This
also simplifies the linter.

The following have been renamed:

Previous name New name
kv.raft_log.disable_synchronization_unsafe kv.raft_log.synchronization.disabled
sql.trace.log_statement_execute sql.log.all_statements.enabled
cloudstorage.gs.chunking.retry_timeout cloudstorage.gs.chunking.per_chunk_retry.timeout
trace.debug.enable trace.http_debug_endpoint.enabled
bulkio.restore.memory_monitor_ssts bulkio.restore.sst_memory_limit.enabled
bulkio.restore.use_simple_import_spans bulkio.restore.simple_import_spans.enabled
bulkio.backup.export_request_verbose_tracing bulkio.backup.verbose_tracing.enabled
changefeed.idle_timeout changefeed.auto_idle.timeout

The following name suffixes have been adjusted to follow our linter
guidelines:

bulkio.backup.split_keys_on_timestamps - added .enabled
changefeed.balance_range_distribution.enable - renamed to .enabled
changefeed.batch_reduction_retry_enabled - renamed to .enabled
changefeed.new_pubsub_sink_enabled - renamed to .enabled
changefeed.new_webhook_sink_enabled - renamed to .enabled
changefeed.permissions.require_external_connection_sink - added .enabled
debug.panic_on_failed_assertions - added .enabled
diagnostics.reporting.send_crash_reports - added .enabled
kv.closed_timestamp.follower_reads_enabled - renamed to .enabled
kv.range_merge.queue_enabled - renamed to .enabled
kv.range_split.by_load_enabled - renamed to .enabled
kv.transaction.parallel_commits_enabled - renamed to .enabled
kv.transaction.write_pipelining_enabled - renamed to .enabled
kv.transaction.write_pipelining_max_batch_size - renamed to .max_batch_size
server.clock.forward_jump_check_enabled - renamed to .enabled
server.oidc_authentication.autologin - added .enabled
server.web_session_timeout - renamed to .timeout
sql.distsql.flow_stream_timeout - renamed to .timeout
sql.metrics.statement_details.dump_to_logs - added .enabled
stream_replication.job_liveness_timeout - renamed to .timeout

Release note (cli change): The following user-visible cluster settings
have been renamed. The previous name is still available for backward
compatibility.

Previous name New name
server.web_session_timeout server.web_session.timeout
kv.closed_timestamp.follower_reads_enabled kv.closed_timestamp.follower_reads.enabled
kv.range_split.by_load_enabled kv.range_split.by_load.enabled
changefeed.balance_range_distribution.enable changefeed.balance_range_distribution.enabled
changefeed.batch_reduction_retry_enabled changefeed.batch_reduction_retry.enabled
server.clock.forward_jump_check_enabled server.clock.forward_jump_check.enabled
server.oidc_authentication.autologin server.oidc_authentication.autologin.enabled
sql.metrics.statement_details.dump_to_logs sql.metrics.statement_details.dump_to_logs.enabled
sql.trace.log_statement_execute sql.log.all_statements.enabled
trace.debug.enable trace.http_debug_endpoint.enabled

Epic: CRDB-28893

@knz knz requested review from stevendanna and yuzefovich August 19, 2023 12:56
@knz knz requested a review from a team as a code owner August 19, 2023 12:56
@knz knz requested a review from a team August 19, 2023 12:56
@knz knz requested review from a team as code owners August 19, 2023 12:56
@knz knz requested a review from a team August 19, 2023 12:56
@knz knz requested review from a team as code owners August 19, 2023 12:56
@knz knz requested a review from a team August 19, 2023 12:56
@knz knz requested review from a team as code owners August 19, 2023 12:56
@knz knz requested review from srosenberg, renatolabs, ericharmeling and samiskin and removed request for a team August 19, 2023 12:56
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz removed request for a team August 19, 2023 12:58
@knz knz removed the request for review from renatolabs August 19, 2023 12:58
@knz knz force-pushed the 20230819-rename-settings2 branch 2 times, most recently from 7b5cb00 to 3c1f4fa Compare August 19, 2023 13:27
@knz knz mentioned this pull request Aug 20, 2023
@knz knz force-pushed the 20230819-rename-settings2 branch from 3c1f4fa to c3e0e8b Compare August 20, 2023 08:59
craig bot pushed a commit that referenced this pull request Aug 20, 2023
109098: sql: fix a leftover bug r=stevendanna a=knz

This (hidden) bug was left over from #108902.
There is no visible negative effect to this bug being present until setting aliases are effectively introduced, which hasn't happened yet.

No tests included here -- this is exercised by #109077 and #109074 and the CI in those PRs readily fail without this fix.

 Epic: CRDB-27642

Co-authored-by: Raphael 'kena' Poss <[email protected]>
@knz knz force-pushed the 20230819-rename-settings2 branch from c3e0e8b to 81f44cb Compare August 21, 2023 08:10
@@ -85,13 +85,15 @@ var (
"bulkio.backup.split_keys_on_timestamps",
"split backup data on timestamps when writing revision history",
true,
settings.WithName("bulkio.backup.split_keys_on_timestamps.enabled"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@dt Now that we have the option to rename things, I wonder if we want to make a follow-up pass to @knz's work here to address some of our long-standing setting naming gripes?

@knz knz force-pushed the 20230819-rename-settings2 branch 4 times, most recently from e495665 to a63b81d Compare August 21, 2023 11:57
Copy link
Contributor

@j82w j82w left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @knz, @samiskin, and @yuzefovich)


-- commits line 4 at r3:
I created this issue about possibly improving the naming. What do you think? #108508


-- commits line 10 at r3:
Shouldn't there be a release note that cluster settings are getting aliased with new names?

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.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @j82w, @samiskin, and @yuzefovich)


-- commits line 4 at r3:

Previously, j82w (Jake) wrote…

I created this issue about possibly improving the naming. What do you think? #108508

Yes I saw. I started this work in light of your issue.
However, note that the concern you raised there already has a plan under epic CRDB-6671. There's multiple steps there before we can consider directly what you suggest.


-- commits line 10 at r3:

Previously, j82w (Jake) wrote…

Shouldn't there be a release note that cluster settings are getting aliased with new names?

I thought I included this release note already. Doesn't it show up for you?

@j82w
Copy link
Contributor

j82w commented Aug 21, 2023

-- commits line 10 at r3:

Previously, knz (Raphael 'kena' Poss) wrote…

I thought I included this release note already. Doesn't it show up for you?

The 2nd commit has the release note. I left the comment looking at the first commit and forgot to delete it.

@j82w
Copy link
Contributor

j82w commented Aug 21, 2023

-- commits line 4 at r3:

Previously, knz (Raphael 'kena' Poss) wrote…

Yes I saw. I started this work in light of your issue.
However, note that the concern you raised there already has a plan under epic CRDB-6671. There's multiple steps there before we can consider directly what you suggest.

If it already supports alias and a rename is being done why can't we incorporate the setting type into the name now?

@knz
Copy link
Contributor Author

knz commented Aug 21, 2023

If it already supports alias and a rename is being done why can't we incorporate the setting type into the name now?

Because the recommendation you made is only one of the ways to solve the problem that you identified. We are going to explore other ways to solve it first.

craig bot pushed a commit that referenced this pull request Aug 21, 2023
109118: sql: fix another buglet with tenant settings r=stevendanna a=knz

This (hidden) bug was left over from #108902.
There is no visible negative effect to this bug being present until setting aliases are effectively introduced, which hasn't happened yet.

The code modified here is already properly exercised by logic tests.
Also, this is exercised by #109077 and #109074 and the CI in those PRs readily fail without this fix.

Epic: CRDB-27642

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Copy link
Contributor

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

Great usability improvement. We'll definitely want docs follow-up on this.

@florence-crl

Now we have a framework that allows us to rename cluster settings
while preserving backward-compatibility, we can clean up the settings
whose name didn't pass the linter, and had been "grand-fathered". This
also simplifies the linter.

The following have been renamed:

| Previous name                                  | New name                                           |
|------------------------------------------------|----------------------------------------------------|
| `kv.raft_log.disable_synchronization_unsafe`   | `kv.raft_log.synchronization.disabled`             |
| `sql.trace.log_statement_execute`              | `sql.log.all_statements.enabled`                   |
| `cloudstorage.gs.chunking.retry_timeout`       | `cloudstorage.gs.chunking.per_chunk_retry.timeout` |
| `trace.debug.enable`                           | `trace.http_debug_endpoint.enabled`                |
| `bulkio.restore.memory_monitor_ssts`           | `bulkio.restore.sst_memory_limit.enabled`          |
| `bulkio.restore.use_simple_import_spans`       | `bulkio.restore.simple_import_spans.enabled`       |
| `bulkio.backup.export_request_verbose_tracing` | `bulkio.backup.verbose_tracing.enabled`            |
| `changefeed.idle_timeout`                      | `changefeed.auto_idle.timeout`                     |

The following name suffixes have been adjusted to follow our linter
guidelines:

```
bulkio.backup.split_keys_on_timestamps - added .enabled
changefeed.balance_range_distribution.enable - renamed to .enabled
changefeed.batch_reduction_retry_enabled - renamed to .enabled
changefeed.new_pubsub_sink_enabled - renamed to .enabled
changefeed.new_webhook_sink_enabled - renamed to .enabled
changefeed.permissions.require_external_connection_sink - added .enabled
debug.panic_on_failed_assertions - added .enabled
diagnostics.reporting.send_crash_reports - added .enabled
kv.closed_timestamp.follower_reads_enabled - renamed to .enabled
kv.range_merge.queue_enabled - renamed to .enabled
kv.range_split.by_load_enabled - renamed to .enabled
kv.transaction.parallel_commits_enabled - renamed to .enabled
kv.transaction.write_pipelining_enabled - renamed to .enabled
kv.transaction.write_pipelining_max_batch_size - renamed to .max_batch_size
server.clock.forward_jump_check_enabled - renamed to .enabled
server.failed_reservation_timeout - renamed to .timeout
server.oidc_authentication.autologin - added .enabled
server.web_session_timeout - renamed to .timeout
sql.distsql.flow_stream_timeout - renamed to .timeout
sql.metrics.statement_details.dump_to_logs - added .enabled
stream_replication.job_liveness_timeout - renamed to .timeout
```

Release note (cli change): The following user-visible cluster settings
have been renamed. The previous name is still available for backward
compatibility.

| Previous name                                  | New name                                             |
|------------------------------------------------|------------------------------------------------------|
| `server.web_session_timeout`                   | `server.web_session.timeout`                         |
| `kv.closed_timestamp.follower_reads_enabled`   | `kv.closed_timestamp.follower_reads.enabled`         |
| `kv.range_split.by_load_enabled`               | `kv.range_split.by_load.enabled`                     |
| `changefeed.balance_range_distribution.enable` | `changefeed.balance_range_distribution.enabled`      |
| `changefeed.batch_reduction_retry_enabled`     | `changefeed.batch_reduction_retry.enabled`           |
| `server.clock.forward_jump_check_enabled`      | `server.clock.forward_jump_check.enabled`            |
| `server.oidc_authentication.autologin`         | `server.oidc_authentication.autologin.enabled`       |
| `sql.metrics.statement_details.dump_to_logs`   | `sql.metrics.statement_details.dump_to_logs.enabled` |
| `sql.trace.log_statement_execute`              | `sql.log.all_statements.enabled`                     |
| `trace.debug.enable`                           | `trace.http_debug_endpoint.enabled`                  |
@knz knz force-pushed the 20230819-rename-settings2 branch from a63b81d to f3cf1e7 Compare August 21, 2023 16:58
@knz
Copy link
Contributor Author

knz commented Aug 21, 2023

@stevendanna has communicated in private his lack of further objection on this one.

TFYRs

bors r=stevendanna,ericharmeling

@craig
Copy link
Contributor

craig bot commented Aug 21, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 21, 2023

Build succeeded:

@craig craig bot merged commit 4698cfe into cockroachdb:master Aug 21, 2023
@knz knz deleted the 20230819-rename-settings2 branch August 22, 2023 07:10
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.

settings: some settings are poorly named and don't make a good story in docs
5 participants