max chunks per query limit shared between ingesters and storage gateways #4260

alanprot · 2021-06-03T23:57:52Z

What this PR does:
Address #4255
Which issue(s) this PR fixes:
Fixes #4255

Checklist

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

treid314

Thanks! Looks good! Had a few comments on configuration options and some other small things. I also want to call out that we should refactor and remove the blocksStoreLimitsMock from the Blocks store queryable tests. I don't think that should be required for this PR but it would be nice.

pkg/util/validation/limits.go

pkg/util/limiter/query_limiter.go

pkg/querier/querier.go

pkg/querier/blocks_store_queryable_test.go

treid314 · 2021-06-21T19:55:09Z

CHANGELOG.md

@@ -45,7 +45,7 @@
 * [BUGFIX] Fixed cache fetch error on Redis Cluster. #4056
 * [BUGFIX] Ingester: fix issue where runtime limits erroneously override default limits. #4246
 * [BUGFIX] Ruler: fix startup in single-binary mode when the new `ruler_storage` is used. #4252
-
+* [CHANGE] Querier / ruler: Migrate the -querier.max-fetched-chunks-per-query limit to the new QueryLimiter to limit the number of chunks returned as a sum of chunks returned from the ingester and storage gateway.


Can you add the PR number as part of the change log entry.

Also please move it above, grouping it along with other [CHANGE] entries.

bboreham

All good, just need to fix up CHANGELOG.
Note the CHANGELOG message should be from the point of view of the user, who isn't so concerned that we moved code around but does want to know about the change in behaviour.

pracucci

Good job, LGTM! I left a couple of nits. I would be glad if you could address them before merging 🙏

pracucci · 2021-06-22T14:47:57Z

CHANGELOG.md

@@ -45,7 +45,7 @@
 * [BUGFIX] Fixed cache fetch error on Redis Cluster. #4056
 * [BUGFIX] Ingester: fix issue where runtime limits erroneously override default limits. #4246
 * [BUGFIX] Ruler: fix startup in single-binary mode when the new `ruler_storage` is used. #4252
-
+* [CHANGE] Querier / ruler: Migrate the -querier.max-fetched-chunks-per-query limit to the new QueryLimiter to limit the number of chunks returned as a sum of chunks returned from the ingester and storage gateway.


Also please move it above, grouping it along with other [CHANGE] entries.

pracucci · 2021-06-22T14:52:43Z

pkg/util/validation/limits.go

