*: give better name to certain cluster settings #109077
Merged
Conversation
This file contains 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
knz
requested review from
srosenberg and
renatolabs
and removed request for
a team
|
This change is |
knz
force-pushed
the
20230819-rename-settings3
branch
from
7ef13d9 to
b512ce8
Compare
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
force-pushed
the
20230819-rename-settings3
branch
4 times, most recently
from
15bab30 to
bf1c079
Compare
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]>
knz
force-pushed
the
20230819-rename-settings3
branch
3 times, most recently
from
b6d30b4 to
1a3cc92
Compare
stevendanna
approved these changes
Aug 21, 2023
There was a problem hiding this comment.
These looks good to me, but would also be interested in @yuzefovich's thoughts.
yuzefovich
approved these changes
Aug 21, 2023
There was a problem hiding this comment.
Reviewed 36 of 94 files at r2, 1 of 5 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @knz)
pkg/server/node.go line 246 at r3 (raw file):
settings.SystemOnly, "server.secondary_tenants.redact_trace.enabled", "if enabled, storage/KV trace results are rediacted when returned to a virtual cluster",
nit: s/rediacted/redacted/.
pkg/sql/exec_util.go line 237 at r3 (raw file):
settings.TenantReadOnly, "sql.scatter.allow_for_secondary_tenant.enabled", "allow secondary tenants to run ALTER TABLE/INDEX ... SCATTER commands; does not affect the system tenant",
nit: should this also be reworded to use "virtual clusters"?
The following settings have been renamed so they group together under a common prefix: | Previous name | New name | |--------------------------------------------------------------|--------------------------------------------------------| | `tenant_external_io_default_bytes_allowed_before_accounting` | `tenant_cost_control.external_io.byte_usage_allowance` | | `tenant_external_io_ru_accounting_mode` | `tenant_cost_control.external_io.ru_accounting_mode` | | `tenant_cost_control_period` | `tenant_cost_control.token_request_period` | | `tenant_cpu_usage_allowance` | `tenant_cost_control.cpu_usage_allowance` | | `tenant_usage_instance_inactivity` | `tenant_cost_control.instance_inactivity.timeout` | Similarly: | Previous name | New name | |---------------------------------------------------------------------|-------------------------------------------------------------------| | `sql.multi_region.allow_abstractions_for_secondary_tenants.enabled` | `sql.virtual_cluster.feature_access.multiregion.enabled` | | `sql.zone_configs.allow_for_secondary_tenant.enabled` | `sql.virtual_cluster.feature_access.zone_configs.enabled` | | `sql.split_at.allow_for_secondary_tenant.enabled` | `sql.virtual_cluster.feature_access.manual_range_split.enabled` | | `sql.scatter.allow_for_secondary_tenant.enabled` | `sql.virtual_cluster.feature_access.manual_range_scatter.enabled` | The following setting have been renamed because they are not specific to multi-tenancy: | Previous name | New name | |-------------------------------------------------|-----------------------------------------------------------| | `sql.tenant_ru_estimation.enabled` | `sql.explain_analyze.include_ru_estimation.enabled` | | `server.secondary_tenants.redact_trace.enabled` | `trace.redact_traces_at_virtual_cluster_boundary.enabled` | | `server.secondary_tenants.authorization.mode` | `server.virtual_cluster_authorization.mode` | | `spanconfig.tenant_coalesce_adjacent.enabled` | `spanconfig.range_coalescing.application.enabled` | | `spanconfig.storage_coalesce_adjacent.enabled` | `spanconfig.range_coalescing.system.enabled` | | `spanconfig.tenant_limit` | `spanconfig.virtual_cluster.max_spans` | The following have been renamed to e-emphasize "tenants" and for coherence with the use of the key `cluster` in SQL and HTTP requests: | Previous name | New name | |----------------------------------------------------------|------------------------------------------------------------------| | `server.controller.default_tenant` | `server.controller.default_target_cluster` | | `server.controller.default_tenant.check_service.enabled` | `server.controller.default_target_cluster.check_service.enabled` | The following has been renamed for coherence with the SQL syntax: | Previous name | New name | |--------------------------------------|-----------------------------------------------| | `sql.create_tenant.default_template` | `sql.create_virtual_cluster.default_template` | The following settings are also now marked as non-public, since they were not meant for use by end-users yet: - `server.controller.default_tenant` - `trace.redact_traces_at_virtual_cluster_boundary.enabled` - `admission.kv.tenant_weights.enabled` - `admission.kv.stores.tenant_weights.enabled` Release note (cli change): The following cluster settings have been renamed. The previous names are available for backward-compatibility. | Previous name | New name | |------------------------------------------------|---------------------------------------------------| | `spanconfig.tenant_coalesce_adjacent.enabled` | `spanconfig.range_coalescing.application.enabled` | | `spanconfig.storage_coalesce_adjacent.enabled` | `spanconfig.range_coalescing.system.enabled` |
knz
force-pushed
the
20230819-rename-settings3
branch
from
1a3cc92 to
9dc56ae
Compare
|
TFYR! bors r=stevendanna,yuzefovich |
|
Build succeeded: |
knz
commented
Aug 22, 2023
There was a problem hiding this comment.
Reviewable status:
pkg/server/node.go line 246 at r3 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit:
s/rediacted/redacted/.
Fixed.
pkg/sql/exec_util.go line 237 at r3 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: should this also be reworded to use "virtual clusters"?
Done.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Follows #109048.
Epic: CRDB-29380
The following settings have been renamed so they group together under a common prefix:
tenant_external_io_default_bytes_allowed_before_accountingtenant_cost_control.external_io.byte_usage_allowancetenant_external_io_ru_accounting_modetenant_cost_control.external_io.ru_accounting_modetenant_cost_control_periodtenant_cost_control.token_request_periodtenant_cpu_usage_allowancetenant_cost_control.cpu_usage_allowancetenant_usage_instance_inactivitytenant_cost_control.instance_inactivity.timeoutSimilarly:
sql.multi_region.allow_abstractions_for_secondary_tenants.enabledsql.virtual_cluster.feature_access.multiregion.enabledsql.zone_configs.allow_for_secondary_tenant.enabledsql.virtual_cluster.feature_access.zone_configs.enabledsql.split_at.allow_for_secondary_tenant.enabledsql.virtual_cluster.feature_access.manual_range_split.enabledsql.scatter.allow_for_secondary_tenant.enabledsql.virtual_cluster.feature_access.manual_range_scatter.enabledThe following setting have been renamed because they are not specific to multi-tenancy:
sql.tenant_ru_estimation.enabledsql.explain_analyze.include_ru_estimation.enabledserver.secondary_tenants.redact_trace.enabledtrace.redact_traces_at_virtual_cluster_boundary.enabledserver.secondary_tenants.authorization.modeserver.virtual_cluster_authorization.modespanconfig.tenant_coalesce_adjacent.enabledspanconfig.range_coalescing.application.enabledspanconfig.storage_coalesce_adjacent.enabledspanconfig.range_coalescing.system.enabledspanconfig.tenant_limitspanconfig.virtual_cluster.max_spansThe following have been renamed to e-emphasize "tenants" and for
coherence with the use of the key
clusterin SQL and HTTP requests:server.controller.default_tenantserver.controller.default_target_clusterserver.controller.default_tenant.check_service.enabledserver.controller.default_target_cluster.check_service.enabledThe following has been renamed for coherence with the SQL syntax:
sql.create_tenant.default_templatesql.create_virtual_cluster.default_templateThe following settings are also now marked as non-public, since they
were not meant for use by end-users yet:
server.controller.default_tenanttrace.redact_traces_at_virtual_cluster_boundary.enabledadmission.kv.tenant_weights.enabledadmission.kv.stores.tenant_weights.enabledRelease note (cli change): The following cluster settings have been
renamed. The previous names are available for backward-compatibility.
spanconfig.tenant_coalesce_adjacent.enabledspanconfig.range_coalescing.application.enabledspanconfig.storage_coalesce_adjacent.enabledspanconfig.range_coalescing.system.enabled