*: replace erroneous uses of Newf/Errorf by Wrap/Wrapf
#68707
Conversation
|
This change is |
|
Arguably we'd need a linter for this. |
|
geo lgtm |
There was a problem hiding this comment.
Reviewed 18 of 22 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @knz)
pkg/ccl/cmdccl/enc_utils/main.go, line 205 at r2 (raw file):
cipher, err := aes.NewCipher(key.rawKey) if err != nil { return nil, errors.Wrapf("could not build AES cipher for file %s", absPath)
err is not being wrapped.
pkg/cli/doctor.go, line 389 at r2 (raw file):
id = int(descpb.InvalidID) } else { return errors.Errorf(err, "failed to parse id %s", fields[3])
This is still the original Errorf but with Wrapf arguments.
pkg/kv/kvserver/store_split.go, line 338 at r2 (raw file):
leftRepl.writeStats.splitRequestCounts(rightRepl.writeStats) if err := s.addReplicaInternalLocked(rightRepl); err != nil { return errors.Wrapf(err, "unable to add replica %v: %s", rightRepl)
The %s for the err is still in the argument list.
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/cmdccl/enc_utils/main.go, line 205 at r2 (raw file):
Previously, mberhault (marc) wrote…
erris not being wrapped.
Good catch. Fixed.
pkg/cli/doctor.go, line 389 at r2 (raw file):
Previously, mberhault (marc) wrote…
This is still the original
Errorfbut withWrapfarguments.
Thanks fixed.
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/store_split.go, line 338 at r2 (raw file):
Previously, mberhault (marc) wrote…
The
%sfor theerris still in the argument list.
Fixed
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
schema
Reviewable status:
There was a problem hiding this comment.
kv and storage
Reviewed 3 of 22 files at r1, 1 of 3 files at r3, all commit messages.
Reviewable status:
There was a problem hiding this comment.
doctor says
Reviewable status:
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:
pkg/kv/kvserver/store_snapshot.go
Outdated
| @@ -1155,7 +1155,7 @@ func sendSnapshot( | |||
| // completed (such as defers that might be run after the previous message was | |||
| // received). | |||
| if unexpectedResp, err := stream.Recv(); err != io.EOF { | |||
| return errors.Errorf("%s: expected EOF, got resp=%v err=%v", to, unexpectedResp, err) | |||
| return errors.Wrapf(err, "%s: expected EOF, got resp=%v", to, unexpectedResp) | |||
There was a problem hiding this comment.
I haven't looked at the possible returns of stream.Recv(); but this might be problematic if err == nil since errors.Wrapf will return nil in that case.
|
@jeffswenson still waiting for your look at the proxy changes |
There was a problem hiding this comment.
sqlproxyccl changes 
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status:
|
bors r=mberhault,adityamaru,miretskiy,stevendanna,jaylim-crl,ajwerner,nvanbenschoten |
|
Build failed: |
|
flake on #68795 |
When constructing an error from another error, it is generally
erroneous to use `Errorf("...%v", err)`: this deconstructs the
original error back to a simple string and loses all its details.
It also makes the original error opaque to redactability, to be
considered an unsafe string, even if the original error already
contained a mix of safe and unsafe strings.
The proper approach is to use `errors.Wrap` / `errors.Wrapf`.
This preserves any embedded details inside the error object and its
redactability attributes.
This change was obtained by running the following command and fixing
up the results (i.e. test code was ignored):
```
git grep -E '(errors|fmt)\.(Errorf|Newf).*%v.*[eE]rr'|grep -v _test.go
```
Release note: None
|
bors r=mberhault,adityamaru,miretskiy,stevendanna,jaylim-crl,ajwerner,nvanbenschoten |
|
Build succeeded: |
This is an omnibus PR touching code by multiple teams. To be reviewed as follows:
See the description below for an explanation.
When constructing an error from another error, it is generally
erroneous to use
Errorf("...%v", err): this deconstructs theoriginal error back to a simple string and loses all its details.
It also makes the original error opaque to redactability, to be
considered an unsafe string, even if the original error already
contained a mix of safe and unsafe strings.
The proper approach is to use
errors.Wrap/errors.Wrapf.This preserves any embedded details inside the error object and its
redactability attributes.
This change was obtained by running the following command and fixing
up the results (i.e. test code was ignored):
Release note: None