Ingester: replace grpc/status usages with gogo/status #6416
Conversation
| } | ||
|
|
||
| func (e errorWithStatus) Unwrap() error { | ||
| return e.err | ||
| } | ||
|
|
||
| func (e errorWithStatus) GRPCStatus() *status.Status { | ||
| return e.status | ||
| func (e errorWithStatus) GRPCStatus() *grpcstatus.Status { |
There was a problem hiding this comment.
Note for reviewer: this is needed to ensure that gRPC engine will be able to recognizer errorWithStatus by calling grpc.status.FromError().
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
| @@ -430,3 +441,20 @@ func newIngesterErrSamplers(freq int64) ingesterErrSamplers { | |||
| log.NewSampler(freq), | |||
| } | |||
| } | |||
|
|
|||
| func handlePushError(err error) error { | |||
There was a problem hiding this comment.
Note for reviewers: this method has been moved from ingester.go to errors.go.
| @@ -292,3 +316,151 @@ func checkIngesterError(t *testing.T, err error, expectedType ingesterErrorType, | |||
| require.ErrorAs(t, err, &softErr) | |||
| } | |||
| } | |||
|
|
|||
| func TestHandlePushError(t *testing.T) { | |||
There was a problem hiding this comment.
Note for reviewers: this test has been moved from ingester.go to errors.go.
| @@ -3154,23 +3154,6 @@ func (i *Ingester) Push(ctx context.Context, req *mimirpb.WriteRequest) (*mimirp | |||
| return nil, handledErr | |||
| } | |||
|
|
|||
| func handlePushError(err error) error { | |||
There was a problem hiding this comment.
Note for reviewers: this method has been moved from ingester.go to errors.go.
| @@ -8992,151 +8991,3 @@ func TestIngester_lastUpdatedTimeIsNotInTheFuture(t *testing.T) { | |||
| // Verify that maxTime of TSDB is actually our future sample. | |||
| require.Equal(t, futureTS, db.db.Head().MaxTime()) | |||
| } | |||
|
|
|||
| func TestIngester_HandlePushError(t *testing.T) { | |||
There was a problem hiding this comment.
Note for reviewers: this test has been moved from ingester.go to errors.go.
pkg/ingester/errors_test.go
Outdated
| require.Equal(t, codes.Unavailable, st.Code()) | ||
| require.Errorf(t, err, st.Message()) | ||
|
|
||
| // Ensure httpgrpc's HTTPResponseFromError does not recognize errWithStatuserrWithStatus. |
There was a problem hiding this comment.
| // Ensure httpgrpc's HTTPResponseFromError does not recognize errWithStatuserrWithStatus. | |
| // Ensure httpgrpc's HTTPResponseFromError does not recognize errWithStatus. |
There was a problem hiding this comment.
I have fixed this typo. Thank you.
pkg/ingester/errors_test.go
Outdated
| require.Equal(t, http.StatusBadRequest, int(st.Code())) | ||
| require.Errorf(t, err, st.Message()) | ||
|
|
||
| // Ensure httpgrpc's HTTPResponseFromError recognizes errWithStatuserrWithHTTPStatus. |
There was a problem hiding this comment.
| // Ensure httpgrpc's HTTPResponseFromError recognizes errWithStatuserrWithHTTPStatus. | |
| // Ensure httpgrpc's HTTPResponseFromError recognizes errWithStatus. |
There was a problem hiding this comment.
I have fixed this typo. Thank you.
| require.Errorf(t, err, st.Message()) | ||
|
|
||
| // Ensure httpgrpc's HTTPResponseFromError does not recognize errWithStatuserrWithStatus. | ||
| resp, ok := httpgrpc.HTTPResponseFromError(errWithStatus) |
There was a problem hiding this comment.
why do we want it to not recognize the status? is that just to ensure that no behavior changes?
There was a problem hiding this comment.
httpgrpc.HttpResponseFromError() ensures that the given error is built by httpgrpc.Errorf(). Since we need to guarantee backwards compatibility, methods that translate errors to the corresponding HTTP or gRPC codes will need to be able to distinguish between errors built by httpgrpc.Errorf() and status.Error().
status.FromError() recognizes both types of errors, but httpgrpc.HttpResponseFromError() recognzies only the former. This test ensures that this property holds.
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
|
|
What this PR does
This PR replaces the usage of
grpc/statuswithgogo/statusin ingester-specific error handling. The reason for that is because the latter allows a more simple handling of custom error details.Which issue(s) this PR fixes or relates to
Related to #6008
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]