-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
move from juju/errors
to pkg/errors
#7151
Conversation
Is it possible to get this PR merged? I am looking at doing the same in pd. I am starting to add stack traces in this PR tikv/pd#1200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you run dep update or any command related with dep
?
If no, it is pretty strange to me that there is some changes at Gopkg.lock
@zhexuany can you click to load the diff and comment on specifically what you find strange? |
cmd/benchfilesort/main.go
Outdated
@@ -80,15 +80,15 @@ func encodeRow(b []byte, row *comparableRow) ([]byte, error) { | |||
sc := &stmtctx.StatementContext{TimeZone: time.Local} | |||
body, err = codec.EncodeKey(sc, body, row.key...) | |||
if err != nil { | |||
return b, errors.Trace(err) | |||
return b, errors.WithStack(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not simply return b, err
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is first step of replacement, finally this case will be b, err
(1)replace all errors.Trace to errors.Withstack (2) delete the "middle call" of errors.WithStack
we can not repalce b, errors.Trace
directly, because same "root call" maybe introduce some standard errors or third-part lib error which is not pkg error
and need to do a wrap..
It's hard to use script to detect it's "middle call" or "root call", so replace them to WithStack, then manual delete middle one orz..
or @disksing is any other good advise~?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasonable and fair. Good luck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a deficiency in the design of pkg/errors that we could remedy by just adding a new function that will take a stack trace if it exists? This has been mentioned by others.
See this error implementation where they coalesce stack traces when printing: https://github.com/upspin/upspin/blob/master/errors/debug.go#L26.
Also see this error package which merges stack traces every time they are being wrapped:
https://github.com/srvc/fail/blob/master/annotators.go#L50
In my PR for improved error codes I am now searching for the deepest stack trace:
https://github.com/pingcap/pd/pull/1200/files#diff-f10e5c1ee6d26ee1746a0a9188a67d4dR32
That assumes use of pkg/errors, but there is also now srvc/fail and go-stack/stack, all with different definitions of what a stack trace are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks~ @gregwebs , it makes sense, I try to implement it again, but use bottom-up way mark error already has stack info~ 3712fc1eb4b24c5a439788c3b5a4ee22375f5193
/run-all-tests |
errors/stack.go
Outdated
@@ -0,0 +1,151 @@ | |||
package errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a license here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done~add license file.
@gregwebs Sorry for such long delay. My concern is just the change of |
errors/errors.go
Outdated
if err == nil { | ||
return nil | ||
} | ||
errWithStack, hasStack := err.(withStackAware) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can extract a function like
func hasStack(err error) bool
For traceability (this would prevent accidentally missing the license our copyright), and testability, I think we need to fork the package on github and add it with The current stack de-duping doesn't search the entire chain. I created a fork of pkg/errors with an |
Thanks, I created a PR here: pingcap/errors#1 |
Gopkg.toml
Outdated
@@ -92,3 +92,7 @@ required = ["github.com/golang/protobuf/jsonpb"] | |||
[[constraint]] | |||
branch = "master" | |||
name = "github.com/etcd-io/gofail" | |||
|
|||
[[constraint]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to move the vendor change to another pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recommand review by commits, 1st comment add vendor, 2nd replace import and remove vendor, 3rd fix some testcase.
run-all-tests tidb-test=pr/617 tidb-private-test=pr/10 |
We are maintaining backwards compatibility with pkg/errors. So it could be better to import this as pkg/errors rather than pingcap/errors. Any other libs that use pkg/errors will end up using our version of errors. |
@gregwebs do you mean we use dep's
I try it, but meet this
so we need make pd use pingcap version first? 🤣 |
Looks like pd needs to not have a pegged version of pkg/errors. I will send a PR there to use pingcap/errors and not peg the version. We probably also need to add a release tag to pingcap/errors, I will create an issue for that. Please take a look at my small PRs to pingcap/errors: https://github.com/pingcap/errors/pulls |
/run-all-tests tidb-test=pr/617 |
/run-all-tests tidb-test=pr/617 |
/run-all-tests tidb-test=pr/617 tidb-private-test=pr/10 |
} | ||
|
||
// FastGen generates a new *Error with the same class and code, and a new formatted message. | ||
// This will not call runtime.Caller to get file and line. | ||
func (e *Error) FastGen(format string, args ...interface{}) *Error { | ||
func (e *Error) FastGen(format string, args ...interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- return
*Error
? - why not warping a
errors.AddStack()
function call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FastGen
seems be used in some situation doesn't need any stack from comment and old code logic...maybe I need more check for them usage..maybe there are some question....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I check FastGen's usage , it just use in duplicate key error situation which are no need stack info. @zz-jason
@@ -136,7 +136,7 @@ func (t backoffType) String() string { | |||
return "" | |||
} | |||
|
|||
func (t backoffType) TError() *terror.Error { | |||
func (t backoffType) TError() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*terror.Error
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woh, terror
is one of two logic change in 3rd commit
in previous terror.Error
maintain stack frame by itself via file
, line
07a6f29#diff-33cf41394e16bb72bf075a6278c80d52L225, so GenXXX
can return *terror.Error
directly.
but after this patch, GenXXX
will return a wrap of terror.Error
(pkg's WithStack), so we need return a error interface, and in test, we use errors.Cause
to check.
@@ -621,7 +621,8 @@ func (c *twoPhaseCommitter) execute(ctx context.Context) error { | |||
if err != nil { | |||
if undeterminedErr := c.getUndeterminedErr(); undeterminedErr != nil { | |||
log.Warnf("con:%d 2PC commit result undetermined, err: %v, rpcErr: %v, tid: %v", c.connID, err, undeterminedErr, c.startTS) | |||
err = errors.Wrap(err, terror.ErrResultUndetermined) | |||
log.Error(err) | |||
err = errors.Trace(terror.ErrResultUndetermined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove the errors.Trace()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for this point, maybe better keep Trace
beacause ErrResultUndetermined is reusable global variable without stack.
executor/write.go
Outdated
func resetErrDataTooLong(colName string, rowIdx int, err error) error { | ||
newErr := types.ErrDataTooLong.Gen("Data too long for column '%v' at row %v", colName, rowIdx) | ||
return errors.Wrap(err, newErr) | ||
log.Error(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
juju/error
's wrap
method is very stranger, it will change the cause and continue attach old error.
e.g.more classic case is in 07a6f29#diff-88da33c1e3eaab083f98c6e9e260373aL1221, we convert failure by some kind of error, and use juju's wrap to change them to ErrTruncate
, so later we call errors.Cause
will get ErrTruncate
instead of golang time parse error, but juju also can keep old's info in previous
field and later can use preivous
to do some output.
pkg/error doesn't have this method (it has wrap but with very different funcation), so I choose log the old error and throw new one.
maybe need a better approch 🤣
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could try uber-go/multierr.
executor/write.go
Outdated
@@ -180,10 +181,11 @@ func updateRecord(ctx sessionctx.Context, h int64, oldData, newData []types.Datu | |||
|
|||
// resetErrDataTooLong reset ErrDataTooLong error msg. | |||
// types.ErrDataTooLong is produced in types.ProduceStrWithSpecifiedTp, there is no column info in there, | |||
// so we reset the error msg here, and wrap old err with errors.Wrap. | |||
// so we reset the error msg here, and wrap old err with new error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's no longer use errors.Wrap so change the comment
LGTM: All my comments of issues have been properly addressed. |
We have to resolve conflicts in order to merge into master. |
- find . -name "*.go" | xargs sed -i 's/github.com\/juju\/errors/github.com\/pkg\/errors/g' - find . -name "*.go" | xargs sed -i 's/errors.Trace(err).(\*errors.Err).Cause()/errors.Cause(errors.Trace(err))/g' - make fmt - dep ensure
conflict resolved @zhexuany |
executor/executor_test.go
Outdated
@@ -990,12 +990,12 @@ func (s *testSuite) TestUnion(c *C) { | |||
|
|||
_, err := tk.Exec("select 1 from (select a from t limit 1 union all select a from t limit 1) tmp") | |||
c.Assert(err, NotNil) | |||
terr := errors.Trace(err).(*errors.Err).Cause().(*terror.Error) | |||
terr := errors.Cause(errors.Trace(err)).(*terror.Error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be simplified to "errors.Cause(err).(*terror.Error)"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's OK to not change this code, just casually ask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and it's simple to change~
terror/terror.go
Outdated
@@ -218,25 +217,23 @@ func (e *Error) getMsg() string { | |||
} | |||
|
|||
// Gen generates a new *Error with the same class and code, and a new formatted message. | |||
func (e *Error) Gen(format string, args ...interface{}) *Error { | |||
func (e *Error) Gen(format string, args ...interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about change the function name to "GenWithStack" and change "GenByArgs" to "GenWithStackByArgs"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-common-test tidb-test=pr/617 tidb-private-test=pr/10 |
juju/errors
to pkg/errors
🎉 thank you! |
What have you changed? (mandatory)
ref #7125 #2622
This PR contain 3 commit, and recommand review by commit
sed
script to replace import packge andmake fmt
[no need review]revert and replay last step if have any conflict
What is the type of the changes? (mandatory)
How has this PR been tested? (mandatory)
Does this PR affect documentation (docs/docs-cn) update? (mandatory)
no
Does this PR affect tidb-ansible update? (mandatory)
no
Does this PR need to be added to the release notes? (mandatory)
no
Refer to a related PR or issue link (optional)
#7125 #2622
https://github.com/pingcap/tidb-test/pull/617
https://github.com/pingcap/tidb-private-test/pull/10
This change is