@@ -147,7 +147,7 @@ func (l *Limits) RegisterFlags(f *flag.FlagSet) {
 	f.IntVar(&l.MaxGlobalMetricsWithMetadataPerUser, "ingester.max-global-metadata-per-user", 0, "The maximum number of active metrics with metadata per user, across the cluster. 0 to disable. Supported only if -distributor.shard-by-all-labels is true.")
 	f.IntVar(&l.MaxGlobalMetadataPerMetric, "ingester.max-global-metadata-per-metric", 0, "The maximum number of metadata per metric, across the cluster. 0 to disable.")
 	f.IntVar(&l.MaxChunksPerQueryFromStore, "store.query-chunk-limit", 2e6, "Deprecated. Use -querier.max-fetched-chunks-per-query CLI flag and its respective YAML config option instead. Maximum number of chunks that can be fetched in a single query. This limit is enforced when fetching chunks from the long-term storage only. When running the Cortex chunks storage, this limit is enforced in the querier and ruler, while when running the Cortex blocks storage this limit is enforced in the querier, ruler and store-gateway. 0 to disable.")
-	f.IntVar(&l.MaxChunksPerQuery, "querier.max-fetched-chunks-per-query", 0, "Maximum number of chunks that can be fetched in a single query from ingesters and long-term storage: the total number of actual fetched chunks could be 2x the limit, being independently applied when querying ingesters and long-term storage. This limit is enforced in the ingester (if chunks streaming is enabled), querier, ruler and store-gateway. Takes precedence over the deprecated -store.query-chunk-limit. 0 to disable.")
+	f.IntVar(&l.MaxChunksPerQuery, "querier.max-fetched-chunks-per-query", 0, "Maximum number of chunks that can be fetched in a single query from ingesters and long-term storage. This limit is enforced in the ingester (if chunks streaming is enabled), querier, ruler and store-gateway. Takes precedence over the deprecated -store.query-chunk-limit. 0 to disable.")


It's not enforced in ingesters, but just in the querier component (which is also internally used by the ruler) and store-gateway.

Suggested change

f.IntVar(&l.MaxChunksPerQuery, "querier.max-fetched-chunks-per-query", 0, "Maximum number of chunks that can be fetched in a single query from ingesters and long-term storage. This limit is enforced in the ingester (if chunks streaming is enabled), querier, ruler and store-gateway. Takes precedence over the deprecated -store.query-chunk-limit. 0 to disable.")

f.IntVar(&l.MaxChunksPerQuery, "querier.max-fetched-chunks-per-query", 0, "Maximum number of chunks that can be fetched in a single query from ingesters and long-term storage. This limit is enforced in the querier, ruler and store-gateway. Takes precedence over the deprecated -store.query-chunk-limit. 0 to disable.")

Signed-off-by: Alan Protasio <approtas@amazon.com>

bboreham

Small tidying required in the CHANGELOG

bboreham · 2021-06-24T13:46:40Z

CHANGELOG.md

@@ -59,11 +60,13 @@
 * [BUGFIX] Fixed cache fetch error on Redis Cluster. #4056
 * [BUGFIX] Ingester: fix issue where runtime limits erroneously override default limits. #4246
 * [BUGFIX] Ruler: fix startup in single-binary mode when the new `ruler_storage` is used. #4252
+<<<<<<< HEAD


Bit of merge conflict debris to clean up

Sorry for that :D

CHANGELOG.md

Signed-off-by: Alan Protasio <approtas@amazon.com>

bboreham · 2021-06-29T11:33:58Z

Sorry but we have begun the process of cutting a new release; please rebase from master and move your CHANGELOG entry to the top under ## master / unreleased

pracucci · 2021-06-29T13:58:57Z

Sorry but we have begun the process of cutting a new release; please rebase from master and move your CHANGELOG entry to the top under ## master / unreleased

Just did it.

pull-request-size bot added the size/L label Jun 3, 2021

alanprot force-pushed the fix/4255 branch 5 times, most recently from a122db9 to b0039f7 Compare June 4, 2021 01:01

treid314 suggested changes Jun 4, 2021

View reviewed changes

pkg/util/validation/limits.go Outdated Show resolved Hide resolved

pkg/util/limiter/query_limiter.go Outdated Show resolved Hide resolved

pkg/querier/querier.go Outdated Show resolved Hide resolved

pkg/querier/blocks_store_queryable_test.go Outdated Show resolved Hide resolved

alanprot force-pushed the fix/4255 branch from 7399a1f to b90cba4 Compare June 4, 2021 20:44

treid314 reviewed Jun 21, 2021

View reviewed changes

treid314 approved these changes Jun 21, 2021

View reviewed changes

bboreham approved these changes Jun 22, 2021

View reviewed changes

pracucci approved these changes Jun 22, 2021

View reviewed changes

alanprot force-pushed the fix/4255 branch from 3ff44f8 to 723e6e8 Compare June 24, 2021 01:39

alanprot added 2 commits June 23, 2021 18:42

max chunks per query limit shared between ingesters and storage gateways

727e9ad

Signed-off-by: Alan Protasio <approtas@amazon.com>

Addressing comments

6945134

Signed-off-by: Alan Protasio <approtas@amazon.com>

alanprot force-pushed the fix/4255 branch 2 times, most recently from 6d02a11 to 7ede961 Compare June 24, 2021 01:44

Addressing comments -2

1cf4802

Signed-off-by: Alan Protasio <approtas@amazon.com>

alanprot force-pushed the fix/4255 branch from 7ede961 to 1cf4802 Compare June 24, 2021 02:16

bboreham reviewed Jun 24, 2021

View reviewed changes

treid314 reviewed Jun 24, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Addressing comments - 3

0483e5c

Signed-off-by: Alan Protasio <approtas@amazon.com>

alanprot force-pushed the fix/4255 branch from 6dc77da to 0483e5c Compare June 24, 2021 16:29

treid314 approved these changes Jun 24, 2021

View reviewed changes

Merge branch 'master' into fix/4255

9ffb3cb

pracucci enabled auto-merge (squash) June 29, 2021 13:59

pracucci merged commit 345ec03 into cortexproject:master Jun 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

max chunks per query limit shared between ingesters and storage gateways #4260

max chunks per query limit shared between ingesters and storage gateways #4260

alanprot commented Jun 3, 2021 •

edited

Loading

treid314 left a comment

treid314 Jun 21, 2021

pracucci Jun 22, 2021

bboreham left a comment

pracucci left a comment

pracucci Jun 22, 2021

pracucci Jun 22, 2021

bboreham left a comment

bboreham Jun 24, 2021

alanprot Jun 24, 2021

bboreham commented Jun 29, 2021

pracucci commented Jun 29, 2021

	f.IntVar(&l.MaxChunksPerQuery, "querier.max-fetched-chunks-per-query", 0, "Maximum number of chunks that can be fetched in a single query from ingesters and long-term storage. This limit is enforced in the ingester (if chunks streaming is enabled), querier, ruler and store-gateway. Takes precedence over the deprecated -store.query-chunk-limit. 0 to disable.")
	f.IntVar(&l.MaxChunksPerQuery, "querier.max-fetched-chunks-per-query", 0, "Maximum number of chunks that can be fetched in a single query from ingesters and long-term storage. This limit is enforced in the querier, ruler and store-gateway. Takes precedence over the deprecated -store.query-chunk-limit. 0 to disable.")

max chunks per query limit shared between ingesters and storage gateways #4260

max chunks per query limit shared between ingesters and storage gateways #4260

Conversation

alanprot commented Jun 3, 2021 • edited Loading

treid314 left a comment

Choose a reason for hiding this comment

treid314 Jun 21, 2021

Choose a reason for hiding this comment

pracucci Jun 22, 2021

Choose a reason for hiding this comment

bboreham left a comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

pracucci Jun 22, 2021

Choose a reason for hiding this comment

pracucci Jun 22, 2021

Choose a reason for hiding this comment

bboreham left a comment

Choose a reason for hiding this comment

bboreham Jun 24, 2021

Choose a reason for hiding this comment

alanprot Jun 24, 2021

Choose a reason for hiding this comment

bboreham commented Jun 29, 2021

pracucci commented Jun 29, 2021

alanprot commented Jun 3, 2021 •

edited

Loading