avoid information loss on .GoError()

fixes cockroachdb#2628.

kv/dist_sender.go

@@ -515,7 +515,7 @@ func (ds *DistSender) sendChunk(ctx context.Context, ba proto.BatchRequest) (*pr
 			// getDescriptors may fail retryably if the first range isn't
 			// available via Gossip.
 			if pErr != nil {
-				if rErr, ok := pErr.GoError().(retry.Retryable); ok && rErr.CanRetry() {
+				if pErr.Retryable {
 					if log.V(1) {
 						log.Warning(pErr)
 					}

kv/txn_coord_sender.go

@@ -20,7 +20,6 @@ package kv
 
 import (
 	"fmt"
-	"reflect"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -695,10 +694,6 @@ func (tc *TxnCoordSender) updateState(ctx context.Context, ba proto.BatchRequest
 		newTxn.Restart(ba.GetUserPriority(), t.Txn.Priority, newTxn.Timestamp)
 		t.Txn = *newTxn
 	}
-	// TODO(tschottdorf): temporary assertion.
-	if txnErr, ok := err.(proto.TransactionRestartError); ok && txnErr.Transaction() != nil && !reflect.DeepEqual(txnErr.Transaction(), newTxn) {
-		panic(fmt.Sprintf("%T did not have the latest transaction updates", txnErr))
-	}
 
 	if len(newTxn.ID) > 0 {
 		id := string(newTxn.ID)

proto/api_test.go

@@ -21,6 +21,8 @@ import (
 	"fmt"
 	"reflect"
 	"testing"
+
+	"github.com/cockroachdb/cockroach/util/retry"
 )
 
 func TestClientCmdIDIsEmpty(t *testing.T) {
@@ -37,11 +39,12 @@ func TestClientCmdIDIsEmpty(t *testing.T) {
 
 type testError struct{}
 
-func (t *testError) Error() string  { return "test" }
-func (t *testError) CanRetry() bool { return true }
+func (t *testError) Error() string             { return "test" }
+func (t *testError) CanRetry() bool            { return true }
+func (t *testError) ErrorIndex() (int32, bool) { return 99, true }
 
 // TestResponseHeaderSetGoError verifies that a test error that
-// implements retryable is converted properly into a generic error.
+// implements retryable or indexed is converted properly into a generic error.
 func TestResponseHeaderSetGoError(t *testing.T) {
 	rh := ResponseHeader{}
 	rh.SetGoError(&testError{})
@@ -52,6 +55,16 @@ func TestResponseHeaderSetGoError(t *testing.T) {
 	if !rh.Error.Retryable {
 		t.Error("expected generic error to be retryable")
 	}
+	if rErr, ok := rh.Error.GoError().(retry.Retryable); !ok || !rErr.CanRetry() {
+		t.Error("generated GoError is not retryable")
+	}
+	if rh.Error.GetIndex().GetIndex() != 99 {
+		t.Errorf("expected generic error to have index set")
+	}
+	if iErr, ok := rh.Error.GoError().(IndexedError); !ok ||
+		func() int32 { i, _ := iErr.ErrorIndex(); return i }() != 99 {
+		t.Errorf("generated GoError lost the index")
+	}
 }
 
 // TestResponseHeaderNilError verifies that a nil error can be set

proto/errors.go

@@ -18,7 +18,6 @@
 package proto
 
 import (
-	"errors"
 	"fmt"
 
 	"github.com/cockroachdb/cockroach/util/retry"
@@ -48,6 +47,12 @@ type TransactionRestartError interface {
 	Transaction() *Transaction
 }
 
+// IndexedError is an interface implemented by errors which are associated
+// with a failed request in a Batch.
+type IndexedError interface {
+	ErrorIndex() (int32, bool) // bool is false iff no index associated
+}
+
 func (e Error) getDetail() error {
 	if e.Detail == nil {
 		return nil
@@ -67,7 +72,42 @@ func NewError(err error) *Error {
 
 // String implements fmt.Stringer.
 func (e *Error) String() string {
-	return e.GoError().Error()
+	return e.Message
+}
+
+type internalError Error
+
+// Error implements error.
+func (e *internalError) Error() string {
+	return (*Error)(e).String()
+}
+
+// CanRetry implements the retry.Retryable interface.
+func (e *internalError) CanRetry() bool {
+	return e.Retryable
+}
+
+// CanRestartTransaction implements the TransactionRestartError interface.
+func (e *internalError) CanRestartTransaction() TransactionRestart {
+	return e.TransactionRestart
+}
+
+// Transaction implements the TransactionRestartError interface by returning
+// nil. The idea is that an error which isn't an ErrorDetail can't hold a
+// transaction.
+// TODO(tschottdorf): If that assumption changes, this methods needs to as
+// well, and the transaction should be added as a field on Error. Probably
+// worth doing this right right away.
+func (e *internalError) Transaction() *Transaction {
+	return nil
+}
+
+// ErrorIndex implements IndexedError.
+func (e *internalError) ErrorIndex() (int32, bool) {
+	if e.Index == nil {
+		return 0, false
+	}
+	return e.Index.Index, true
 }
 
 // GoError returns the non-nil error from the proto.Error union.
@@ -76,12 +116,12 @@ func (e *Error) GoError() error {
 		return nil
 	}
 	if e.Detail == nil {
-		return errors.New(e.Message)
+		return (*internalError)(e)
 	}
 	err := e.getDetail()
 	if err == nil {
 		// Unknown error detail; return the generic error.
-		return errors.New(e.Message)
+		return (*internalError)(e)
 	}
 	// Make sure that the flags in the generic portion of the error
 	// match the methods of the specific error type.
@@ -116,6 +156,11 @@ func (e *Error) SetResponseGoError(err error) {
 	if r, ok := err.(TransactionRestartError); ok {
 		e.TransactionRestart = r.CanRestartTransaction()
 	}
+	if r, ok := err.(IndexedError); ok {
+		if index, ok := r.ErrorIndex(); ok {
+			e.Index = &Error_Index{Index: index}
+		}
+	}
 	// If the specific error type exists in the detail union, set it.
 	detail := &ErrorDetail{}
 	if detail.SetValue(err) {

storage/store.go

@@ -139,15 +139,25 @@ func verifyKeys(start, end proto.Key, checkEndKey bool) error {
 	return nil
 }
 
+// errWithIndex is returned when an individual request in a Batch results in
+// an error, wrapping the original error and its location within the Batch.
 type errWithIndex struct {
 	index int32
 	err   error
 }
 
+// Error implements error.
 func (ewi *errWithIndex) Error() string {
 	return fmt.Sprintf("at %d: %s", ewi.index, ewi.err)
 }
 
+// ErrorIndex implements proto.IndexedError.
+// It's currently un-used but its presence is mandatory should errWithIndex
+// ever be added to proto.ErrorDetail.
+func (ewi *errWithIndex) ErrorIndex() (int32, bool) {
+	return ewi.index, true
+}
+
 // unwrapIndexedError returns the wrapped error for an *errWithIndex, and
 // the given error otherwise.
 func unwrapIndexedError(err error) error {
@@ -1229,6 +1239,8 @@ func (s *Store) ExecuteCmd(ctx context.Context, args proto.Request) (proto.Respo
 			// clients will retry.
 			err = proto.NewRangeNotFoundError(rng.Desc().RangeID)
 		} else if iErr, ok := err.(*errWithIndex); ok {
+			// Note that we're not using the proto.IndexedError interface here
+			// because we need to unwrap the error anyways.
 			err, index = iErr.err, gogoproto.Int32(iErr.index)
 		}
 

storage/store_test.go

@@ -21,6 +21,7 @@ package storage
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"math"
 	"strings"
@@ -1550,3 +1551,19 @@ func TestMaybeRemove(t *testing.T) {
 		t.Errorf("Unexpected removed range %v", removedRng)
 	}
 }
+
+func TestErrWithIndex(t *testing.T) {
+	defer leaktest.AfterTest(t)
+	ind := int32(99)
+	err := error(&errWithIndex{err: errors.New("foo"), index: ind})
+	i1, ok1 := err.(proto.IndexedError).ErrorIndex()
+	pErr := &proto.Error{}
+	pErr.SetResponseGoError(err)
+	if pErr.GetIndex().GetIndex() != ind {
+		log.Warningf("proto.Error lost index information")
+	}
+	i2, ok2 := pErr.GoError().(proto.IndexedError).ErrorIndex()
+	if i1 != i2 || ok1 != ok2 {
+		t.Fatalf("information loss after GoError(): (%d,%t) -> (%d,%t)", i1, ok1, i2, ok2)
+	}
+}