-
Notifications
You must be signed in to change notification settings - Fork 569
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
Remove errors.Cause() usage from store-gateway and its clients #7213
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.
🔥
I have worked on the error handling improvement on the write path, and I ensured that most of the usages of the Nevertheless, in this PR the semantics wouldn't change. But if at a point somebody decides to replace |
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
7d3e19a
to
d535c3d
Compare
That's a very good feedback @duricanikolic (what I was looking for!). I checked There are few other cases of |
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.
The follow up PR: #7224 |
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, the wrapping logic (using now Unwrap()
instead of Cause()
) shouldn't change for the errors the store-gateway creates.
IIRC the prometheus libraries also stopped returning errors with Cause()
methods (I remember you mentioned it in a PR Marco), but those wouldn't have been gRPC errors anyways.
Yes, already addressed all such cases (in previous PR). |
What this PR does
We have few usage of
errors.Cause()
in the store-gateway and the querier which I believe being superfluous. The usage addressed by this PR isstatus.FromError(errors.Cause(err))
. The actualstatus.FromError()
implementation internally useserrors.As()
(which navigates the chain of wrapped errors), so we shouldn't neederrors.Cause()
.The
errors.Cause()
inshouldStopQueryFunc()
was introduced by #4100 which also introduced the integration testTest_MaxSeriesAndChunksPerQueryLimitHit
. The integration test passes on top of this PR. As a counter proof, of we completely remove thestatus.FromError()
check fromshouldStopQueryFunc()
, then theTest_MaxSeriesAndChunksPerQueryLimitHit
integration test fails.Note: this PR is part of a work I'm doing to completely remove errors.Cause() from Mimir.
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.