errors: introduce a general-purpose modular error library with good properties#37121
errors: introduce a general-purpose modular error library with good properties#37121craig[bot] merged 22 commits intocockroachdb:masterfrom
Conversation
|
This change is |
cfe233b to
f7f9fc4
Compare
2b6a997 to
86082e0
Compare
2c05229 to
d8fbe3c
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
r1 LGTM except we should look at contributing the source line context stuff to testify/assert and using that instead (we already use it quite a bit)
I have some some comments on r2 for now
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz and @RaduBerinde)
pkg/errors/errbase_api.go, line 21 at r2 (raw file):
// UnwrapOnce accesses the direct cause of the error if any, otherwise // returns nil. var UnwrapOnce func(err error) error = errbase.UnwrapOnce
Defining copies of functions like this is pretty hacky, and makes navigating code annoying.. And same goes for using a lot of type aliases. Adding different names for the same thing makes it harder to figure out what's going on. It's fine to use these on a limited basis, but this pretty much the main API..
What's the problem with calling this stuff from errbase directly? Alternatively, can't all the stuff in errbase just live in errors (and stuff on top of it can be in another package)? (although errors is already used by at least two different packages, so maybe it's good to have a different name)
pkg/errors/errbase_api.go, line 46 at r2 (raw file):
var GetAllSafeDetails func(err error) []SafeDetailPayload = errbase.GetAllSafeDetails // GetSafeDetails collects the safe details from the given error
These comments are copies of comments in errbase and are likely to go stale.
pkg/errors/errbase/data.proto, line 10 at r2 (raw file):
message EncodedError { // An error is either... oneof error {
[nit] I think we don't have tabs in proto files usually
pkg/errors/errbase/data.proto, line 25 at r2 (raw file):
string message = 1; // always a fully qualified go type name, which will
[nit] This commenting style is kind of odd, especially when you want to look at a specific field.
pkg/errors/errbase/data.proto, line 27 at r2 (raw file):
// always a fully qualified go type name, which will // be used to look up a decoding function. string type_name = 2;
I think using fully qualified go type names is flawed. What happens when we move code around? We will lose backward compatibility just because this name changes. I think instead this should be a more generic "handler id", and let the caller of Register come up with a name. And we should have a list of "deprecated id"s which prevent registering with that name again.
pkg/testutils/simplecheck.go, line 31 at r1 (raw file):
// This file provides assertions and checks for Go tests, two // facilities which also provide somes lines of source code around the
[nit] some
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/util/protoutil/marshal.go, line 33 at r2 (raw file):
// SimpleMessage forwards the proto.Message interface and can be used // when the additional methods on Message are not relevant.
What's the point of this alias? The two names can be used interchangeably so I don't see what it buys us
3401c79 to
ae7ea98
Compare
Release note: None
... since the error library's Wrap is decent enough. Release note: None
Release note: None
Release note: None
The new function `Flatten()` provides the bridge between the new error library on the remainder of the code using pgerror.Error. Release note: None
…rors Release note: None
Release note: None
This replaces code that mutates `pgerror.Error` objects in place by code that adds hints and detail annotations agnostic of the particular error type. Release note: None
Release note: None
Prior to this patch, the opt code was special-casing pgerror.Error and required an object of this type to be present in the causal chain to work properly. This patch removes the requirement by using the facilities from the errors library. Release note: None
Release note: None
Release note: None
`GetPGCause()` is annoying/counter-productive because it peels all the decorations around a `pgerror.Error`, including details that may be helpful to troubleshooting. It is also not friendly to the new errors library which is able to define pg error code without using `pgerror.Error`. This patch removes all its uses but one (last remaining in `distsqlpb`, modified in a later patch) and replaces them, when applicable, by `GetPGCode()` which is friendly to the new errors library. Release note: None
This patch introduces support for receiving
decorated (non-`pgerror.Error`) errors from distsql flows. It also
redefines the `pgerror` helpers to use the new errors library, so as
to not instantiate `pgerror.Error` object until/unless `Flatten()` is
called.
This way errors remain as regular errors (and not `pgerror.Error`)
throughout most of the CockroachDB internals, and only become
`pgerror.Error` ("flattened") at very few points:
- to generate a compatibility payload when sending an error to 19.1 nodes
- in pgwire to generate packets for clients
- in the output of SHOW SYNTAX
- in some tests
Release note: None
Release note: None
Release note: None
A while ago I had changed many calls to `errors.Errorf`/`errors.New`/`fmt.Errorf`/`errors.Wrap` to use `pgerror` error constructors instead. The goal was to ensure the error objects were decorated with safe details suitable for reporting. However using those APIs also required a pg error code, even in cases where none was obviously available. So I chose then to use the pg code "CodeDataException" as a dummy value. Now that the `errors` library is able to annotate errors in all the good ways, the `pgerror` interface is not needed any more. This commit reverts the past change. This change is nearly all mechanical, using perl. The only manual change is to the function `wrapRowErr` in the `importccl` package. Release note: None
... instead of the aliased definitions in `pgerror`. (This change is entirely mechanical, using perl) Release note: None
Release note: None
... where appropriate. This drops the dependency on `util/log` in many cases. (This change was mechanical, using perl) Release note: None
... and remove pgwire/pgerror/codes.go Release note: None
Previously `teamcity-testrace.sh` would run `testrace` passing all the modified packages simultaneously in the PKG make var. This causes `go test` to issue all the tests concurrently, regardless of available hardware. If there are sufficiently many packages modified, this overloads the machines, makes test run much slower than usual, and triggers bad behavior (timeouts). This patch alleviates the potential problem by running the tests one after the other. Release note: None
|
bors r+ (crossing fingers) |
37121: errors: introduce a general-purpose modular error library with good properties r=knz a=knz This PR introduces an error library implemented after the principles laid out in #36987. It subsumes the following two PRs: - #38127 *: demonstrate uses of the errors library - review approval: #38127 (comment) - #37813 sql, *: simplify remove dependencies on pgerror, util/log - review approval: #37813 (comment) Release note: None Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
|
nvm I wasn't looking in the right place |
Build succeeded |
This PR introduces an error library implemented after the principles laid out in #36987.
It subsumes the following two PRs:
Release note: None