Skip to content

Commit

Permalink
grpcutil: Treat all gRPC errors as ambiguous
Browse files Browse the repository at this point in the history
In gRPC 1.6, we could identify certain unambiguous failures with
string matching on the error. In 1.7, the same error message is used
for both ambiguous and unambiguous cases, so we must assume it is
ambiguous.

Release note (bug fix): Fixed a case in which ambiguous errors were
treated as unambiguous and led to inappropriate retries.

Updates cockroachdb#19708
  • Loading branch information
bdarnell committed Nov 15, 2017
1 parent f3cb1b7 commit 65ca11c
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 1 deletion.
6 changes: 5 additions & 1 deletion pkg/util/grpcutil/grpc_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,11 @@ func RequestDidNotStart(err error) bool {
// This is a non-gRPC error; assume nothing.
return false
}
if s.Code() == codes.Unavailable && s.Message() == "grpc: the connection is unavailable" {
// TODO(bdarnell): In gRPC 1.7, we have no good way to distinguish
// ambiguous from unambiguous failures, so we must assume all gRPC
// errors are ambiguous.
// https://github.com/cockroachdb/cockroach/issues/19708#issuecomment-343891640
if false && s.Code() == codes.Unavailable && s.Message() == "grpc: the connection is unavailable" {
return true
}
return false
Expand Down
2 changes: 2 additions & 0 deletions pkg/util/grpcutil/grpc_util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ func (hs healthServer) Check(
func TestRequestDidNotStart(t *testing.T) {
defer leaktest.AfterTest(t)()

t.Skip("https://github.com/cockroachdb/cockroach/issues/19708")

lis, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
t.Fatal(err)
Expand Down

0 comments on commit 65ca11c

Please sign in to comment.