From 017c3676c820e81e2cf082b90057798e2bc32cd0 Mon Sep 17 00:00:00 2001 From: Antoine Tollenaere Date: Mon, 3 Jul 2023 09:29:57 +0200 Subject: [PATCH] (internal/retry) Simplify gRPC status code mapping of retry error gRPC v1.55.0 with a change in semantic for status.FromError such that the gRPC status of wrapped errors is returned when defined. The implementation of GRPCStatus in internal/retry code was doing just that. Removing GRPCStatus entirely makes the code much easier to understand: at the moment, status.FromError calls GRPCStatus which in turns calls status.FromError and it's not immediately obvious why this cannot result in infinite recursion. A previous change I made to this code made sure this method never returns status Code.OK (https://github.com/googleapis/google-cloud-go/pull/8128), but I failed to realize that since this project depends on gRPC 1.55 that already handles wrapped errors in status.FromError, we can simply remove the implementation of `GRPCStatus` and let gRPC status.FromError handle unwrapping. Note that I barely had to change the tests, but there *IS* a slight change in behavior: the message of the wrapping error is included in the gRPC error. I think it's fine, bet let me know if you think otherwise (for the same reasons discussed in https://github.com/grpc/grpc-go/pull/6150). --- internal/retry.go | 10 ---------- internal/retry_test.go | 7 ++----- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/internal/retry.go b/internal/retry.go index 290e744b7d36..4c9220e0dc36 100644 --- a/internal/retry.go +++ b/internal/retry.go @@ -20,8 +20,6 @@ import ( "time" gax "github.com/googleapis/gax-go/v2" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) // Retry calls the supplied function f repeatedly according to the provided @@ -76,11 +74,3 @@ func (e wrappedCallErr) Unwrap() error { func (e wrappedCallErr) Is(err error) bool { return e.ctxErr == err || e.wrappedErr == err } - -// GRPCStatus allows the wrapped error to be used with status.FromError. -func (e wrappedCallErr) GRPCStatus() *status.Status { - if s, ok := status.FromError(e.wrappedErr); ok { - return s - } - return status.New(codes.Unknown, e.Error()) -} diff --git a/internal/retry_test.go b/internal/retry_test.go index 1c26dcbcc394..0ede8b747ceb 100644 --- a/internal/retry_test.go +++ b/internal/retry_test.go @@ -88,7 +88,7 @@ func TestRetryPreserveError(t *testing.T) { if g, w := got.Code(), codes.NotFound; g != w { t.Errorf("got code %v, want %v", g, w) } - wantMessage := "not found" + wantMessage := "retry failed with context deadline exceeded; last error: rpc error: code = NotFound desc = not found" if g, w := got.Message(), wantMessage; g != w { t.Errorf("got message %q, want %q", g, w) } @@ -111,10 +111,7 @@ func TestRetryWrapsErrorWithStatusUnknown(t *testing.T) { if g, w := err.Error(), wantError; g != w { t.Errorf("got error %q, want %q", g, w) } - got, ok := status.FromError(err) - if !ok { - t.Fatal("expected error to implement a gRPC status") - } + got, _ := status.FromError(err) if g, w := got.Code(), codes.Unknown; g != w { t.Errorf("got code %v, want %v", g, w) }