-
Notifications
You must be signed in to change notification settings - Fork 543
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
Fix query sharding with too many requests error #2447
Conversation
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.
Broadly seems fine; couple of thoughts below
@@ -438,3 +441,8 @@ func decorateWithParamName(err error, field string) error { | |||
} | |||
return apierror.Newf(apierror.TypeBadData, errTmpl, field, err) | |||
} | |||
|
|||
func mustReadAll(r io.Reader) []byte { | |||
body, _ := ioutil.ReadAll(r) |
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.
could this use bodyBuffer(r)
, which DecodeResponse()
calls on the non-error case ?
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.
I don't think it's needed now, but it will definitely leave a more robust code, so replaced it here: a44e91b
Thanks.
integration/query_frontend_test.go
Outdated
require.NoError(t, distributor.WaitSumMetrics(e2e.Equals(512+1), "cortex_ring_tokens_total")) | ||
require.NoError(t, querier.WaitSumMetrics(e2e.Equals(512), "cortex_ring_tokens_total")) | ||
|
||
// Push a series for each user to Mimir. |
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.
"for each user" doesn't seem to match the code?
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.
Too much copy-paste, fixed 0a80fdf
integration/query_frontend_test.go
Outdated
} | ||
|
||
func TestQueryFrontendWithQueryShardingAndTooManyRequests(t *testing.T) { | ||
runQueryFrontendWithQueryShardingAndTooManyRequestsTest := func(t *testing.T, cfg queryFrontendTestConfig) { |
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.
Making this a local func pushes indenting out; is it worth 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.
I don't have a strong opinion on this, so extracted: 85b9d95
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, Thank you for fixing this!
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
When using a local queue, the v1.Frontend responds with an httpgrpc.Error when the queue is full. We need to convert that into a 429 http response instead of failing to roundtripping. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
This will let us reuse the body if needed in the future. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
a44e91b
to
a7bed6e
Compare
What this PR does
Handles the 429 status code when query sharding uses the prometheus codec.
Which issue(s) this PR fixes or relates to
Fixes #2169
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]