-
Notifications
You must be signed in to change notification settings - Fork 529
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
Conversation
The 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
07fcedb
to
866853a
Compare
…go to the storegateway package
0502930
to
62cec6a
Compare
For the rest, let's discuss it next week after you have reviewed the code. |
7a18e80
to
0d40426
Compare
@@ -3,7 +3,7 @@ | |||
// Provenance-includes-license: Apache-2.0 | |||
// Provenance-includes-copyright: The Cortex Authors. | |||
|
|||
package querier | |||
package storegateway |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
…and implement review findings
There was a problem hiding this 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.
dcb4b81
to
d6ea271
Compare
There was a problem hiding this 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.
b709314
to
aebc968
Compare
da91bcd
to
ee65582
Compare
b2542d9
to
d488c6f
Compare
Signed-off-by: Marco Pracucci <marco@pracucci.com>
What this PR does
This PR introduces the following changes:
Which issue(s) this PR fixes or relates to
Extension of the fix for issue #3382
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]