Merge cockroachdb#33572

33572: roachpb: Rename HandledRetryableTxnError to RetryUsingTransactionError r=bobvawter a=bobvawter

The name HandledRetryableTxnError doesn't clearly indicate what, if anything,
the caller is expected to do with the error. The name "Handled" seems to imply
that nothing needs to be done (i.e. the problem represented by the error has
already been taken care of). The misunderstanding about just what "handled"
means lead to some fruitless debugging of a problem that didn't actually exist.

This change renames the error to, hopefully, be more descritive about what it
really means.

Release note: None

Co-authored-by: Bob Vawter <bob@cockroachlabs.com>

pkg/internal/client/client_test.go

@@ -347,7 +347,7 @@ func TestClientRunConcurrentTransaction(t *testing.T) {
 			for _, err := range concErrs {
 				if err != nil {
 					anyError = err
-					if _, ok := err.(*roachpb.HandledRetryableTxnError); ok {
+					if _, ok := err.(*roachpb.RetryUsingTransactionError); ok {
 						return err
 					}
 				}

pkg/internal/client/db.go

@@ -587,10 +587,10 @@ func (db *DB) Txn(ctx context.Context, retryable func(context.Context, *Txn) err
 	if err != nil {
 		txn.CleanupOnError(ctx, err)
 	}
-	// Terminate HandledRetryableTxnError here, so it doesn't cause a higher-level
+	// Terminate RetryUsingTransactionError here, so it doesn't cause a higher-level
 	// txn to be retried. We don't do this in any of the other functions in DB; I
 	// guess we should.
-	if _, ok := err.(*roachpb.HandledRetryableTxnError); ok {
+	if _, ok := err.(*roachpb.RetryUsingTransactionError); ok {
 		return errors.Wrapf(err, "terminated retryable error")
 	}
 	return err

pkg/internal/client/txn.go

@@ -686,7 +686,7 @@ func (txn *Txn) exec(ctx context.Context, fn func(context.Context, *Txn) error)
 				err = txn.Commit(ctx)
 				log.Eventf(ctx, "client.Txn did AutoCommit. err: %v\n", err)
 				if err != nil {
-					if _, retryable := err.(*roachpb.HandledRetryableTxnError); !retryable {
+					if _, retryable := err.(*roachpb.RetryUsingTransactionError); !retryable {
 						// We can't retry, so let the caller know we tried to
 						// autocommit.
 						err = &AutoCommitError{cause: err}
@@ -702,16 +702,16 @@ func (txn *Txn) exec(ctx context.Context, fn func(context.Context, *Txn) error)
 		case *roachpb.UnhandledRetryableError:
 			if txn.typ == RootTxn {
 				// We sent transactional requests, so the TxnCoordSender was supposed to
-				// turn retryable errors into HandledRetryableTxnError. Note that this
+				// turn retryable errors into RetryUsingTransactionError. Note that this
 				// applies only in the case where this is the root transaction.
 				log.Fatalf(ctx, "unexpected UnhandledRetryableError at the txn.exec() level: %s", err)
 			}
 
-		case *roachpb.HandledRetryableTxnError:
+		case *roachpb.RetryUsingTransactionError:
 			if !txn.IsRetryableErrMeantForTxn(*t) {
 				// Make sure the txn record that err carries is for this txn.
 				// If it's not, we terminate the "retryable" character of the error. We
-				// might get a HandledRetryableTxnError if the closure ran another
+				// might get a RetryUsingTransactionError if the closure ran another
 				// transaction internally and let the error propagate upwards.
 				return errors.Wrapf(err, "retryable error from another txn")
 			}
@@ -740,7 +740,7 @@ func (txn *Txn) PrepareForRetry(ctx context.Context, err error) {
 
 // IsRetryableErrMeantForTxn returns true if err is a retryable
 // error meant to restart this client transaction.
-func (txn *Txn) IsRetryableErrMeantForTxn(retryErr roachpb.HandledRetryableTxnError) bool {
+func (txn *Txn) IsRetryableErrMeantForTxn(retryErr roachpb.RetryUsingTransactionError) bool {
 	txn.mu.Lock()
 	defer txn.mu.Unlock()
 
@@ -783,7 +783,7 @@ func (txn *Txn) Send(
 		return br, nil
 	}
 
-	if retryErr, ok := pErr.GetDetail().(*roachpb.HandledRetryableTxnError); ok {
+	if retryErr, ok := pErr.GetDetail().(*roachpb.RetryUsingTransactionError); ok {
 		if requestTxnID != retryErr.TxnID {
 			// KV should not return errors for transactions other than the one that sent
 			// the request.
@@ -799,7 +799,7 @@ func (txn *Txn) Send(
 }
 
 func (txn *Txn) handleErrIfRetryableLocked(ctx context.Context, err error) {
-	retryErr, ok := err.(*roachpb.HandledRetryableTxnError)
+	retryErr, ok := err.(*roachpb.RetryUsingTransactionError)
 	if !ok {
 		return
 	}
@@ -867,7 +867,7 @@ func (txn *Txn) UpdateStateOnRemoteRetryableErr(ctx context.Context, pErr *roach
 	}
 
 	pErr = txn.mu.sender.UpdateStateOnRemoteRetryableErr(ctx, pErr)
-	txn.replaceSenderIfTxnAbortedLocked(ctx, pErr.GetDetail().(*roachpb.HandledRetryableTxnError), origTxnID)
+	txn.replaceSenderIfTxnAbortedLocked(ctx, pErr.GetDetail().(*roachpb.RetryUsingTransactionError), origTxnID)
 
 	return pErr.GoError()
 }
@@ -878,7 +878,7 @@ func (txn *Txn) UpdateStateOnRemoteRetryableErr(ctx context.Context, pErr *roach
 // origTxnID is the id of the txn that generated retryErr. Note that this can be
 // different from retryErr.Transaction - the latter might be a new transaction.
 func (txn *Txn) replaceSenderIfTxnAbortedLocked(
-	ctx context.Context, retryErr *roachpb.HandledRetryableTxnError, origTxnID uuid.UUID,
+	ctx context.Context, retryErr *roachpb.RetryUsingTransactionError, origTxnID uuid.UUID,
 ) {
 	// The proto inside the error has been prepared for use by the next
 	// transaction attempt.
@@ -927,7 +927,7 @@ func (txn *Txn) SetFixedTimestamp(ctx context.Context, ts hlc.Timestamp) {
 	txn.mu.sender.SetFixedTimestamp(ctx, ts)
 }
 
-// GenerateForcedRetryableError returns a HandledRetryableTxnError that will
+// GenerateForcedRetryableError returns a RetryUsingTransactionError that will
 // cause the txn to be retried.
 //
 // The transaction's epoch is bumped, simulating to an extent what the
@@ -941,7 +941,7 @@ func (txn *Txn) GenerateForcedRetryableError(ctx context.Context, msg string) er
 	now := txn.db.clock.Now()
 	txn.mu.sender.ManualRestart(ctx, txn.mu.userPriority, now)
 	txn.resetDeadlineLocked()
-	return roachpb.NewHandledRetryableTxnError(
+	return roachpb.NewRetryUsingTransactionError(
 		msg,
 		txn.mu.ID,
 		roachpb.MakeTransaction(

pkg/internal/client/txn_test.go

@@ -278,7 +278,7 @@ func TestRunTransactionRetryOnErrors(t *testing.T) {
 								if pErr.TransactionRestart != roachpb.TransactionRestart_NONE {
 									// HACK ALERT: to do without a TxnCoordSender, we jump through
 									// hoops to get the retryable error expected by db.Txn().
-									return nil, roachpb.NewError(roachpb.NewHandledRetryableTxnError(
+									return nil, roachpb.NewError(roachpb.NewRetryUsingTransactionError(
 										pErr.Message, ba.Txn.ID, *ba.Txn))
 								}
 								return nil, pErr
@@ -425,7 +425,7 @@ func TestWrongTxnRetry(t *testing.T) {
 			t.Fatal(err)
 		}
 		// Simulate an inner txn by generating an error with a bogus txn id.
-		return roachpb.NewHandledRetryableTxnError("test error", uuid.MakeV4(), roachpb.Transaction{})
+		return roachpb.NewRetryUsingTransactionError("test error", uuid.MakeV4(), roachpb.Transaction{})
 	}
 
 	if err := db.Txn(context.TODO(), txnClosure); !testutils.IsError(err, "test error") {

pkg/kv/dist_sender_server_test.go

@@ -1704,7 +1704,7 @@ func TestPropagateTxnOnError(t *testing.T) {
 		b.CPut(targetKey, "new_val", origVal)
 		err := txn.CommitInBatch(ctx, b)
 		if epoch == 1 {
-			if retErr, ok := err.(*roachpb.HandledRetryableTxnError); ok {
+			if retErr, ok := err.(*roachpb.RetryUsingTransactionError); ok {
 				if !testutils.IsError(retErr, "ReadWithinUncertaintyIntervalError") {
 					t.Errorf("expected ReadWithinUncertaintyIntervalError, but got: %s", retErr)
 				}

pkg/kv/integration_test.go

@@ -182,8 +182,8 @@ func TestDelayedBeginRetryable(t *testing.T) {
 		t.Fatal(pushErr)
 	}
 
-	if _, ok := pErr.GetDetail().(*roachpb.HandledRetryableTxnError); !ok {
-		t.Fatalf("expected HandledRetryableTxnError, got: %v", pErr)
+	if _, ok := pErr.GetDetail().(*roachpb.RetryUsingTransactionError); !ok {
+		t.Fatalf("expected RetryUsingTransactionError, got: %v", pErr)
 	}
 	exp := "TransactionAbortedError(ABORT_REASON_ABORT_SPAN)"
 	if !testutils.IsPError(pErr, regexp.QuoteMeta(exp)) {

pkg/kv/txn_coord_sender.go

@@ -670,7 +670,7 @@ func (tc *TxnCoordSender) Send(
 					"unexpected retryable error at the client.Txn level: (%T) %s",
 					pErr.GetDetail(), pErr)
 			}
-		} else if _, ok := pErr.GetDetail().(*roachpb.HandledRetryableTxnError); ok {
+		} else if _, ok := pErr.GetDetail().(*roachpb.RetryUsingTransactionError); ok {
 			retriable = true
 		}
 
@@ -750,15 +750,15 @@ func (tc *TxnCoordSender) maybeRejectClientLocked(
 			roachpb.NewTransactionAbortedError(roachpb.ABORT_REASON_CLIENT_REJECT), &tc.mu.txn)
 		if tc.typ == client.LeafTxn {
 			// Leaf txns return raw retriable errors (which get handled by the
-			// root) rather than HandledRetryableTxnError.
+			// root) rather than RetryUsingTransactionError.
 			return abortedErr
 		}
 		newTxn := roachpb.PrepareTransactionForRetry(
 			ctx, abortedErr,
 			// priority is not used for aborted errors
 			roachpb.NormalUserPriority,
 			tc.clock)
-		return roachpb.NewError(roachpb.NewHandledRetryableTxnError(
+		return roachpb.NewError(roachpb.NewRetryUsingTransactionError(
 			abortedErr.Message, tc.mu.txn.ID, newTxn))
 	}
 
@@ -806,7 +806,7 @@ func (tc *TxnCoordSender) UpdateStateOnRemoteRetryableErr(
 // needs to call tc.mu.txn.Update(pErr.GetTxn()).
 func (tc *TxnCoordSender) handleRetryableErrLocked(
 	ctx context.Context, pErr *roachpb.Error,
-) *roachpb.HandledRetryableTxnError {
+) *roachpb.RetryUsingTransactionError {
 	// If the error is a transaction retry error, update metrics to
 	// reflect the reason for the restart.
 	// TODO(spencer): this code path does not account for retry errors
@@ -826,8 +826,8 @@ func (tc *TxnCoordSender) handleRetryableErrLocked(
 	errTxnID := pErr.GetTxn().ID
 	newTxn := roachpb.PrepareTransactionForRetry(ctx, pErr, tc.mu.userPriority, tc.clock)
 
-	// We'll pass a HandledRetryableTxnError up to the next layer.
-	retErr := roachpb.NewHandledRetryableTxnError(
+	// We'll pass a RetryUsingTransactionError up to the next layer.
+	retErr := roachpb.NewRetryUsingTransactionError(
 		pErr.Message,
 		errTxnID, // the id of the transaction that encountered the error
 		newTxn)

pkg/kv/txn_coord_sender_server_test.go

@@ -134,7 +134,7 @@ func TestHeartbeatFindsOutAboutAbortedTransaction(t *testing.T) {
 	// Check that further sends through the aborted txn are rejected. The
 	// TxnCoordSender is supposed to synthesize a TransactionAbortedError.
 	if err := txn.CommitOrCleanup(ctx); !testutils.IsError(
-		err, "HandledRetryableTxnError: TransactionAbortedError",
+		err, "RetryUsingTransactionError: TransactionAbortedError",
 	) {
 		t.Fatalf("expected aborted error, got: %s", err)
 	}

pkg/kv/txn_coord_sender_test.go

@@ -562,7 +562,7 @@ func TestTxnCoordSenderAddIntentOnError(t *testing.T) {
 }
 
 func assertTransactionRetryError(t *testing.T, e error) {
-	if retErr, ok := e.(*roachpb.HandledRetryableTxnError); ok {
+	if retErr, ok := e.(*roachpb.RetryUsingTransactionError); ok {
 		if !testutils.IsError(retErr, "TransactionRetryError") {
 			t.Fatalf("expected the cause to be TransactionRetryError, but got %s",
 				retErr)
@@ -573,7 +573,7 @@ func assertTransactionRetryError(t *testing.T, e error) {
 }
 
 func assertTransactionAbortedError(t *testing.T, e error) {
-	if retErr, ok := e.(*roachpb.HandledRetryableTxnError); ok {
+	if retErr, ok := e.(*roachpb.RetryUsingTransactionError); ok {
 		if !testutils.IsError(retErr, "TransactionAbortedError") {
 			t.Fatalf("expected the cause to be TransactionAbortedError, but got %s",
 				retErr)

pkg/roachpb/errors.go

@@ -324,15 +324,15 @@ func (*TransactionAbortedError) canRestartTransaction() TransactionRestart {
 var _ ErrorDetailInterface = &TransactionAbortedError{}
 var _ transactionRestartError = &TransactionAbortedError{}
 
-func (e *HandledRetryableTxnError) Error() string {
+func (e *RetryUsingTransactionError) Error() string {
 	return e.message(nil)
 }
 
-func (e *HandledRetryableTxnError) message(_ *Error) string {
-	return fmt.Sprintf("HandledRetryableTxnError: %s", e.Msg)
+func (e *RetryUsingTransactionError) message(_ *Error) string {
+	return fmt.Sprintf("RetryUsingTransactionError: %s", e.Msg)
 }
 
-var _ ErrorDetailInterface = &HandledRetryableTxnError{}
+var _ ErrorDetailInterface = &RetryUsingTransactionError{}
 
 // NewTransactionAbortedError initializes a new TransactionAbortedError.
 func NewTransactionAbortedError(reason TransactionAbortedReason) *TransactionAbortedError {
@@ -341,14 +341,14 @@ func NewTransactionAbortedError(reason TransactionAbortedReason) *TransactionAbo
 	}
 }
 
-// NewHandledRetryableTxnError initializes a new HandledRetryableTxnError.
+// NewRetryUsingTransactionError initializes a new RetryUsingTransactionError.
 //
 // txnID is the ID of the transaction being restarted.
 // txn is the transaction that the client should use for the next attempts.
-func NewHandledRetryableTxnError(
+func NewRetryUsingTransactionError(
 	msg string, txnID uuid.UUID, txn Transaction,
-) *HandledRetryableTxnError {
-	return &HandledRetryableTxnError{Msg: msg, TxnID: txnID, Transaction: txn}
+) *RetryUsingTransactionError {
+	return &RetryUsingTransactionError{Msg: msg, TxnID: txnID, Transaction: txn}
 }
 
 // NewTransactionPushError initializes a new TransactionPushError.