From 8385a695274dc826d29e0e2db1e144eb05016469 Mon Sep 17 00:00:00 2001 From: Max Neverov Date: Sat, 23 Jan 2021 09:25:40 +0100 Subject: [PATCH] grpcutil: Return true from RequestDidNotStart on open circuit breaker error Previously, the gRPC error check returned false on open circuit breaker error that led to the request retries. Returning true instead, should prevent needless retries. Fixes: #55869 Release note: None --- pkg/util/grpcutil/BUILD.bazel | 3 +++ pkg/util/grpcutil/grpc_util.go | 4 +++- pkg/util/grpcutil/grpc_util_test.go | 8 ++++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/util/grpcutil/BUILD.bazel b/pkg/util/grpcutil/BUILD.bazel index df5b8892a8f3..2ee05ed902ce 100644 --- a/pkg/util/grpcutil/BUILD.bazel +++ b/pkg/util/grpcutil/BUILD.bazel @@ -14,6 +14,7 @@ go_library( "//pkg/util/netutil", "//pkg/util/syncutil", "//pkg/util/timeutil", + "@com_github_cockroachdb_circuitbreaker//:circuitbreaker", "@com_github_cockroachdb_errors//:errors", "@org_golang_google_grpc//:go_default_library", "@org_golang_google_grpc//codes", @@ -36,7 +37,9 @@ go_test( "//pkg/util/leaktest", "//pkg/util/log/severity", "//pkg/util/timeutil", + "@com_github_cockroachdb_circuitbreaker//:circuitbreaker", "@com_github_cockroachdb_errors//:errors", + "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@org_golang_google_grpc//:go_default_library", "@org_golang_google_grpc//health/grpc_health_v1", diff --git a/pkg/util/grpcutil/grpc_util.go b/pkg/util/grpcutil/grpc_util.go index d5c820c4b56b..231f0a4b577c 100644 --- a/pkg/util/grpcutil/grpc_util.go +++ b/pkg/util/grpcutil/grpc_util.go @@ -16,6 +16,7 @@ import ( "io" "strings" + circuit "github.com/cockroachdb/circuitbreaker" "github.com/cockroachdb/cockroach/pkg/util/netutil" "github.com/cockroachdb/errors" "google.golang.org/grpc" @@ -111,7 +112,8 @@ func IsAuthenticationError(err error) bool { // https://github.com/grpc/grpc-go/issues/1443 is resolved. func RequestDidNotStart(err error) bool { if errors.HasType(err, connectionNotReadyError{}) || - errors.HasType(err, (*netutil.InitialHeartbeatFailedError)(nil)) { + errors.HasType(err, (*netutil.InitialHeartbeatFailedError)(nil)) || + errors.Is(err, circuit.ErrBreakerOpen) { return true } s, ok := status.FromError(errors.Cause(err)) diff --git a/pkg/util/grpcutil/grpc_util_test.go b/pkg/util/grpcutil/grpc_util_test.go index ab86752b1a42..b57ba82a1d1f 100644 --- a/pkg/util/grpcutil/grpc_util_test.go +++ b/pkg/util/grpcutil/grpc_util_test.go @@ -16,11 +16,13 @@ import ( "strings" "testing" + circuit "github.com/cockroachdb/circuitbreaker" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/util/grpcutil" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/errors" + "github.com/stretchr/testify/assert" "google.golang.org/grpc" healthpb "google.golang.org/grpc/health/grpc_health_v1" ) @@ -118,3 +120,9 @@ func TestRequestDidNotStart(t *testing.T) { t.Fatalf("request should not have started, but got %s", err) } } + +func TestRequestDidNotStart_OpenBreaker(t *testing.T) { + err := errors.Wrapf(circuit.ErrBreakerOpen, "unable to dial n%d", 42) + res := grpcutil.RequestDidNotStart(err) + assert.True(t, res) +}