Skip to content

Commit

Permalink
roachpb: remove deprecated fields from Error
Browse files Browse the repository at this point in the history 
CRDB 21.2 already populates `Errors.EncodedError`, so we can remove the
deprecated fields.

I was motivated by this when working on a follow-up to #73602 and
ended up trying to marshal an `Error` to JSON, which panics since
it ends up trying to use `reflect` to get an unexported value (due
to the `customname` tags on the deprecated fields). This behavior
persists even with the `json: "-"` tag, by the way, indicating
that we should never use `(gogoproto.customname)` to make a field
private. It's also just one more papercut that comes with using
`gogoproto` but that's a much bigger fish to fry.

Release note: None
  • Loading branch information
@tbg
tbg committed Dec 10, 2021
1 parent 3bed3a6 commit 66c8399
Show file tree
Hide file tree
Showing 5 changed files with 296 additions and 492 deletions.
11 changes: 5 additions & 6 deletions pkg/kv/kvclient/kvcoord/dist_sender_test.go
View file
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
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

0 comments on commit 66c8399

Please sign in to comment.