roachpb: use cockroachdb/errors.EncodedError inside of internalError #54939
Assignees
Labels
A-error-handling
Error messages, error propagations/annotations
A-kv
Anything in KV that doesn't belong in a more specific category.
A-kv-server
Relating to the KV-level RPC server
C-enhancement
Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Comments
ajwerner
added
the
A-error-handling
|
Hi @ajwerner, I've guessed the C-ategory of your issue and suitably labeled it. Please re-label if inaccurate. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
blathers-crl
bot
added
the
C-enhancement
knz
added
A-kv
|
cc @tbg for triage and prioritization |
|
I also just ran into a snag with this over in #54746. The whole |
|
The bear poked me, so I will poke back. In the context of decommissioned nodes, we need a way for the KV API to return a general error from the RPC layer that we can sniff out as meaning that access to the cluster has been revoked. I don't want to shoehorn this through an error detail, so will take a look at migrating Error to use EncodedError. |
|
Started cleaning up around Error a little bit (#56581), though haven't touched this issue yet. Planning on doing so tomorrow. |
This was referenced Nov 12, 2020
tbg
added a commit
to tbg/cockroach
that referenced
this issue
Nov 12, 2020
`roachpb.Error` has long been poor to work with. It could only transport structured errors for a hard-coded enum message (`ErrorDetail`) and had a complex and circuituous interaction with the standard library's `error` type: `pErr.GoError()` would return an `error` that was sometimes just `pErr` itself (hidden behind an identical type); other times it was an `UnhandledRetryableError` wrapping `pErr`, and then of course sometimes it would be the structured error from which `pErr` was created in the first place. Thanks to `cockroachdb/errors`, there's a much better option around, namely `errorspb.EncodedError` which has powerful capabilities to essentially carry any error across RPC boundaries (and notably does so "out of the box" for protobuf-backed errors) while retaining its original structure, as long as the error type is known on both sides of the RPC. The simplifications that result from this primitive would be enough to motivate this change in itself, but additionally after this change we will be able to reliably detect context cancellations, etc, without relying on error-prone and insecure string matching. Going into more detail, this commit makes the following changes: - adds an `EncodedError` to `Error` - deprecates the following fields on `Error`, which are henceforth deduced from the `EncodedError` where present: - `transaction_restart` - `message` - `detail` These fields are populated for the time being so that 20.2 nodes understand the errors emitted by 21.1 nodes. In 21.2, we can drop them all. - deprecates `(*Error).SetDetail`, which has only one real caller anyway (and that caller will be able to replace EncodedError instead once 21.2 comes around). - makes `(*Error).SafeFormat` redact properly based on `EncodedError` (yay!), though there's a larger TODO that's left for a follow-up change (around the fact that error details can depend on the surrounding Error for rendering, particularly relevant when SetTxn is called) We can capitalize on EncodedError before the 21.2 cycle, but this is left for future PRs. Fixes cockroachdb#54939. Release note: None
tbg
added a commit
to tbg/cockroach
that referenced
this issue
Nov 12, 2020
`roachpb.Error` has long been poor to work with. It could only transport structured errors for a hard-coded enum message (`ErrorDetail`) and had a complex and circuituous interaction with the standard library's `error` type: `pErr.GoError()` would return an `error` that was sometimes just `pErr` itself (hidden behind an identical type); other times it was an `UnhandledRetryableError` wrapping `pErr`, and then of course sometimes it would be the structured error from which `pErr` was created in the first place. Thanks to `cockroachdb/errors`, there's a much better option around, namely `errorspb.EncodedError` which has powerful capabilities to essentially carry any error across RPC boundaries (and notably does so "out of the box" for protobuf-backed errors) while retaining its original structure, as long as the error type is known on both sides of the RPC. The simplifications that result from this primitive would be enough to motivate this change in itself, but additionally after this change we will be able to reliably detect context cancellations, etc, without relying on error-prone and insecure string matching. Going into more detail, this commit makes the following changes: - adds an `EncodedError` to `Error` - deprecates the following fields on `Error`, which are henceforth deduced from the `EncodedError` where present: - `transaction_restart` - `message` - `detail` These fields are populated for the time being so that 20.2 nodes understand the errors emitted by 21.1 nodes. In 21.2, we can drop them all. - deprecates `(*Error).SetDetail`, which has only one real caller anyway (and that caller will be able to replace EncodedError instead once 21.2 comes around). - makes `(*Error).SafeFormat` redact properly based on `EncodedError` (yay!), though there's a larger TODO that's left for a follow-up change (around the fact that error details can depend on the surrounding Error for rendering, particularly relevant when SetTxn is called) We can capitalize on EncodedError before the 21.2 cycle, but this is left for future PRs. Fixes cockroachdb#54939. Release note: None
tbg
added a commit
to tbg/cockroach
that referenced
this issue
Nov 13, 2020
`roachpb.Error` has long been poor to work with. It could only transport structured errors for a hard-coded enum message (`ErrorDetail`) and had a complex and circuituous interaction with the standard library's `error` type: `pErr.GoError()` would return an `error` that was sometimes just `pErr` itself (hidden behind an identical type); other times it was an `UnhandledRetryableError` wrapping `pErr`, and then of course sometimes it would be the structured error from which `pErr` was created in the first place. Thanks to `cockroachdb/errors`, there's a much better option around, namely `errorspb.EncodedError` which has powerful capabilities to essentially carry any error across RPC boundaries (and notably does so "out of the box" for protobuf-backed errors) while retaining its original structure, as long as the error type is known on both sides of the RPC. The simplifications that result from this primitive would be enough to motivate this change in itself, but additionally after this change we will be able to reliably detect context cancellations, etc, without relying on error-prone and insecure string matching. Going into more detail, this commit makes the following changes: - adds an `EncodedError` to `Error` - deprecates the following fields on `Error`, which are henceforth deduced from the `EncodedError` where present: - `transaction_restart` - `message` - `detail` These fields are populated for the time being so that 20.2 nodes understand the errors emitted by 21.1 nodes. In 21.2, we can drop them all. - deprecates `(*Error).SetDetail`, which has only one real caller anyway (and that caller will be able to replace EncodedError instead once 21.2 comes around). - makes `(*Error).SafeFormat` redact properly based on `EncodedError` (yay!), though there's a larger TODO that's left for a follow-up change (around the fact that error details can depend on the surrounding Error for rendering, particularly relevant when SetTxn is called) We can capitalize on EncodedError before the 21.2 cycle, but this is left for future PRs. Fixes cockroachdb#54939. Release note: None
tbg
added a commit
to tbg/cockroach
that referenced
this issue
Nov 13, 2020
`roachpb.Error` has long been poor to work with. It could only transport structured errors for a hard-coded enum message (`ErrorDetail`) and had a complex and circuituous interaction with the standard library's `error` type: `pErr.GoError()` would return an `error` that was sometimes just `pErr` itself (hidden behind an identical type); other times it was an `UnhandledRetryableError` wrapping `pErr`, and then of course sometimes it would be the structured error from which `pErr` was created in the first place. Thanks to `cockroachdb/errors`, there's a much better option around, namely `errorspb.EncodedError` which has powerful capabilities to essentially carry any error across RPC boundaries (and notably does so "out of the box" for protobuf-backed errors) while retaining its original structure, as long as the error type is known on both sides of the RPC. The simplifications that result from this primitive would be enough to motivate this change in itself, but additionally after this change we will be able to reliably detect context cancellations, etc, without relying on error-prone and insecure string matching. Going into more detail, this commit makes the following changes: - adds an `EncodedError` to `Error` - deprecates the following fields on `Error`, which are henceforth deduced from the `EncodedError` where present: - `transaction_restart` - `message` - `detail` These fields are populated for the time being so that 20.2 nodes understand the errors emitted by 21.1 nodes. In 21.2, we can drop them all. - deprecates `(*Error).SetDetail`, which has only one real caller anyway (and that caller will be able to replace EncodedError instead once 21.2 comes around). - makes `(*Error).SafeFormat` redact properly based on `EncodedError` (yay!), though there's a larger TODO that's left for a follow-up change (around the fact that error details can depend on the surrounding Error for rendering, particularly relevant when SetTxn is called) We can capitalize on EncodedError before the 21.2 cycle, but this is left for future PRs. Fixes cockroachdb#54939. Release note: None
craig bot
pushed a commit
that referenced
this issue
Nov 13, 2020
56603: roachpb: use EncodedError in Error r=knz a=tbg roachpb: use EncodedError in Error `roachpb.Error` has long been poor to work with. It could only transport structured errors for a hard-coded enum message (`ErrorDetail`) and had a complex and circuituous interaction with the standard library's `error` type: `pErr.GoError()` would return an `error` that was sometimes just `pErr` itself (hidden behind an identical type); other times it was an `UnhandledRetryableError` wrapping `pErr`, and then of course sometimes it would be the structured error from which `pErr` was created in the first place. Thanks to `cockroachdb/errors`, there's a much better option around, namely `errorspb.EncodedError` which has powerful capabilities to essentially carry any error across RPC boundaries (and notably does so "out of the box" for protobuf-backed errors) while retaining its original structure, as long as the error type is known on both sides of the RPC. The simplifications that result from this primitive would be enough to motivate this change in itself, but additionally after this change we will be able to reliably detect context cancellations, etc, without relying on error-prone and insecure string matching. Going into more detail, this commit makes the following changes: - adds an `EncodedError` to `Error` - deprecates the following fields on `Error`, which are henceforth deduced from the `EncodedError` where present: - `transaction_restart` - `message` - `detail` These fields are populated for the time being so that 20.2 nodes understand the errors emitted by 21.1 nodes. In 21.2, we can drop them all. - deprecates `(*Error).SetDetail`, which has only one real caller anyway (and that caller will be able to replace EncodedError instead once 21.2 comes around). - makes `(*Error).SafeFormat` redact properly based on `EncodedError` (yay!), though there's a larger TODO that's left for a follow-up change (around the fact that error details can depend on the surrounding Error for rendering, particularly relevant when SetTxn is called) We can capitalize on EncodedError before the 21.2 cycle, but this is left for future PRs. Fixes #54939. Release note: None Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Is your feature request related to a problem? Please describe.
roachpb.Error is a wrapper around a number of concrete error types important for the cockroach transactional model. The object can be constructed from any go error. Go errors which are themselves implementations of the relevant concrete errors are properly wrapper for serialization. All other errors, however, are captured as just a string inside of the
roachpb.internalErrorstruct. The problem with this error is that it represents the errors used to construct it as a string, throwing away all structured information. This further means that any structured information will not be transmitted over the network.This is not obviously a big problem today. The most important errors to detect programtically (in my opinion) which are today lost are
context.Canceledandcontext.DeadlineExceeded. However, it may not be so important to detect these errors coming from remote procedure calls as the errors should be reproduced on the client-side as most of these deadlines and cancelations should flow from the client anyway. Unfortunately, there is no gaurantee that the context error will always be checked or that it will be preferred over the returned error from the RPC. In fact, we generally go out of our way to prefer the error being propagated from the call path.Describe the solution you'd like
I'd like to see the
roachpb.internalErroradopt thecockroachdb/errors.EncodedErrorfor its representation of the error used to construct it (when not a roachpb defined error). This would allow structure to be preserved.Additional context
This approach was utilized for
execinfrapb.Errorin #37121.The text was updated successfully, but these errors were encountered: