Return data fetched from a subset of store-gateways instead of returning error if a single store-gateway fails #4532
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roystchiang
force-pushed
the
store-gateway-partial-failure
branch
from
88550a2
to
e45c8e3
Compare
roystchiang
force-pushed
the
store-gateway-partial-failure
branch
2 times, most recently
from
492feae
to
412eb6a
Compare
roystchiang
force-pushed
the
store-gateway-partial-failure
branch
from
b09eb12
to
a787055
Compare
|
Hi, |
alanprot
reviewed
Jan 31, 2022
| @@ -654,12 +659,14 @@ func (q *blocksStoreQuerier) fetchSeriesFromStores( | |||
| if h := resp.GetHints(); h != nil { | |||
| hints := hintspb.SeriesResponseHints{} | |||
| if err := types.UnmarshalAny(h, &hints); err != nil { | |||
| return errors.Wrapf(err, "failed to unmarshal series hints from %s", c.RemoteAddress()) | |||
| level.Warn(spanLog).Log("err", errors.Wrapf(err, "failed to unmarshal series hints from %s", c.RemoteAddress())) | |||
harry671003
reviewed
Apr 5, 2022
| @@ -590,6 +590,9 @@ func (q *blocksStoreQuerier) fetchSeriesFromStores( | |||
| // TODO(goutham): we should ideally be passing the hints down to the storage layer | |||
| // and let the TSDB return us data with no chunks as in prometheus#8050. | |||
| // But this is an acceptable workaround for now. | |||
|
|
|||
| // Only fail the function if we have validation error. We should return blocks that were successfully | |||
There was a problem hiding this comment.
We should also fail the function when store-gateways return a limit error. It'll be good to avoid unnecessary retries in that case.
roystchiang
force-pushed
the
store-gateway-partial-failure
branch
3 times, most recently
from
3aea3af
to
8ebebae
Compare
|
Nice! LGTM |
alanprot
approved these changes
Jul 8, 2022
alvinlin123
reviewed
Jul 13, 2022
| @@ -595,6 +602,10 @@ func (q *blocksStoreQuerier) fetchSeriesFromStores( | |||
|
|
|||
| stream, err := c.Series(gCtx, req) | |||
| if err != nil { | |||
| if isRetryableError(err) { | |||
| level.Warn(spanLog).Log("err", errors.Wrapf(err, "failed to fetch series from %s", c.RemoteAddress())) | |||
There was a problem hiding this comment.
Can we make it clear that this is a retriable error in the log message? Something like "failed to fetch series from %s due to retriable error".
alvinlin123
approved these changes
Jul 13, 2022
…ing error if a single store-gateway fails Signed-off-by: Roy Chiang <roychi@amazon.com>
Signed-off-by: Roy Chiang <roychi@amazon.com>
Signed-off-by: Roy Chiang <roychi@amazon.com>
Signed-off-by: Roy Chiang <roychi@amazon.com>
Signed-off-by: Roy Chiang <roychi@amazon.com>
Signed-off-by: Roy Chiang <roychi@amazon.com>
roystchiang
force-pushed
the
store-gateway-partial-failure
branch
from
546b41c
to
d1f10b2
Compare
alvinlin123
approved these changes
Jul 13, 2022
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does:
Previously, if a single store-gateway fails to return data, the whole function fails. This causes queries to fail without reattempting other available store-gateways.
Now, the function responsible for fetching data returns whatever was retrieved, and relies on the caller to determine whether all the necessary blocks were gathered.
Which issue(s) this PR fixes:
Fixes #4529
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]