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.

Sign up for GitHub

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

Jump to bottom

cluster settings: add setting class to naming #108508

Closed
j82w opened this issue Aug 10, 2023 · 2 comments
Closed

cluster settings: add setting class to naming #108508

j82w opened this issue Aug 10, 2023 · 2 comments
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@j82w
Copy link
Contributor

j82w commented Aug 10, 2023 •
edited by knz
Loading

Problem:
Cluster settings have 3 options, and there is no way for users and/or engineers to know which setting is for which tenant. This causes confusion and requires either trying it on each of the different tenants or looking up the code to see what the settings option is specified in the code.

Solution:
Add the cluster setting type to the cluster naming. Existing cluster settings can not be renamed because it would be a breaking change. The way to workaround this

  1. Define the new naming schema and force all new cluster settings to follow it to stop the issue from growing.
  2. Add alias support to the cluster naming. That way all existing settings can be renamed to the new format, but still support the original name.
// SystemOnly settings are associated with single-tenant clusters and host
// clusters. Settings with this class do not exist on non-system tenants and
// can only be used by the system tenant.
SystemOnly Class = iota

// TenantReadOnly settings are visible to non-system tenants but cannot be
// modified by the tenant. Values for these settings are set from the system
// tenant and propagated from the host cluster.
TenantReadOnly

// TenantWritable settings are visible to and can be modified by non-system
// tenants. The system can still override these settings; the overrides are
// propagated from the host cluster.
TenantWritable

Jira issue: CRDB-30501
Epic: CRDB-6671
@j82w j82w added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Aug 10, 2023
@knz
Copy link
Contributor

knz commented Aug 10, 2023

I am very interested in an infrastructure to rename (or provide aliases) to cluster settings.

The place I'm coming from is that many cluster settings today have non-descriptive or poorly-thought names, that are hard to remember or properly document. From a UX design perspective it's important to keep the option to change the name of things on screen over time, in a backward-compatible fashion.

@knz knz mentioned this issue Aug 17, 2023
Merged
@knz knz changed the title cluster settings: add setting type to naming cluster settings: add setting class to naming Aug 17, 2023
craig bot pushed a commit that referenced this issue Aug 18, 2023
@knz
Merge #108902
9ce61b0 
108902: settings,*: update the settings.Setting API r=yuzefovich a=knz

Fixes #108903.
Informs: #108508
Epic: CRDB-27642

TLDR: this commit introduces an API distinction between:

- the *name* of a cluster setting used for user-facing UX, including the SQL syntax and error messages.
- the *key* of a cluster setting used to store its value in `system.settings` and organize various data structures.

(In this commit, the name and key remain equivalent to each other; only the interfaces change.)

This change achieves the following:

- it introduces specific Go types for both key and name. The benefit here is to avoid the go `string` type; and force users of the API to consider what data is used through the settings API.

  As a side benefit, it ensures that every setting key or name included in logs and error messages is not redacted away.

- it paves the way for a later change where the name is allowed to diverge from the key. This will allow us to enhance UX without breaking compatibility.

- as a side-benefit, it also marks the "setting origin" attribute
  as non-redactable.

Salient API change, prior to this change:
```go
type Setting interface {
  // Key returns the name of the specific cluster setting.
  Key() string
}
```

After this change:

```go
type Setting interface {
  // InternalKey returns the internal key used to store the setting.
  // To display the name of the setting (eg. in errors etc) or the
  // SET/SHOW CLUSTER SETTING statements, use the Name() method instead.
  //
  // The internal key is suitable for use with:
  // - direct accesses to system.settings.
  // - rangefeed logic associated with system.settings.
  // - (in unit tests only) interchangeably with the name for SET CLUSTER SETTING
  InternalKey() InternalKey

  // Name is the user-visible (display) name of the setting.
  // This is suitable for:
  // - SHOW/SET CLUSTER SETTING.
  // - inclusion in error messages.
  Name() SettingName
}
```

Release note: None


Co-authored-by: Raphael 'kena' Poss <[email protected]>
@knz knz mentioned this issue Aug 18, 2023
Closed
@j82w j82w mentioned this issue Aug 21, 2023
Merged
@knz
Copy link
Contributor

knz commented Oct 4, 2023 •
edited
Loading

We have addressed this issue in the following ways:

The one thing we have decided NOT to do was to include the class as part of the setting name. It's unclear what actual user problem this would solve, AND it would introduce a new problem: it would require us to teach the user what this means and i would make it hard(er) to change the class in the future.

@knz knz closed this as completed Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
No open projects
SQL Foundations
Archived in project
Milestone
No milestone
Development

No branches or pull requests
2 participants
@knz @j82w