avoid information loss on .GoError() #2629
Conversation
| if e.Index == nil { | ||
| return 0, false | ||
| } | ||
| return (*Error)(e).Index.Index, true |
There was a problem hiding this comment.
I think I used GetIndex() before. Changed.
tbg
force-pushed
the
int_error
branch
3 times, most recently
from
e1d8909 to
c451a05
Compare
|
PTAL. I actually cleaned up some more stuff in the vicinity (see third commit message), which brought some nice simplifications. |
|
That these errors carry indexes into the batch in which they are contained is starting to feel pretty dirty to me. What's the long term plan here? |
| @@ -465,8 +465,6 @@ func TestStoreExecuteNoop(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| // TestStoreErrorWithIndex verifies that an *errWithIndex returned from the | |||
| // replica is correctly translated into a *proto.Error with a corresponding | |||
| // Index. See the companion test TestBatchWithError. | |||
There was a problem hiding this comment.
Removed the test. TestBatchErrorWithIndex tests the same thing closer to
the source.
On Wed, Sep 23, 2015 at 10:53 AM, Tamir Duberstein <[email protected]
wrote:
In storage/store_test.go
#2629 (comment):@@ -465,8 +465,6 @@ func TestStoreExecuteNoop(t *testing.T) {
}
}-// TestStoreErrorWithIndex verifies that an *errWithIndex returned from the
-// replica is correctly translated into a *proto.Error with a corresponding
// Index. See the companion test TestBatchWithError.this comment is broken now
—
Reply to this email directly or view it on GitHub
https://github.com/cockroachdb/cockroach/pull/2629/files#r40212652.
|
LGTM |
| @@ -192,6 +193,11 @@ enum TransactionRestart { | |||
| IMMEDIATE = 2; | |||
| } | |||
|
|
|||
| // ErrPosition describes the position of an error in a Batch. | |||
| message ErrPosition { | |||
There was a problem hiding this comment.
Is there a value to this message versus just embedding optional int32 index in the errors which require it?
There was a problem hiding this comment.
yes, in proto3 primitive types are not nullable. there was a discussion on this in a previous PR
There was a problem hiding this comment.
I've moved up the comment about that from message Error.
|
LGTM |
|
LGTM. This change is definitely an improvement, but embedding the position doesn't sit well with me either. Is there something else we could do to provide the same functionality? For example, if ConditionalPut returned the key as well as the value, would that be enough for the SQL layer to construct the right error message for uniqueness violations? |
|
@bdarnell yes, that would be enough for the SQL layer. However, I think in general we will still want an error -> operation mapping for batches. |
|
The only location in which they're really "on" the batch is when they're on On Wed, Sep 23, 2015 at 10:52 AM, Tamir Duberstein <[email protected]
|
Yes, I thought about that too but it seemed fairly inflexible. With hindsight, while I think this change makes indexed errors workable, I agree it's not the cream of the crop yet and it might've been better to start small and just add a |
remove errWithIndex, which was used in `./storage` to wrap a batch
execution error along with the position at which it occurred.
That construct was prone to bugs and the later unwrapping into
a proto.Error meant that you could end up with a proto.Error
which carried more information than its associated ErrorDetail
(for example, `errWithIndex{err: ConditionalPutError, index:1}`
would translate into `proto.Error{Index: 1, Detail: ConditionalPutError}`
but since `ConditionalPutError` is a valid `ErrorDetail`, `GoError()`
would then return exactly that, losing the index.
Now an error whose relative position could be relevant at some future
point needs to implement `proto.IndexedError`, and then everything will
work as expected, regardless of whether the original error is an
`ErrorDetail` or not.
this is now possible since the index information is stored on the `ErrorDetail` itself, so exposing the `GoError()` only is sufficient.
plus some comment fixes
avoid information loss on .GoError()
fixes #2628.