-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[store] Add response hints to label APIs #3437
Conversation
b236112
to
69e7faf
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 @gouthamve! LGTM. Could you take a look at the comments, fix tests and rebase master
, please?
cffb972
to
342db76
Compare
Fixes #2984 |
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.
LGTM, thanks!
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.
Nice, work! Minor style nit only, otherwise LGTM 👍
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
Fixes thanos-io#2984 Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
18afd9f
to
a437244
Compare
@bwplotka Nice catch, not sure what I was thinking there. Fixed it! |
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.
LGTM, we can ignore Markdown check link, it's flaky, otherwise, we can merge on green
* Add hints support to labels API responses Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com> * Move to dedicated hints types and add tests Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com> * Add external labels to the values returned in API Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com> * Add changelog entries Fixes thanos-io#2984 Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com> Signed-off-by: Oghenebrume50 <raphlbrume@gmail.com>
Changes
In Cortex, we shard blocks b/w store-gateways, hence we need to send the list of blockids to query and the list of ids that have been queried in the response to make sure all the relevant blocks are queried. This has already been done for the
Series
API.I have not yet added request hints yet. I will add them in a subsequent PR.
Verification
tbh, I have not tested it yet. I am only relying on the unit tests. Any tips on how I can test it?