avoid information loss on .GoError()

client/client_test.go

@@ -407,7 +407,7 @@ func TestClientBatch(t *testing.T) {
 			b.Inc(key, int64(i))
 		}
 
-		if err := db.Run(b).GoError(); err != nil {
+		if err := db.Run(b); err != nil {
 			t.Error(err)
 		}
 
@@ -423,7 +423,7 @@ func TestClientBatch(t *testing.T) {
 		b := &client.Batch{}
 		b.Scan(testUser+"/key 00", testUser+"/key 05", 0)
 		b.Scan(testUser+"/key 05", testUser+"/key 10", 0)
-		if err := db.Run(b).GoError(); err != nil {
+		if err := db.Run(b); err != nil {
 			t.Error(err)
 		}
 
@@ -454,7 +454,7 @@ func TestClientBatch(t *testing.T) {
 		b := &client.Batch{}
 		b.ReverseScan(testUser+"/key 00", testUser+"/key 05", 0)
 		b.ReverseScan(testUser+"/key 05", testUser+"/key 10", 0)
-		if err := db.Run(b).GoError(); err != nil {
+		if err := db.Run(b); err != nil {
 			t.Error(err)
 		}
 
@@ -492,7 +492,7 @@ func TestClientBatch(t *testing.T) {
 
 		b := &client.Batch{}
 		b.CPut(key, "goodbyte", nil) // should fail
-		if err := db.Run(b).GoError(); err == nil {
+		if err := db.Run(b); err == nil {
 			t.Error("unexpected success")
 		} else {
 			var foundError bool
@@ -518,7 +518,7 @@ func TestClientBatch(t *testing.T) {
 		b := &client.Batch{}
 		b.CPut(key, "goodbyte", nil) // should fail
 		if err := db.Txn(func(txn *client.Txn) error {
-			return txn.Run(b).GoError()
+			return txn.Run(b)
 		}); err == nil {
 			t.Error("unexpected success")
 		} else {

client/db.go

@@ -432,11 +432,11 @@ func sendAndFill(send func(...proto.Call) *proto.Error, b *Batch) *proto.Error {
 // Upon completion, Batch.Results will contain the results for each
 // operation. The order of the results matches the order the operations were
 // added to the batch.
-func (db *DB) Run(b *Batch) *proto.Error {
+func (db *DB) Run(b *Batch) error {
 	if err := b.prepare(); err != nil {
-		return proto.NewError(err)
+		return err
 	}
-	return sendAndFill(db.send, b)
+	return sendAndFill(db.send, b).GoError()
 }
 
 // Txn executes retryable in the context of a distributed transaction. The
@@ -514,20 +514,20 @@ func (db *DB) send(calls ...proto.Call) (pErr *proto.Error) {
 
 // Runner only exports the Run method on a batch of operations.
 type Runner interface {
-	Run(b *Batch) *proto.Error
+	Run(b *Batch) error
 }
 
 func runOneResult(r Runner, b *Batch) (Result, error) {
-	if pErr := r.Run(b); pErr != nil {
-		return Result{Err: pErr.GoError()}, pErr.GoError()
+	if err := r.Run(b); err != nil {
+		return Result{Err: err}, err
 	}
 	res := b.Results[0]
 	return res, res.Err
 }
 
 func runOneRow(r Runner, b *Batch) (KeyValue, error) {
-	if pErr := r.Run(b); pErr != nil {
-		return KeyValue{}, pErr.GoError()
+	if err := r.Run(b); err != nil {
+		return KeyValue{}, err
 	}
 	res := b.Results[0]
 	return res.Rows[0], res.Err

client/db_test.go

@@ -146,7 +146,7 @@ func ExampleBatch() {
 	b := &client.Batch{}
 	b.Get("aa")
 	b.Put("bb", "2")
-	if err := db.Run(b).GoError(); err != nil {
+	if err := db.Run(b); err != nil {
 		panic(err)
 	}
 	for _, result := range b.Results {
@@ -168,7 +168,7 @@ func ExampleDB_Scan() {
 	b.Put("aa", "1")
 	b.Put("ab", "2")
 	b.Put("bb", "3")
-	if err := db.Run(b).GoError(); err != nil {
+	if err := db.Run(b); err != nil {
 		panic(err)
 	}
 	rows, err := db.Scan("a", "b", 100)
@@ -192,7 +192,7 @@ func ExampleDB_ReverseScan() {
 	b.Put("aa", "1")
 	b.Put("ab", "2")
 	b.Put("bb", "3")
-	if err := db.Run(b).GoError(); err != nil {
+	if err := db.Run(b); err != nil {
 		panic(err)
 	}
 	rows, err := db.ReverseScan("ab", "c", 100)
@@ -216,7 +216,7 @@ func ExampleDB_Del() {
 	b.Put("aa", "1")
 	b.Put("ab", "2")
 	b.Put("ac", "3")
-	if err := db.Run(b).GoError(); err != nil {
+	if err := db.Run(b); err != nil {
 		panic(err)
 	}
 	if err := db.Del("ab"); err != nil {
@@ -252,7 +252,7 @@ func ExampleTx_Commit() {
 	b := &client.Batch{}
 	b.Get("aa")
 	b.Get("ab")
-	if err := db.Run(b).GoError(); err != nil {
+	if err := db.Run(b); err != nil {
 		panic(err)
 	}
 	for i, result := range b.Results {

client/txn.go

@@ -280,11 +280,11 @@ func (txn *Txn) DelRange(begin, end interface{}) error {
 // Upon completion, Batch.Results will contain the results for each
 // operation. The order of the results matches the order the operations were
 // added to the batch.
-func (txn *Txn) Run(b *Batch) *proto.Error {
+func (txn *Txn) Run(b *Batch) error {
 	if err := b.prepare(); err != nil {
-		return proto.NewError(err)
+		return err
 	}
-	return sendAndFill(txn.send, b)
+	return sendAndFill(txn.send, b).GoError()
 }
 
 func (txn *Txn) commit() error {
@@ -314,7 +314,7 @@ func (txn *Txn) CommitNoCleanup() error {
 func (txn *Txn) CommitInBatch(b *Batch) error {
 	b.calls = append(b.calls, endTxnCall(true /* commit */, txn.systemDBTrigger))
 	b.initResult(1, 0, nil)
-	return txn.Run(b).GoError()
+	return txn.Run(b)
 }
 
 // Commit sends an EndTransactionRequest with Commit=true.

examples/kv_bank/main.go

@@ -112,7 +112,7 @@ func (bank *Bank) continuouslyTransferMoney(cash int64) {
 			batchRead := &client.Batch{}
 			batchRead.Get(from)
 			batchRead.Get(to)
-			if err := runner.Run(batchRead).GoError(); err != nil {
+			if err := runner.Run(batchRead); err != nil {
 				return err
 			}
 			// Read from value.
@@ -143,7 +143,7 @@ func (bank *Bank) continuouslyTransferMoney(cash int64) {
 				batchWrite.Put(from, fromValue)
 				batchWrite.Put(to, toValue)
 			}
-			return runner.Run(batchWrite).GoError()
+			return runner.Run(batchWrite)
 		}
 		var err error
 		if *useTransaction {
@@ -198,7 +198,7 @@ func (bank *Bank) initBankAccounts(cash int64) int64 {
 				newCash += cash
 			}
 		}
-		return txn.Run(batch).GoError()
+		return txn.Run(batch)
 	}); err != nil {
 		log.Fatal(err)
 	}

kv/db_test.go

@@ -171,7 +171,7 @@ func TestKVDBInternalMethods(t *testing.T) {
 		}
 		b := &client.Batch{}
 		b.InternalAddCall(proto.Call{Args: args, Reply: args.CreateReply()})
-		err := db.Run(b).GoError()
+		err := db.Run(b)
 		if err == nil {
 			t.Errorf("%d: unexpected success calling %s", i, args.Method())
 		} else if !testutils.IsError(err, "(couldn't find method|contains commit trigger)") {
@@ -184,7 +184,7 @@ func TestKVDBInternalMethods(t *testing.T) {
 		b = &client.Batch{}
 		b.InternalAddCall(proto.Call{Args: ba, Reply: &proto.BatchResponse{}})
 
-		if err := db.Run(b).GoError(); err == nil {
+		if err := db.Run(b); err == nil {
 			t.Errorf("%d: unexpected success calling %s", i, args.Method())
 		} else if !testutils.IsError(err, "(contains an internal request|contains commit trigger)") {
 			t.Errorf("%d: expected disallowed method error %s; got %s", i, args.Method(), err)
@@ -255,14 +255,14 @@ func TestAuthentication(t *testing.T) {
 	// Create a "node" client and call Run() on it which lets us build
 	// our own request, specifying the user.
 	db1 := createTestClientForUser(t, s.Stopper(), s.ServingAddr(), security.NodeUser)
-	if err := db1.Run(b).GoError(); err != nil {
+	if err := db1.Run(b); err != nil {
 		t.Fatal(err)
 	}
 
 	// Try again, but this time with certs for a non-node user (even the root
 	// user has no KV permissions).
 	db2 := createTestClientForUser(t, s.Stopper(), s.ServingAddr(), security.RootUser)
-	if err := db2.Run(b).GoError(); err == nil {
+	if err := db2.Run(b); err == nil {
 		t.Fatal("Expected error!")
 	}
 }

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/dist_sender_server_test.go

@@ -120,7 +120,7 @@ func TestMultiRangeBatchBounded(t *testing.T) {
 	b.Scan("aaa", "dd", 3)
 	b.Scan("a", "z", 2)
 	b.Scan("cc", "ff", 3)
-	if err := db.Run(b).GoError(); err != nil {
+	if err := db.Run(b); err != nil {
 		t.Fatal(err)
 	}
 
@@ -231,7 +231,7 @@ func TestMultiRangeScanReverseScanInconsistent(t *testing.T) {
 	for _, key := range keys {
 		b.Put(key, "value")
 	}
-	if err := db.Run(b).GoError(); err != nil {
+	if err := db.Run(b); err != nil {
 		t.Fatal(err)
 	}
 	for i := range keys {

kv/txn_coord_sender.go

@@ -20,7 +20,6 @@ package kv
 
 import (
 	"fmt"
-	"reflect"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -694,10 +693,12 @@ func (tc *TxnCoordSender) updateState(ctx context.Context, ba proto.BatchRequest
 		newTxn.Timestamp.Forward(t.Txn.Timestamp)
 		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))
+	case proto.TransactionRestartError:
+		// Assertion: The above cases should exhaust all ErrorDetails which
+		// carry a Transaction.
+		if pErr.Detail != nil {
+			panic(fmt.Sprintf("unhandled TransactionRestartError %T", err))
+		}
 	}
 
 	if len(newTxn.ID) > 0 {

proto/api.go

@@ -424,7 +424,7 @@ func (rh *ResponseHeader) SetGoError(err error) {
 		return
 	}
 	rh.Error = &Error{}
-	rh.Error.SetResponseGoError(err)
+	rh.Error.SetGoError(err)
 }
 
 // Verify verifies the integrity of the get response value.

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,13 @@ 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 }
+func (t *testError) SetErrorIndex(_ int32)     { panic("unsupported") }
 
 // 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 +56,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