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

Added -store.max-chunks-per-query limit support to blocks storage #2852

Conversation

pracucci
Copy link
Contributor

@pracucci pracucci commented Jul 8, 2020

What this PR does:
In this PR:

  • Added -store.max-chunks-per-query limit support to blocks storage
  • Removed max-sample-count support because it's flawed in Thanos (it's per block and not per query as documented). I will work on this in a subsequent PR (I need to fix it in Thanos first) and the limit in store-gateway will be based on -store.max-chunks-per-query too.

Out of the scope of this PR:

  • Add the limit to the store-gateway too (will be done in a separate PR, because I need to refactor and fix Thanos first)
  • Add a negative cache to avoid running the same query for the same tenant if we already know it will hit the max chunks limit (issue)

Which issue(s) this PR fixes:
Fixes #2844

Checklist

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

@pracucci pracucci requested a review from pstibrany July 8, 2020 08:39
Copy link
Contributor

@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.

LGTM, good job.

(I'm becoming more nervous about mixing direct and atomic access to the same variable, although in this specific case, there is clear happens-before relationship between the two, so I think it is correct. [Talking about numChunks in fetchSeriesFromStores method])

pracucci added 2 commits July 14, 2020 09:41
…y limit support to blocks storage

Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
@pracucci
Copy link
Contributor Author

LGTM, good job.

Thanks!

(I'm becoming more nervous about mixing direct and atomic access to the same variable, although in this specific case, there is clear happens-before relationship between the two, so I think it is correct. [Talking about numChunks in fetchSeriesFromStores method])

It's safe because of the g.Wait() before reading numChunks, but let's be consistent: I've committed the usage of atomic.LoadInt32() when reading it in the return too.

@pracucci pracucci force-pushed the add-max-chunks-per-query-support-to-blocks-storage branch from b45bd5d to 4d68aaa Compare July 14, 2020 07:44
@pstibrany
Copy link
Contributor

(I'm becoming more nervous about mixing direct and atomic access to the same variable, although in this specific case, there is clear happens-before relationship between the two, so I think it is correct. [Talking about numChunks in fetchSeriesFromStores method])

To expand on this comment, I think my reasoning is wrong. Program happens-before order, and atomic accesses are independent, so correct version of the code should use atomic load at the end of fetchSeriesFromStores. It would be cleaner to use uber-go/atomic package instead for this.

But no need to change it in this PR... we have similar code elsewhere.

@pstibrany
Copy link
Contributor

It's safe because of the g.Wait() before reading numChunks

This is what I thought as well. Unfortunately, there is no such guarantee.

@pracucci pracucci merged commit 2b41aa3 into cortexproject:master Jul 14, 2020
@pracucci pracucci deleted the add-max-chunks-per-query-support-to-blocks-storage branch July 14, 2020 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MaxChunksPerQuery support to blocks storage
2 participants