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

Querier: propagate errors with status code 422 coming from the store-gateway #4100

Merged
merged 14 commits into from
Feb 2, 2023

Conversation

duricanikolic
Copy link
Contributor

@duricanikolic duricanikolic commented Jan 27, 2023

What this PR does

This PR introduces the following changes:

  • Makes the querier propagate errors with status code 422 coming from the store-gateway
  • New integration tests have been added in store_gateway_limits_hit_test.go: they ensure that when the max-fetched-chunks-per-query or max-fetched-series-per-query limits are hit, both store-gateway and querier will return a 422 status code

Which issue(s) this PR fixes or relates to

Extension of the fix for issue #3382

Checklist

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

@duricanikolic duricanikolic requested a review from a team as a code owner January 27, 2023 14:16
@pracucci
Copy link
Collaborator

store_gateway_client.go and store_gateway_test.go have been moved from the querier to the storegatewaypb package

The *pb packages should contain only protobufs.

That being said I don't think an integration should use a gRPC client. The idea of integration tests is that they should emulate how users use the system. Let's talk on Slack about it, so we can see if we can find a better approach for the testing here.

…erier hit the max-fetched-series-per-query limit
@duricanikolic duricanikolic force-pushed the yuri/itegration-test-querier-sg-422 branch from 07fcedb to 866853a Compare January 27, 2023 14:26
@duricanikolic duricanikolic force-pushed the yuri/itegration-test-querier-sg-422 branch from 0502930 to 62cec6a Compare January 27, 2023 14:48
@duricanikolic
Copy link
Contributor Author

The *pb packages should contain only protobufs.
@pracucci I have fixed that and moved the 2 sources to the storegateway package instead.

For the rest, let's discuss it next week after you have reviewed the code.

@duricanikolic duricanikolic force-pushed the yuri/itegration-test-querier-sg-422 branch from 7a18e80 to 0d40426 Compare January 27, 2023 16:58
@@ -3,7 +3,7 @@
// Provenance-includes-license: Apache-2.0
// Provenance-includes-copyright: The Cortex Authors.

package querier
package storegateway
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about putting this in storegateway/client, same as we do for the ingester client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@56quarters I had a call with @colega in which we discussed that probably the whole implementation of store_gateway_client.go (currently belonging to the querier, and not storegateway package), as well as of pool.go were done before we started using generics, and therefore the implementation looked a bit more complicated than it should be. The changes I made in store_gateway_client.go were needed for a new test that I introduced, and that will now be removed. So, I will also remove the changes I did there. Nevertheless, we find that it might be appropriate to introduce generics in pool.go and related sources, and to update mimir accordingly. What do you think about that?

pkg/storegateway/store_gateway_client.go Outdated Show resolved Hide resolved
pkg/storegateway/store_gateway_client.go Outdated Show resolved Hide resolved
pkg/querier/blocks_store_replicated_set_test.go Outdated Show resolved Hide resolved
integration/store_gateway_limits_hit_test.go Outdated Show resolved Hide resolved
@duricanikolic duricanikolic changed the title Integration test: return a 422 status code when store-gateway and quierier hit the max-fetched-series-per-query limit Querier: propagate errors with status code 422 coming from the store-gateway Jan 30, 2023
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Good job! The fix LGTM. I left few comments about the integration test cause I think we can further simplify it.

integration/store_gateway_limits_hit_test.go Outdated Show resolved Hide resolved
integration/store_gateway_limits_hit_test.go Outdated Show resolved Hide resolved
integration/store_gateway_limits_hit_test.go Outdated Show resolved Hide resolved
integration/store_gateway_limits_hit_test.go Outdated Show resolved Hide resolved
integration/store_gateway_limits_hit_test.go Outdated Show resolved Hide resolved
integration/store_gateway_limits_hit_test.go Outdated Show resolved Hide resolved
@duricanikolic duricanikolic force-pushed the yuri/itegration-test-querier-sg-422 branch from dcb4b81 to d6ea271 Compare January 31, 2023 16:26
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks for your patience. Left few more comments.

integration/store_gateway_limits_hit_test.go Outdated Show resolved Hide resolved
integration/store_gateway_limits_hit_test.go Outdated Show resolved Hide resolved
integration/store_gateway_limits_hit_test.go Outdated Show resolved Hide resolved
integration/store_gateway_limits_hit_test.go Outdated Show resolved Hide resolved
@duricanikolic duricanikolic force-pushed the yuri/itegration-test-querier-sg-422 branch from b709314 to aebc968 Compare January 31, 2023 21:36
CHANGELOG.md Outdated Show resolved Hide resolved
@duricanikolic duricanikolic force-pushed the yuri/itegration-test-querier-sg-422 branch from da91bcd to ee65582 Compare February 1, 2023 17:07
@duricanikolic duricanikolic force-pushed the yuri/itegration-test-querier-sg-422 branch from b2542d9 to d488c6f Compare February 1, 2023 21:36
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci enabled auto-merge (squash) February 2, 2023 18:19
@pracucci pracucci merged commit d9418f2 into main Feb 2, 2023
@pracucci pracucci deleted the yuri/itegration-test-querier-sg-422 branch February 2, 2023 18:34
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.

None yet

4 participants