setting: introduce setting classes by RaduBerinde · Pull Request #73937 · cockroachdb/cockroach

RaduBerinde · 2021-12-16T18:58:51Z

This commit introduces the three setting classes in the RFC (#73349):
SystemOnly, TenantReadOnly, and TenantWritable. The SystemOnly
class replaces the existing WithSystemOnly().

In this change we don't yet implement the advertised semantics. We
mechanically use TenantWritable for all settings except those that
were using WithSystemOnly() which use SystemOnly; this should not
change any existing behavior. The classes will be revisited in a
separate change, after we implement the semantics.

Release note: None

cockroach-teamcity · 2021-12-16T18:58:59Z

This change is

catj-cockroach

Reviewed 162 of 162 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @knz)

miretskiy · 2021-12-16T21:31:45Z

as far as mechanical change concerned for changefeed code, but please see my comment re API.

miretskiy · 2021-12-16T21:31:09Z

pkg/settings/setting.go

+	// 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


just curios: is the purpose of this API change to ensure that each cluster setting is annotated with appropriate scope and that there is no way to forget to do so, like it'd be possible if "WithSystemOnly" was augmented with WithTeanantReadonly?

Have we considered linters perhaps to ensure that With* always applied when registering settings to enforce that (as an alternative to this change)?

Is it possible that at some point we would want to express more complex class of settings (perhaps TennantWritable that's not a System visible or overridable)? Seems like the "With*" approach might be more flexible...

dt · 2021-12-16T21:38:40Z

Is there really a class-difference between TenantReadOnly and TenantWritable in code? isn't just sticking an override in the overrides table on the host cluster sufficient to make any setting TenantReadOnly?

RaduBerinde

Is there really a class-difference between TenantReadOnly and TenantWritable in code? isn't just sticking an override in the overrides table on the host cluster sufficient to make any setting TenantReadOnly?

There will be a difference when there is no override. Having to maintain server-side overrides for all settings that we really don't want tenants to change (e.g. tenant_cpu_usage_allowance) is more error-prone. Also, these classes better document the expected usage.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @knz, and @miretskiy)

pkg/settings/setting.go, line 106 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

just curios: is the purpose of this API change to ensure that each cluster setting is annotated with appropriate scope and that there is no way to forget to do so, like it'd be possible if "WithSystemOnly" was augmented with WithTeanantReadonly?

Have we considered linters perhaps to ensure that With* always applied when registering settings to enforce that (as an alternative to this change)?

Is it possible that at some point we would want to express more complex class of settings (perhaps TennantWritable that's not a System visible or overridable)? Seems like the "With*" approach might be more flexible...

Yes, it is to force you to think about the proper class whenever you add a setting. What would be the advantage of forcing use of .WithSomething() with a linter? This syntax is cleaner and more concise.

Generally the .WithSomething() things should be used when there is a reasonable default (preferably that's used for most settings).

Adding more classes should be similar with both patterns. I don't see us having "parameterized" classes that require arguments (though you could make that work in this register-against-the-class model too).

miretskiy · 2021-12-16T22:57:32Z

Yes, it is to force you to think about the proper class whenever you add a setting. What would be the advantage of forcing use of .WithSomething() with a linter? This syntax is cleaner and more concise.

Well, I can think of few cases. For one, it's not changing the existing API, so, the familiar usage patterns
remain. I'm not too sure about syntax being cleaner or more concise though. I guess it is in the eye of a beholder; but
since we are still keeping e.g. WithPublic, etc I would argue that having 1 mechanism for a builder like pattern is probably
preferable to having 2 (with 2 patterns being setting.XXX which is a builder for "class", followed by more builders WithPublic, etc).

Generally the .WithSomething() things should be used when there is a reasonable default (preferably that's used for most settings).
Adding more classes should be similar with both patterns. I don't see us having "parameterized" classes that require arguments (though you could make that work in this register-against-the-class model too).

Well, I guess that was the crux of my question. I prefer With because it maintains status quo (wrt to the APIs -- just one builder like pattern), but I understand the need to document the type of setting... So, I suggested a linter.

But come to think, if you're changing 100s of call sites anyway, why not just have an argument for settings class when registering? RegisterBoolSetting(settings.ForTenant, ...)?

RaduBerinde · 2021-12-17T00:54:28Z

I'm pretty strongly against using With for non-optional arguments (imagine if we used that pattern for eg the description).

I started with the plan to make it an argument, but thought this way was nicer (and more concise, only one "settings."). Obviously it is subjective, and I don't mind switching if folks prefer an argument. But I'd like to understand why do you prefer it? Is calling against the class confusing / too magicy? As a general note, when suggesting an alternative, you should say why you prefer it rather than asking "why not?" :)

docs/RFCS/20211106_multitenant_cluster_settings.md

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @catj-cockroach, @dt, @knz, and @miretskiy)

docs/RFCS/20211106_multitenant_cluster_settings.md, line 220 at r1 (raw file):

Previously, dt (David Taylor) wrote…

apologies for this off-topic drive-by comment, that is also late and should have been raised on the RFC (so feel free to just hit "Resolve Converation" here if this is too off-topic for this pr), but should't this "when in doubt the first choice" be tenant-rw, not tenant-ro? like, by default a tenant should have the same controls and capabilities as any other user over the behavior of their cluster, and we should only reach for the more restrictive ones, that apply to tenants differently than non-tenants, when we need them, right? particularly in the the world where we can override from host with a value at runtime if we need to with no code change, even if it is tenant-rw in the code?

Agreed, I think I wrote that before we added overrides for all of them.

miretskiy · 2021-12-17T13:11:41Z

I started with the plan to make it an argument, but thought this way was nicer (and more concise, only one "settings."). Obviously it is subjective, and I don't mind switching if folks prefer an argument. But I'd like to understand why do you prefer it? Is calling against the class confusing / too magicy?

As a general note, when suggesting an alternative, you should say why you prefer it rather than asking "why not?" :)

Allow me to disagree. I think this PR proposes an alternative mechanism to already well established pattern of using With, as well as an alternative to a simple function argument. So, I really think the onus is on PR author to explain "why not" use existing mechanisms and introduce a new one.

Few more reasons (some stylistical) for me are:

I think setting.TennatnWritable. a bit more verbose -- though maybe it's a plus.
If used as a function argument, it's easy to see how such system could be extended. Adding a new class is trivial; even changing those settings to be bitmasks if we need to to express more complex setting properties.
Function arguments are a good mechanism to express "this property is required" -- just like setting name.

At any rate, I already gave LG -- so, it's more about understanding why we haven't used existing mechanisms.
I would also be interested to hear what other people think.

RaduBerinde · 2021-12-17T14:07:22Z

It was agreed (as part of the RFC) that the class would be mandatory when defining a setting, so that IMO rules out the With mechanism which is designed for optional modifiers. As for using an argument, those are reasonable points; thanks for spelling them out. I am ok switching to an argument but I want to hear what @dt, @knz, @ajwerner think before I go back modifying 500 call sites.

dt · 2021-12-17T14:35:24Z

hear what @dt

No strong opinions -- I suppose if I have to choose, I have mild preference for a regular required function argument vs the receiver factories. (while aesthetically it feels like it'd go after description, putting it first, before the name arg, could keep this a simple find/replace, since we'd just need to put settings.TenantWritable after the open paren?)

RaduBerinde · 2021-12-17T21:23:28Z

Right, the proposed change is:

-	intervalBaseSetting = settings.TenantWritable.RegisterFloatSetting(
+	intervalBaseSetting = settings.RegisterFloatSetting(
+		settings.TenantWritable,
 		intervalBaseSettingKey,
 		"the base multiplier for other intervals such as adopt, cancel, and gc",
 		defaultIntervalBase,

RaduBerinde · 2021-12-20T21:10:00Z

Ok, I updated the change to pass the class as the first argument.

This commit introduces the three setting classes in the RFC (cockroachdb#73349): `SystemOnly`, `TenantReadOnly`, and `TenantWritable`. The `SystemOnly` class replaces the existing `WithSystemOnly()`. In this change we don't yet implement the advertised semantics. We mechanically use `TenantWritable` for all settings except those that were using `WithSystemOnly()` which use `SystemOnly`; this should not change any existing behavior. The classes will be revisited in a separate change, after we implement the semantics. Release note: None

miretskiy · 2021-12-21T15:03:15Z

knz

Reviewed 1 of 1 files at r2, 154 of 156 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @miretskiy)

RaduBerinde · 2021-12-21T17:04:58Z

TFTRs!

bors r+

craig · 2021-12-21T18:42:30Z

Build succeeded:

GitHub CI (Cockroach)

RaduBerinde requested review from a team, ajwerner, dt and knz December 16, 2021 18:58

RaduBerinde requested review from a team as code owners December 16, 2021 18:58

RaduBerinde requested a review from a team December 16, 2021 18:58

RaduBerinde requested a review from a team as a code owner December 16, 2021 18:58

RaduBerinde requested a review from a team December 16, 2021 18:58

RaduBerinde requested review from a team as code owners December 16, 2021 18:58

catj-cockroach approved these changes Dec 16, 2021

View reviewed changes

miretskiy reviewed Dec 16, 2021

View reviewed changes

RaduBerinde commented Dec 16, 2021

View reviewed changes

dt approved these changes Dec 17, 2021

View reviewed changes

docs/RFCS/20211106_multitenant_cluster_settings.md Outdated Show resolved Hide resolved

RaduBerinde force-pushed the setting-classes branch from b825315 to b84f52d Compare December 17, 2021 03:06

RaduBerinde commented Dec 17, 2021

View reviewed changes

RaduBerinde force-pushed the setting-classes branch from b84f52d to 00d9fac Compare December 20, 2021 21:09

RaduBerinde force-pushed the setting-classes branch from 00d9fac to 52c2df4 Compare December 20, 2021 21:57

knz approved these changes Dec 21, 2021

View reviewed changes

craig bot merged commit 779f08b into cockroachdb:master Dec 21, 2021

RaduBerinde deleted the setting-classes branch December 21, 2021 21:02

RaduBerinde mentioned this pull request Feb 17, 2022

settings: multi-tenant cluster settings tracking issue #73857

Closed

16 tasks

Conversation

RaduBerinde commented Dec 16, 2021

Uh oh!

cockroach-teamcity commented Dec 16, 2021

Uh oh!

catj-cockroach left a comment

Choose a reason for hiding this comment

Uh oh!

miretskiy commented Dec 16, 2021 • edited by petermattis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miretskiy Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

dt commented Dec 16, 2021

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

miretskiy commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RaduBerinde commented Dec 17, 2021

Uh oh!

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

miretskiy commented Dec 17, 2021

Uh oh!

RaduBerinde commented Dec 17, 2021

Uh oh!

dt commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RaduBerinde commented Dec 17, 2021

Uh oh!

RaduBerinde commented Dec 20, 2021

Uh oh!

miretskiy commented Dec 21, 2021 • edited by bdarnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde commented Dec 21, 2021

Uh oh!

craig bot commented Dec 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

miretskiy commented Dec 16, 2021 •

edited by petermattis

Loading

miretskiy commented Dec 16, 2021 •

edited

Loading

dt commented Dec 17, 2021 •

edited

Loading

miretskiy commented Dec 21, 2021 •

edited by bdarnell

Loading