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

issues/5253 convert cache-unaligned-requests to per-tenant config #5312

Merged

Conversation

francoposa
Copy link
Member

@francoposa francoposa commented Jun 22, 2023

What this PR does

Moves cache_unaligned_requests into the limits, so that it can be overridden per-tenant.

Allows operators to turn this on for the rare tenants that will actually need it, without ballooning the cache for everyone else in a multi-tenant cell that does not need this.

Global parameter cacheUnalignedRequests was removed from splitAndCacheMiddleware in favor of access to limits helper functions.

Which issue(s) this PR fixes or relates to

Fixes #5253

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pkg/frontend/querymiddleware/roundtrip.go Outdated Show resolved Hide resolved
pkg/util/validation/limits.go Outdated Show resolved Hide resolved
pkg/util/validation/limits.go Show resolved Hide resolved
@francoposa francoposa force-pushed the francoposa/issues/5253/cache-unaligned-requests-per-tenant branch from f9bb744 to 833b45c Compare June 29, 2023 14:47
@francoposa francoposa marked this pull request as ready for review June 29, 2023 16:08
@francoposa francoposa requested review from a team as code owners June 29, 2023 16:08
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you. Left comment with one detail to fix, lgtm otherwise.

CacheResults bool `yaml:"cache_results"`
MaxRetries int `yaml:"max_retries" category:"advanced"`
ShardedQueries bool `yaml:"parallelize_shardable_queries"`
DeprecatedCacheUnalignedRequests bool `yaml:"cache_unaligned_requests" category:"advanced" doc:"hidden"` // TODO: Deprecated in Mimir 2.10.0, remove in Mimir 2.12.0
Copy link
Member

Choose a reason for hiding this comment

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

When someone sets this field in their config, we should modify the default limit value.

We should do that in initRuntimeConfig, before calling validation.SetDefaultLimitsForYAMLUnmarshalling, similar to

mimir/pkg/mimir/modules.go

Lines 273 to 279 in 52d9875

// QueryIngestersWithin is moving from a global config that can in the querier yaml to a limit config
// We need to preserve the option in the querier yaml for two releases
// If the querier config is configured by the user, the default limit is overwritten
// TODO: Remove in Mimir 2.11.0
if t.Cfg.Querier.QueryIngestersWithin != querier.DefaultQuerierCfgQueryIngestersWithin {
t.Cfg.LimitsConfig.QueryIngestersWithin = model.Duration(t.Cfg.Querier.QueryIngestersWithin)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

this is addressed now;

Is there a good place in the docs to put a little playbook for migrating a config option into the limits section?

There are quite a few checklist items for this process, and I would expect this is far from the last time it will be done.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a good place in the docs to put a little playbook for migrating a config option into the limits section?

That's a great idea. We can create short document in docs/internal/contributing directory.

@francoposa francoposa force-pushed the francoposa/issues/5253/cache-unaligned-requests-per-tenant branch from 2494f00 to 11d21f6 Compare June 30, 2023 19:31
…config overrides a default setting for the new limit config until removal in v2.12
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Thank you!

@pstibrany pstibrany merged commit dd61efa into main Jul 4, 2023
28 checks passed
@pstibrany pstibrany deleted the francoposa/issues/5253/cache-unaligned-requests-per-tenant branch July 4, 2023 10:16
56quarters added a commit that referenced this pull request Mar 1, 2024
The setting has per-tenant and moved under limits configuration since
Mimir 2.10

See #5312

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this pull request Mar 1, 2024
…7519)

The setting has been made per-tenant and moved under limits configuration since
Mimir 2.10

See #5312

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
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.

Range query not looked up from the results cache when start=end and step=86400
3 participants