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

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

Closed
knz opened this issue Aug 18, 2023 · 0 comments · Fixed by #109074
Closed

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

knz opened this issue Aug 18, 2023 · 0 comments · Fixed by #109074
Assignees
@knz
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@knz
Copy link
Contributor

knz commented Aug 18, 2023 •
edited by cockroach-jira-scripts
Loading

Extracted from #108508..

Describe the problem

Several settings have non-descriptive names. A couple even escaped the name linter when they were originally introduced due to a build system mishap.

We would like to rename settings for better UX and docs.

Expected behavior

New setting names, but without breaking backward compatibility.

Epic: CRDB-28893

Jira issue: CRDB-30749
@knz knz added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Aug 18, 2023
@knz knz self-assigned this Aug 18, 2023
This was referenced Aug 18, 2023
Merged
Merged
Merged
craig bot pushed a commit that referenced this issue Aug 19, 2023
@knz
Merge #109012
0352a78 
109012: settings,*: use the option pattern with the setting register functions r=yuzefovich a=knz

First commit from  #108902.
Fixes #109011.
Informs #109046.

TLDR: this patch introduces the "functional option style" in the
settings API.

For example, this call:
```go
var s = func() *settings.BoolSetting {
  s := settings.RegisterBoolSetting(...).WithPublic()
  s.SetReportable(true)
}()
```

can now be written as:
```go
var s = settings.RegisterBoolSetting(...,
  settings.WithPublic,
  settings.WithReportable(true),
)
```

Internally, the register function now take a last argument of type
`...SettingOption`.
The following options are defined:

```go
var Retired SettingOption
var WithPublic SettingOption
func WithReportable(reportable bool) SettingOption
func WithVisibility(v Visibility) SettingOption

func WithValidateDuration(fn func(time.Duration) error) SettingOption // NEW
var NonNegativeDuration SettingOption
var PositiveDuration SettingOption
func NonNegativeDurationWithMaximum(maxValue time.Duration) SettingOption
func DurationWithMinimum(minValue time.Duration) SettingOption // RENAMED
func DurationWithMinimumOrZeroDisable(minValue time.Duration) SettingOption // NEW
func DurationInRange(minVal, maxVal int64) SettingOption // NEW

func WithValidateFloat(fn func(float64) error) SettingOption // NEW
var NonNegativeFloat SettingOption
var PositiveFloat SettingOption
func NonNegativeFloatWithMaximum(maxValue float64) SettingOption
func FloatWithMinimum(minValue float64) SettingOption // NEW
func FloatWithMinimumOrZeroDisable(minValue float64) SettingOption // NEW
var NonZeroFloat SettingOption // NEW
var Fraction SettingOption // NEW
var FractionUpperExclusive SettingOption // NEW
func FloatInRange(minVal, maxVal float64) SettingOption // NEW
func FloatInRangeUpperExclusive(minVal, maxVal float64) SettingOption // NEW

func WithValidateInt(fn func(int64) error) SettingOption // NEW
var NonNegativeInt SettingOption
var PositiveInt SettingOption
func NonNegativeIntWithMaximum(maxValue int64) SettingOption
func IntWithMinimum(minValue int64) SettingOption // NEW
func IntInRange(minVal, maxVal int64) SettingOption // NEW
func IntInRangeOrZeroDisable(minVal, maxVal int64) SettingOption // NEW

func WithValidateProto(fn func(*Values, protoutil.Message) error) SettingOption // NEW

func WithValidateString(fn func(*Values, string) error) SettingOption // NEW

func ByteSizeWithMinimum(minVal int64) SettingOption // NEW
```

Release note: None
Epic: CRDB-28893

Co-authored-by: Raphael 'kena' Poss <[email protected]>
@knz knz mentioned this issue Aug 19, 2023
Merged
craig bot pushed a commit that referenced this issue Aug 19, 2023
@knz
Merge #109048
6e160d5 
109048: settings: allow setting names to diverge from the key r=stevendanna a=knz

Informs #109046.
Epic: CRDB-28893

This patch makes it possible to change the name of cluster settings
over time while preserving backward-compatibility.

For example:
```go
var s = RegisterDurationSetting(
  "server.web_session_timeout", // <- this is the key; it is immutable.
  settings.WithName("server.web_session.timeout"), // <- user-visible name
)
```

When the name is not specified, the key is used as name. If the name
is specified, it must not exist already - neither as a key, nor as a
name for another setting, nor as a previously-known name.

If later, it becomes necessary to change the name again, the
`WithRetiredName` option can be used. This is necessary to properly
redirect users to the new name.

For example:
```go
var s = RegisterDurationSetting(
  "server.web_session_timeout", // <- this is the key; it is immutable.
  settings.WithName("http.web_session.timeout"), // <- new user-visible name
  settings.WithRetiredName("server.web_session.timeout"), // <- past name
)
```

A pgwire notice is sent to the user if they attempt to use SHOW/SET
CLUSTER SETTING using a retired name.

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
@craig craig bot closed this as completed in 4698cfe Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@knz knz

Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

settings,*: give better names to certain settings knz/cockroach
1 participant
@knz