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

server: ignore spanconfig limit for shared-process tenants #99918

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

stevendanna
Copy link
Collaborator

@stevendanna stevendanna commented Mar 29, 2023

This change installs a noop spanconfig limiter when starting shared-process tenants. Since shared-process tenants are currently used for "unified architecture" in which the tenant is expected to behave as close as possible to the system tenant.

When we have a tenant-side capabilities reader, we should tie this to a capability instead.

This has an unfortunate side-effect of making any tenant-level override for the span limit a lie for shared-process tenants.

Fixes #93562.
Fixes #93561.

Release note: None

This change installs a noop spanconfig limiter when starting
shared-process tenants.  Since shared-process tenants are currently
used for "unified architecture" in which the tenant is expected to
behave as close as possible to the system tenant.

When we have a tenant-side capabilities reader, we should tie this to
a capability instead.

This has an unfortunate side-effect of making any tenant-level
override for the span limit a lie for shared-process tenants.

Fixes cockroachdb#93562

Release note: None
@stevendanna stevendanna requested a review from a team as a code owner March 29, 2023 10:49
@stevendanna stevendanna requested a review from a team March 29, 2023 10:49
@stevendanna stevendanna requested review from a team as code owners March 29, 2023 10:49
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)


pkg/server/tenant.go line 187 at r1 (raw file):

		instanceIDContainer:   baseCfg.IDContainer.SwitchToSQLIDContainerForStandaloneSQLInstance(),
		costControllerFactory: NewTenantSideCostController,
		spanLimiterFactory: func(ie isql.Executor, st *cluster.Settings, knobs *spanconfig.TestingKnobs) spanconfig.Limiter {

Does spanLimiterFactory: spanconfiglimiter.New not work?

@knz knz added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Mar 29, 2023
Copy link
Collaborator Author

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)


pkg/server/tenant.go line 187 at r1 (raw file):

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

Does spanLimiterFactory: spanconfiglimiter.New not work?

spanconfiglimiter.New returns the concrete type.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

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


pkg/server/tenant.go line 187 at r1 (raw file):

Previously, stevendanna (Steven Danna) wrote…

spanconfiglimiter.New returns the concrete type.

Cheers 👍

@stevendanna
Copy link
Collaborator Author

bors r=knz

@craig
Copy link
Contributor

craig bot commented Mar 29, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 30, 2023

Build failed:

@knz
Copy link
Contributor

knz commented Mar 30, 2023

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 30, 2023

Build succeeded:

@stevendanna
Copy link
Collaborator Author

blathers backport 23.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
3 participants