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

Add limit for store gateway downloaded bytes #5179

Merged
merged 4 commits into from
Apr 28, 2023

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Feb 27, 2023

What this PR does:

Add the limit from thanos-io/thanos#5801 to Cortex. This is a store gateway only limit that limits all data downloaded for each gRPC request to store gateway.

Checklist

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

@yeya24 yeya24 marked this pull request as draft February 27, 2023 05:30
@alvinlin123
Copy link
Member

He @yeya24 do we really need this in addition to max_fetched_data_bytes_per_query? I am trying to understand how max_downloaded_bytes_per_request in store-gateway would work with other Cortex limits related to size of query.

@yeya24
Copy link
Contributor Author

yeya24 commented Feb 28, 2023

It is kind of tricky. How Store Gateway works is that based on the matchers of user query, it will try to fetch postings either from cache or from S3.
Users can write very high cardinality matchers to match several GB postings but end up matching no or few series. In this case max_fetched_data_bytes_per_query won't help since it only considers series and chunks bytes that match the result.

I think this limit can cover some edge cases but I am also not sure if it is good to have that. As we don't have the visibility for that and it is enforced per gRPC request, not per query.

@harry671003
Copy link
Contributor

harry671003 commented Mar 7, 2023

I think this limit can cover some edge cases but I am also not sure if it is good to have that. As we don't have the visibility for that and it is enforced per gRPC request, not per query.

I think this limit is still useful to protect store-gateways from getting OOM killed. We enforce the max_fetched_data_bytes_per_query at the query level in the queriers. The max_downloaded_bytes_per_request should be set equal to the max_fetched_data_bytes_per_query limit.

@yeya24 yeya24 marked this pull request as ready for review April 26, 2023 22:33
@alanprot
Copy link
Member

LGTM!

In the future we may consider deprecating the other ones? Did it help to prevent OOM?

@yeya24
Copy link
Contributor Author

yeya24 commented Apr 28, 2023

In the future we may consider deprecating the other ones? Did it help to prevent OOM?

What are the other ones you mentioned here? The series and chunk limit on store gateway?

Signed-off-by: Ben Ye <benye@amazon.com>
Signed-off-by: Ben Ye <benye@amazon.com>
Signed-off-by: Ben Ye <benye@amazon.com>
Signed-off-by: Ben Ye <benye@amazon.com>
@yeya24
Copy link
Contributor Author

yeya24 commented Apr 28, 2023

Merging it now.

@yeya24 yeya24 merged commit 169a062 into cortexproject:master Apr 28, 2023
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.

4 participants