Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

roachpb: remove deprecated fields from Error #73686

Closed
wants to merge 1 commit into from
+296 −492
Closed

roachpb: remove deprecated fields from Error #73686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions pkg/kv/kvclient/kvcoord/dist_sender_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3559,12 +3559,11 @@ func TestErrorIndexAlignment(t *testing.T) {
var testFn simpleSendFn = func(ctx context.Context, ba roachpb.BatchRequest) (*roachpb.BatchResponse, error) {
reply := ba.CreateReply()
if nthRequest == tc.nthPartialBatch {
reply.Error = &roachpb.Error{
// The relative index is always 0 since
// we return an error for the first
// request of the nthPartialBatch.
Index: &roachpb.ErrPosition{Index: 0},
}
reply.Error = roachpb.NewError(errors.Errorf("injected error on batch #%d", nthRequest))
// The relative index is always 0 since
// we return an error for the first
// request of the nthPartialBatch.
reply.Error.SetErrorIndex(0)
}
nthRequest++
return reply, nil
Expand Down
176 changes: 49 additions & 127 deletions pkg/roachpb/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/protoutil"
"github.com/cockroachdb/cockroach/pkg/util/uuid"
"github.com/cockroachdb/errors"
"github.com/cockroachdb/errors/errorspb"
_ "github.com/cockroachdb/errors/extgrpc" // register EncodeError support for gRPC Status
"github.com/cockroachdb/redact"
)
Expand Down Expand Up @@ -149,30 +148,12 @@ func NewError(err error) *Error {
if err == nil {
return nil
}
e := &Error{
EncodedError: errors.EncodeError(context.Background(), err),
if intErr, ok := err.(*internalError); ok {
return (*Error)(intErr)
}

// This block is deprecated behavior retained for compat with
// 20.2 nodes. It makes sure that if applicable, deprecatedMessage,
// ErrorDetail, and deprecatedTransactionRestart are set.
{
if intErr, ok := err.(*internalError); ok {
*e = *(*Error)(intErr)
} else if detail := ErrorDetailInterface(nil); errors.As(err, &detail) {
e.deprecatedMessage = detail.message(e)
if r, ok := detail.(transactionRestartError); ok {
e.deprecatedTransactionRestart = r.canRestartTransaction()
} else {
e.deprecatedTransactionRestart = TransactionRestart_NONE
}
e.deprecatedDetail.MustSetInner(detail)
e.checkTxnStatusValid()
} else {
e.deprecatedMessage = err.Error()
}
return &Error{
EncodedError: errors.EncodeError(context.Background(), err),
}
return e
}

// NewErrorWithTxn creates an Error from the given error and a transaction.
Expand All @@ -194,49 +175,43 @@ func NewErrorf(format string, a ...interface{}) *Error {
return NewError(err)
}

func (e *Error) decode() error {
if e.EncodedError.Error == nil {
// DecodeError would crash on this EncodedError, so terminate it here.
return errors.AssertionFailedf("Error with nil EncodedError")
}
return errors.DecodeError(context.Background(), e.EncodedError)
}

// SafeFormat implements redact.SafeFormatter.
func (e *Error) SafeFormat(s redact.SafePrinter, _ rune) {
if e == nil {
s.Print(nil)
return
}

if e.EncodedError != (errors.EncodedError{}) {
err := errors.DecodeError(context.Background(), e.EncodedError)
var iface ErrorDetailInterface
if errors.As(err, &iface) {
// Deprecated code: if there is a detail and the message produced by the detail
// (which gets to see the surrounding Error) is different, then use that message.
// What is likely the cause of that is that someone passed an updated Transaction
// to the Error via SetTxn, and the error detail prints that txn.
//
// TODO(tbg): change SetTxn so that instead of stashing the transaction on the
// Error struct, it wraps the EncodedError with an error containing the updated
// txn. Make GetTxn retrieve the first one it sees (overridden by UnexposedTxn
// while it's still around). Remove the `message(Error)` methods from ErrorDetailInterface.
// We also have to remove GetDetail() in the process since it doesn't understand the
// wrapping; instead we need a method that looks for a specific kind of detail, i.e.
// we basically want to use `errors.As` instead.
deprecatedMsg := iface.message(e)
if deprecatedMsg != err.Error() {
s.Print(deprecatedMsg)
return
}
}
s.Print(err)
} else {
// TODO(tbg): remove this block in the 21.2 cycle and rely on EncodedError
// always being populated.
switch t := e.GetDetail().(type) {
case nil:
s.Print(e.deprecatedMessage)
default:
// We have a detail and ignore e.deprecatedMessage. We do assume that if a detail is
// present, e.deprecatedMessage does correspond to that detail's message. This
// assumption is not enforced but appears sane.
s.Print(t)
err := e.decode()
var iface ErrorDetailInterface
if errors.As(err, &iface) {
// Deprecated code: if there is a detail and the message produced by the detail
// (which gets to see the surrounding Error) is different, then use that message.
// What is likely the cause of that is that someone passed an updated Transaction
// to the Error via SetTxn, and the error detail prints that txn.
//
// TODO(tbg): change SetTxn so that instead of stashing the transaction on the
// Error struct, it wraps the EncodedError with an error containing the updated
// txn. Make GetTxn retrieve the first one it sees (overridden by UnexposedTxn
// while it's still around). Remove the `message(Error)` methods from ErrorDetailInterface.
// We also have to remove GetDetail() in the process since it doesn't understand the
// wrapping; instead we need a method that looks for a specific kind of detail, i.e.
// we basically want to use `errors.As` instead.
deprecatedMsg := iface.message(e)
if deprecatedMsg != err.Error() {
s.Print(deprecatedMsg)
return
}
}
s.Print(err)

if txn := e.GetTxn(); txn != nil {
s.SafeString(": ")
Expand All @@ -251,14 +226,8 @@ func (e *Error) String() string {

// TransactionRestart returns the TransactionRestart for this Error.
func (e *Error) TransactionRestart() TransactionRestart {
if e.EncodedError == (errorspb.EncodedError{}) {
// Legacy code.
//
// TODO(tbg): delete in 21.2.
return e.deprecatedTransactionRestart
}
var iface transactionRestartError
if errors.As(errors.DecodeError(context.Background(), e.EncodedError), &iface) {
if errors.As(e.decode(), &iface) {
return iface.canRestartTransaction()
}
return TransactionRestart_NONE
Expand Down Expand Up @@ -334,39 +303,28 @@ const (
NumErrors int = 43
)

// GoError returns a Go error converted from Error. If the error is a transaction
// retry error, it returns the error itself wrapped in an UnhandledRetryableError.
// Otherwise, if an error detail is present, is is returned (i.e. the result will
// match GetDetail()). Otherwise, returns the error itself masqueraded as an `error`.
// GoError returns a Go error wrapped in this Error. If the error is a
// transaction retry error, it returns the error itself wrapped in an
// UnhandledRetryableError.
//
// Note that this is a lossy conversion: the UnexposedTxn, OriginNode, Index,
// and Now fields will be lost during roachpb.NewError(pErr.GoError()).
func (e *Error) GoError() error {
if e == nil {
return nil
}
if e.EncodedError != (errorspb.EncodedError{}) {
err := errors.DecodeError(context.Background(), e.EncodedError)
var iface transactionRestartError
if errors.As(err, &iface) {
if txnRestart := iface.canRestartTransaction(); txnRestart != TransactionRestart_NONE {
// TODO(tbg): revisit this unintuitive error wrapping here and see if
// a better solution can be found.
return &UnhandledRetryableError{
PErr: *e,
}
err := e.decode()
var iface transactionRestartError
if errors.As(err, &iface) {
if txnRestart := iface.canRestartTransaction(); txnRestart != TransactionRestart_NONE {
// TODO(tbg): revisit this unintuitive error wrapping here and see if
// a better solution can be found.
return &UnhandledRetryableError{
PErr: *e,
}
}
return err
}

// Everything below is legacy behavior that can be deleted in 21.2.
if e.TransactionRestart() != TransactionRestart_NONE {
return &UnhandledRetryableError{
PErr: *e,
}
}
if detail := e.GetDetail(); detail != nil {
return detail
}
return (*internalError)(e)
return err
}

// GetDetail returns an error detail associated with the error, or nil otherwise.
Expand All @@ -375,14 +333,7 @@ func (e *Error) GetDetail() ErrorDetailInterface {
return nil
}
var detail ErrorDetailInterface
if e.EncodedError != (errorspb.EncodedError{}) {
errors.As(errors.DecodeError(context.Background(), e.EncodedError), &detail)
} else {
// Legacy behavior.
//
// TODO(tbg): delete in v21.2.
detail, _ = e.deprecatedDetail.GetInner().(ErrorDetailInterface)
}
errors.As(e.decode(), &detail)
return detail
}

Expand All @@ -404,35 +355,6 @@ func (e *Error) UpdateTxn(o *Transaction) {
} else {
e.UnexposedTxn.Update(o)
}
if sErr, ok := e.deprecatedDetail.GetInner().(ErrorDetailInterface); ok {
// Refresh the message as the txn is updated.
//
// TODO(tbg): deprecated, remove in 21.2.
e.deprecatedMessage = sErr.message(e)
}
e.checkTxnStatusValid()
}

// checkTxnStatusValid verifies that the transaction status is in-sync with the
// error detail.
func (e *Error) checkTxnStatusValid() {
// TODO(tbg): this will need to be updated when we
// remove all of these deprecated fields in 21.2.

txn := e.UnexposedTxn
err := e.deprecatedDetail.GetInner()
if txn == nil {
return
}
if e.deprecatedTransactionRestart == TransactionRestart_NONE {
return
}
if errors.HasType(err, (*TransactionAbortedError)(nil)) {
return
}
if txn.Status.IsFinalized() {
log.Fatalf(context.TODO(), "transaction unexpectedly finalized in (%T): %v", err, e)
}
}

// GetTxn returns the txn.
Expand Down
Loading