Merge #59309

59309: grpcutil: Return true from RequestDidNotStart on open circuit breaker error r=tbg a=mneverov 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 Co-authored-by: Max Neverov <[email protected]>
cockroachdb · Jan 25, 2021 · 8efa63d · 8efa63d
2 parents 48fbbe7 + 8385a69
commit 8efa63d
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 1 deletion.
diff --git 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
@@ -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
@@ -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)
+}