settings,*: use the option pattern with the setting register functions #109012

knz · 2023-08-18T16:11:07Z

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

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

For example, this call:

var s = func() *settings.BoolSetting {
  s := settings.RegisterBoolSetting(...).WithPublic()
  s.SetReportable(true)
}()

can now be written as:

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:

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

cockroach-teamcity · 2023-08-18T16:11:21Z

This change is

yuzefovich

Reviewed 159 of 159 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @stevendanna)

-- commits line 98 at r2:
Should we modify "NonNegative...WithMinimum" to be "...WithMinimum"? It seems somewhat redundant to have both "non-negative" (which implies "with minimum zero") and "with minimum". It seems like we always have the minimum value greater than zero, so it's non-negative by definition.

pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go line 104 at r2 (raw file):

// checkDurationInRange returns a function used to validate duration cluster
// settings. Because these values are currently settable by the tenant, we need

nit: perhaps preserve the second sentence as the comments on where settings.DurationInRange is now used.

pkg/ccl/streamingccl/streamingest/stream_ingestion_processor.go line 56 at r2 (raw file):

	"the minimum timestamp between flushes; flushes may still occur if internal buffers fill up",
	5*time.Second,
	settings.WithPublic,

This wasn't public, was this change intentional?

pkg/settings/byte_size.go line 54 at r2 (raw file):

		hasExplicitValidationFn := false
		for _, opt := range opts {
			if opt.validateProtoFn != nil ||

nit: should we flip these checks from "blocklist" to "allowlist"? I.e. we'd only allow commonOpt (which is skipped over) and validateInt64Fn (which is called) to be non-nil, and we would panic in all other cases. This seems more bullet-proof in case new validation options are added (and a bit less verbose too).

pkg/settings/float.go line 220 at r2 (raw file):

// FractionUpperExclusive requires the setting to be in the interval
// [0, 1]. It can be passed to RegisterFloatSetting.

nit: I think we usually highlight "upper exclusive" as [0, 1) interval.

pkg/settings/options.go line 1 at r2 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: s/2021/2023/.

pkg/settings/options.go line 52 at r2 (raw file):

// retiredSettings map (in settings/registry.go). The Retired option
// exists for cases where there is a need to maintain compatibility
// with autommation.

nit: s/autommation/automation/.

pkg/sql/storageparam/tablestorageparam/table_storage_param.go line 553 at r2 (raw file):

	return func(intVal int64) error {
		if intVal < 0 {
			return errors.Newf("cannot be set to a negative interger: %d", intVal)

nit: s/interger/integer/.

pkg/util/admission/scheduler_latency_listener.go line 223 at r2 (raw file):

		"sets the floor on per-node elastic work CPU % utilization",
		0.05, // 5%
		settings.FloatInRange(0.05, 1.0),

Here minimum used to 0.01.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna and @yuzefovich)

-- commits line 98 at r2:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Should we modify "NonNegative...WithMinimum" to be "...WithMinimum"? It seems somewhat redundant to have both "non-negative" (which implies "with minimum zero") and "with minimum". It seems like we always have the minimum value greater than zero, so it's non-negative by definition.

There's a lot of uses of the other NonNegative functions already. I felt the patch was already big enough as it is.

If you insist, I can do this - it's a simple search-and-replace.

pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go line 104 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: perhaps preserve the second sentence as the comments on where settings.DurationInRange is now used.

It was inaccurate - since this was written the class was changed to TenantReadOnly.

pkg/ccl/streamingccl/streamingest/stream_ingestion_processor.go line 56 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

This wasn't public, was this change intentional?

It was public (RegisterPublicDurationSetting... in the original call)

pkg/settings/byte_size.go line 54 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: should we flip these checks from "blocklist" to "allowlist"? I.e. we'd only allow commonOpt (which is skipped over) and validateInt64Fn (which is called) to be non-nil, and we would panic in all other cases. This seems more bullet-proof in case new validation options are added (and a bit less verbose too).

Can you show me how you would write that? I don't see it.

pkg/settings/float.go line 220 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I think we usually highlight "upper exclusive" as [0, 1) interval.

Done.

pkg/settings/options.go line 1 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: s/2021/2023/.

Done.

pkg/settings/options.go line 52 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: s/autommation/automation/.

Done.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna and @yuzefovich)

pkg/sql/storageparam/tablestorageparam/table_storage_param.go line 553 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: s/interger/integer/.

Done.

pkg/util/admission/scheduler_latency_listener.go line 223 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Here minimum used to 0.01.

Done

yuzefovich

Reviewed 4 of 163 files at r3, 161 of 161 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz and @stevendanna)

-- commits line 98 at r2:

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

There's a lot of uses of the other NonNegative functions already. I felt the patch was already big enough as it is.

If you insist, I can do this - it's a simple search-and-replace.

In other use cases "non-negative" prefix makes sense since it's complimentary to the other constraint imposed by the validation function (or it's the only constraint), but "non-negative with minimum" is a bit redundant. I don't feel strongly about this to insist, so I'll leave it up to you.

pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go line 104 at r2 (raw file):

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

It was inaccurate - since this was written the class was changed to TenantReadOnly.

Ack, makes sense.

pkg/ccl/streamingccl/streamingest/stream_ingestion_processor.go line 56 at r2 (raw file):

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

It was public (RegisterPublicDurationSetting... in the original call)

Right, missed that, nvm.

pkg/settings/byte_size.go line 54 at r2 (raw file):

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

Can you show me how you would write that? I don't see it.

I was thinking something like

switch {
  case opt.commonOpt != nil:
    // not a validation option, skip it
  case opt.validateInt64Fn != nil:
    hasExplicitValidationFn = true
    ... fn(v)
  default:
    panic(errors.AssertionFailedf("wrong validator type"))
}

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @stevendanna and @yuzefovich)

-- commits line 98 at r2:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

In other use cases "non-negative" prefix makes sense since it's complimentary to the other constraint imposed by the validation function (or it's the only constraint), but "non-negative with minimum" is a bit redundant. I don't feel strongly about this to insist, so I'll leave it up to you.

Done.

pkg/settings/byte_size.go line 54 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I was thinking something like

switch {
  case opt.commonOpt != nil:
    // not a validation option, skip it
  case opt.validateInt64Fn != nil:
    hasExplicitValidationFn = true
    ... fn(v)
  default:
    panic(errors.AssertionFailedf("wrong validator type"))
}

nice. Done.

knz · 2023-08-19T09:05:04Z

TFYR!

bors r=yuzefovich

knz · 2023-08-19T09:38:33Z

bors r-

craig · 2023-08-19T09:38:35Z

Canceled.

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

knz · 2023-08-19T09:42:38Z

bors r=yuzefovich

craig · 2023-08-19T10:18:22Z

Build succeeded:

Bazel Essential CI (Cockroach)

knz requested review from a team August 18, 2023 16:11

knz requested review from a team as code owners August 18, 2023 16:11

knz requested a review from a team August 18, 2023 16:11

knz requested a review from a team as a code owner August 18, 2023 16:11

knz requested a review from a team August 18, 2023 16:11

knz requested review from a team as code owners August 18, 2023 16:11

knz requested review from Santamaura, lidorcarmel, jbowens, HonoreDB and yuzefovich and removed request for a team August 18, 2023 16:11

knz removed request for a team August 18, 2023 16:11

knz requested review from stevendanna and yuzefovich and removed request for a team, jbowens, DrewKimball, lidorcarmel and Santamaura August 18, 2023 16:11

knz force-pushed the 20230818-setting-api branch 3 times, most recently from 864ba9f to dd64e29 Compare August 18, 2023 16:51

yuzefovich reviewed Aug 18, 2023

View reviewed changes

knz force-pushed the 20230818-setting-api branch from dd64e29 to 0c75f3c Compare August 18, 2023 21:03

knz commented Aug 18, 2023

View reviewed changes

knz force-pushed the 20230818-setting-api branch from 0c75f3c to 3c48c1e Compare August 18, 2023 21:21

knz commented Aug 18, 2023

View reviewed changes

knz mentioned this pull request Aug 18, 2023

settings: allow setting names to diverge from the key #109048

Merged

yuzefovich approved these changes Aug 18, 2023

View reviewed changes

knz force-pushed the 20230818-setting-api branch from 3c48c1e to 094740b Compare August 19, 2023 08:12

knz commented Aug 19, 2023

View reviewed changes

knz force-pushed the 20230818-setting-api branch from 094740b to 7fd29ff Compare August 19, 2023 09:02

knz force-pushed the 20230818-setting-api branch from 7fd29ff to 7e4233d Compare August 19, 2023 09:42

craig bot merged commit 0352a78 into cockroachdb:master Aug 19, 2023

knz deleted the 20230818-setting-api branch August 19, 2023 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

settings,*: use the option pattern with the setting register functions #109012

settings,*: use the option pattern with the setting register functions #109012

knz commented Aug 18, 2023 •

edited

Loading

cockroach-teamcity commented Aug 18, 2023

yuzefovich left a comment

knz left a comment

knz left a comment

yuzefovich left a comment

knz left a comment

knz commented Aug 19, 2023

knz commented Aug 19, 2023

craig bot commented Aug 19, 2023

knz commented Aug 19, 2023

craig bot commented Aug 19, 2023

settings,*: use the option pattern with the setting register functions #109012

settings,*: use the option pattern with the setting register functions #109012

Conversation

knz commented Aug 18, 2023 • edited Loading

cockroach-teamcity commented Aug 18, 2023

yuzefovich left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

knz commented Aug 19, 2023

knz commented Aug 19, 2023

craig bot commented Aug 19, 2023

knz commented Aug 19, 2023

craig bot commented Aug 19, 2023

knz commented Aug 18, 2023 •

edited

Loading