roachpb: clarify parameter to (*Error).SetDetail #56581
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 7 of 7 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @tbg)
pkg/kv/kvserver/store.go, line 1740 at r2 (raw file):
annotatedCtx := repl.AnnotateCtx(ctx) if _, pErr := repl.redirectOnOrAcquireLease(annotatedCtx); pErr != nil { if _, ok := pErr.GoError().(*roachpb.NotLeaseHolderError); !ok {
Do we want GoError in cases like these where we're testing for a specific ErrorDetail type? Doesn't that have a chance of being more expensive than GetDetail, due to the creation of an UnhandledRetryableError?
I wonder whether this is common enough that we should invest in a HasDetail or HasDetailType method. Maybe even one that accepts an ErrorDetailType. That's a drive-by comment so don't pursue it unless you think it's a good idea.
pkg/roachpb/errors.go, line 311 at r2 (raw file):
} // If the specific error type exists in the detail union, set it. if !e.Detail.SetInner(detail) {
Does it make sense for this to ever return false anymore? Should we assert that it doesn't? Or even create a MustSetInner method like we have elsewhere?
This code path is only hit when `err` is not an `*internalError`. Release note: None
The circuitous error handling has long been a source of confusion,
but there are some straightforward modifications, one of which this
commit makes: there is no reason to pass an `error` to `SetDetail`;
it can always be an `ErrorDetail`. In particular, this prevents a
situation which caused problems in the past, namely that of passing
an `*Error` to `SetDetail` (which in itself is a method on `*Error`).
This is just not possible now, period, because we're also changing
`internalError` to *not* implement `ErrorDetailInterface`.
Most of the time in this change was verifying that nothing in the
code relies on `GetDetail()` always returning a non-nil value for
a non-nil `pErr`. I did a full review (prod and tests) and found
a few such instances, but the vast majority of callers are of the
pattern
```go
if pErr != nil {
if _, ok := pErr.GetDetail().(*roachpb.SomeErrorDetail); ok {
return specialHandling()
}
return pErr
}
```
and thus required no change.
Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/store.go, line 1740 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Do we want
GoErrorin cases like these where we're testing for a specific ErrorDetail type? Doesn't that have a chance of being more expensive thanGetDetail, due to the creation of anUnhandledRetryableError?I wonder whether this is common enough that we should invest in a
HasDetailorHasDetailTypemethod. Maybe even one that accepts anErrorDetailType. That's a drive-by comment so don't pursue it unless you think it's a good idea.
Huh, I'm not sure why I even changed this line. The original was fine and didn't incur the alloc.
pkg/roachpb/errors.go, line 311 at r2 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Does it make sense for this to ever return false anymore? Should we assert that it doesn't? Or even create a
MustSetInnermethod like we have elsewhere?
good idea, #56600
|
bors r=nvanbenschoten |
|
Build succeeded: |
There was a problem hiding this comment.
In particular, this prevents a
situation which caused problems in the past, namely that of passing
an *Error to SetDetail (which in itself is a method on *Error).
This is just not possible now, period, because we're also changing
internalError to not implement ErrorDetailInterface.
I think this is a fine patch, but I'm a bit confused about this motivation. It is not true that a *Error could have been passed to SetDetail() - *Error does not implement error. What you could have done is pErr.SetDetail(pErr2.GoError()). Is that what you were trying to prevent?
Reviewable status:
There was a problem hiding this comment.
I was a bit sloppy in the description - GoError() can return an *internalError which is structually equivalent to *Error. The whole cyclical nature of everything here just got me really confused so it had to go.
Reviewable status:
There was a problem hiding this comment.
The "true" motivation is that I wanted to do #56603, and so I needed some initial cleanups to get some sanity going :-)
Reviewable status:
|
Oh I had forgotten that internalError is just an alias for Error. Yeah,
pretty messy.
…On Thu, Nov 12, 2020 at 10:37 AM Tobias Grieger ***@***.***> wrote:
***@***.**** commented on this pull request.
The "true" motivation is that I wanted to do #56603
<#56603>, and so I needed
some initial cleanups to get some sanity going :-)
*Reviewable
<https://reviewable.io/reviews/cockroachdb/cockroach/56581#-:-MLx5X3-9orRVc4MklRx:b-1wkdqw>*
status: [image:
] complete! 1 of 0 LGTMs obtained (waiting on
@nvanbenschoten <https://github.com/nvanbenschoten>)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#56581 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC4C4MNA2WK6H2NG5Q5YILSPP6LRANCNFSM4TSHBCWQ>
.
|
roachpb: clarify parameter to (*Error).SetDetail
The circuitous error handling has long been a source of confusion,
but there are some straightforward modifications, one of which this
commit makes: there is no reason to pass an
errortoSetDetail;it can always be an
ErrorDetail. In particular, this prevents asituation which caused problems in the past, namely that of passing
an
*ErrortoSetDetail(which in itself is a method on*Error).This is just not possible now, period, because we're also changing
internalErrorto not implementErrorDetailInterface.Most of the time in this change was verifying that nothing in the
code relies on
GetDetail()always returning a non-nil value fora non-nil
pErr. I did a full review (prod and tests) and founda few such instances, but the vast majority of callers are of the
pattern
and thus required no change.
Release note: None