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.

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

rfcs: update the RFC on multi-tenant cluster settings #85970

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

knz
Copy link
Contributor

@knz knz commented Aug 11, 2022

Note: this is a RFC update, not a new RFC. We are evaluating the 'diff' between the previous and the new version.

Informs #85729

Text of RFC: https://github.com/knz/cockroach/blob/20220811-tenant-settings/docs/RFCS/20211106_multitenant_cluster_settings.md

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz force-pushed the 20220811-tenant-settings branch from af42383 to 359d7a5 Compare August 11, 2022 17:27
@knz knz requested review from stevendanna, dt and a team August 11, 2022 17:28
@knz
Copy link
Contributor Author

knz commented Aug 11, 2022

@dt @stevendanna could you help here and provide some examples (1-2 would be sufficient) of cluster settings that would benefit from the new definition of system-ro?

@knz knz marked this pull request as ready for review August 11, 2022 17:28
@knz knz requested a review from a team as a code owner August 11, 2022 17:28
@knz knz changed the title docs: update the RFC on multi-tenant cluster settings rfcs: update the RFC on multi-tenant cluster settings Aug 11, 2022
@knz knz force-pushed the 20220811-tenant-settings branch from 359d7a5 to e8fdb04 Compare August 11, 2022 17:34
@dt
Copy link
Member

dt commented Aug 11, 2022

rangefeeds.enabled is the classic one, where it is a system-only settings (it only controls kvserver behavior) but we want to know its value in sql to provide a better error if it is off and we're about to attempt something that requires it be on.

@ajwerner
Copy link
Contributor

That's my least favorite example because there's such an easy solution to it: make it a zone config instead of a cluster setting.

@dt
Copy link
Member

dt commented Aug 12, 2022

@ajwerner sure, yes, but for now it is a cluster setting ,that (a) controls kv server behavior that (b) SQL behavior wants to know its value, so it fits the bill.

Some hypothetical ones:
kvserver has a cluster setting to limit the number of slots it provides to concurrently process some type of request; observing that limit allows SQL-side senders to adjust their sending concurrency accordingly, as there is no point in them spending more resources to create and send and wait for wildly more requests than the server will possibly handle.

Similar to rangefeeds, if a certain type of kv request has to be enabled/disabled, knowing that fact client-side can allow switching to a different implementation (e.g. poll if you won't be allowed to get a feed, use different backfiller if you can't backdate sstables, etc) or at least supplying a better error.

@ajwerner
Copy link
Contributor

Another one which affects the kvclient and sql layer is the maximum raft command size.

@knz knz force-pushed the 20220811-tenant-settings branch from e8fdb04 to e89bc3d Compare August 15, 2022 09:30
@knz
Copy link
Contributor Author

knz commented Aug 15, 2022

Added the examples in the text. Thanks!

Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I still prefer to conceptually separate discussion of overrides -- a capability common to any tenant-read setting -- from the two types of tenant read setting and how they differ (reading the host's value vs their local value).

But even if we don't do that, the document is clear with this edits as they are now than it was before, so I'm not going to die on this hill.


3. Tenant writable (`tenant-rw`)
- Settings that benefit from being overridden per tenant, but where
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me these aren't so much two distinct categories, but rather one category, plus the same override-capability we have for all tenant-* settings, ro or rw.

I'd describe this category as:

  • Settings where exposing the system tenant's current value -- whether that is coming from its default or has been configured explicitly -- is desirable. For example, clients typically try to limit the size of batches they send to stay within the limits of what the host cluster is configured to process, making it useful to know the current values of kv.raft.command.max_size and kv.bulk_ingest.batch_size in the host cluster.

  • As with all tenant-exposed setting, the value read by a specific or all tenants can be forcibly overridden from the host, which could be useful, for example, as a mitigation measure in an incident to force a tenant who is running several large imports to use a smaller batch size to stop OOMs.

As I sit here and think about it: Is there really any reason tenant_cpu_usage_allowance should actually be tenant-ro versus tenant-rw? Seems like it doesn't really matter that it be tenant-ro if we're just going to override it anyway.

@knz knz force-pushed the 20220811-tenant-settings branch from e89bc3d to 39d13bd Compare September 29, 2023 10:31
@knz
Copy link
Contributor Author

knz commented Sep 29, 2023

i've updated this as per our recent discussions and the work in #111378.

bors r=dt

@craig
Copy link
Contributor

craig bot commented Sep 29, 2023

Build succeeded:

@craig craig bot merged commit 1edd24d into cockroachdb:master Sep 29, 2023
3 checks passed
@knz knz deleted the 20220811-tenant-settings branch September 29, 2023 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants