Remove -store.query-chunk-limit #705
Conversation
| # CLI flag: -querier.max-fetched-chunks-per-query | ||
| [max_fetched_chunks_per_query: <int> | default = 0] | ||
| [max_fetched_chunks_per_query: <int> | default = 2000000] |
There was a problem hiding this comment.
I wasn't sure if changing this default was correct or not. max_fetched_chunks_per_query previously could not have had a default, since if it was set it would override max_chunks_per_query. With that entanglement now gone, I thought it made sense to inherit the default rather than be left unlimited.
There was a problem hiding this comment.
I think setting this to the 2M is limit is correct for how this this limit behaves today. In blocks_store_queryable we call MaxChunksPerQueryFromStore which would call the limits.MaxChunksPerQueryFromStore code below. Since we have max_fetched_chunks_per_query set to 0 (disabled) by default with the logic in MaxChunksPerQueryFromStore we would get the max_chunks_per_query limit of 2M since max_fetched_chunks_per_query is not greater than 0.
However, I think the original intention was for this limit to be disabled by default. So maybe 0 is what we want here. 2M chunks seems to be a large amount of chunks so I don't really see us hitting that limit often if ever today so I don't think it would effect anything to disable this limit.
I'm also only looking at the blocks path here since the intention is to remove the chunks storage path.
There was a problem hiding this comment.
Actually in looking at the original PR I see Marco say:
(in my opinion having the limit applied to ingesters too is better than having a true global limit, considering ingesters are currently unbounded, but I'm open to feedback).
So maybe the intention here is to make sure that limit is carried through to introduce a limit to the ingesters and storegateway, so then 2M would be correct as the default.
Previously -querier.max-fetched-chunks-per-query was introduced to limit chunks fetched from long term storage and ingesters. This caused -store.query-chunk-limit to be deprecated, since it only limited chunks from long term storage. Now that some time has passed since this deprecation, it would be nice to have one flag again.
andyasp
force-pushed
the
aasp/remove-query-chunk-limit
branch
from
c51f960 to
7eeeaeb
Compare
| @@ -360,7 +360,7 @@ func (c *store) getMetricNameChunks(ctx context.Context, userID string, from, th | |||
| filtered := filterChunksByTime(from, through, chunks) | |||
| level.Debug(log).Log("Chunks post filtering", len(chunks)) | |||
|
|
|||
| maxChunksPerQuery := c.limits.MaxChunksPerQueryFromStore(userID) | |||
| maxChunksPerQuery := c.limits.MaxChunksPerQuery(userID) | |||
There was a problem hiding this comment.
Rather than fixing this code (and other files in this package), we should remove it. But until then, this is fine.
There was a problem hiding this comment.
Yeah I agree, it felt a little fruitless making some of these edits with the removal pending. I did it in this order to help myself understand it in isolation. I've been looking at the query path removal story, so if that goes well hopefully most of the package will be deleted soon.
What this PR does:
Previously
-querier.max-fetched-chunks-per-querywas introduced to limit chunks fetched from long term storage and ingesters. This caused-store.query-chunk-limitto be deprecated, since it only limited chunks from long term storage.Now that some time has passed since this deprecation, it would be nice to have one flag again. For reference, the deprecation was done in Cortex PR 4125 (not sure if I should link directly).
Which issue(s) this PR fixes:
Fixes #12
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]