move from `juju/errors` to `pkg/errors` #7151

lysu · 2018-07-25T07:09:14Z

What have you changed? (mandatory)

This PR contain 3 commit, and recommand review by commit

first commit add pingcap/errors vendor that modified with make tidb adaptor in errors. errors#2
second commit fix some terror testcase
last commit use sed script to replace import packge and make fmt[no need review]

find . -name "*.go" | xargs sed -i 's/github.com\/juju\/errors/github.com\/pingcap\/tidb\/errors/g'
find . -name "*.go" | xargs sed -i 's/errors.Trace(err).(\*errors.Err).Cause()/errors.Cause(errors.Trace(err))/g'

revert and replay last step if have any conflict

What is the type of the changes? (mandatory)

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

unit tests
integration tests, need work with https://github.com/pingcap/tidb-test/pull/617 https://github.com/pingcap/tidb-private-test/pull/10

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

gregwebs · 2018-08-18T06:37:07Z

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

zhexuany

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

gregwebs · 2018-08-20T12:59:04Z

@zhexuany can you click to load the diff and comment on specifically what you find strange?
It seems like juju/errors should be removed from it now.

disksing · 2018-08-27T03:47:28Z

cmd/benchfilesort/main.go

@@ -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)


Why not simply return b, err?

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~?

Reasonable and fair. Good luck.

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.

thanks~ @gregwebs , it makes sense, I try to implement it again, but use bottom-up way mark error already has stack info~ 3712fc1eb4b24c5a439788c3b5a4ee22375f5193

lysu · 2018-09-04T10:54:01Z

/run-all-tests

lysu · 2018-09-04T11:06:46Z

@coocood @zz-jason PTAL, it seem already to ship, also take care about new PR description.

jackysp · 2018-09-04T11:30:14Z

errors/stack.go

@@ -0,0 +1,151 @@
+package errors


Need a license here.

Done~add license file.

zhexuany · 2018-09-04T13:25:12Z

@gregwebs Sorry for such long delay. My concern is just the change of Gopkg.lock is caused by running dep command or not. If no, that is where my concern can play a role of.

coocood · 2018-09-04T14:17:11Z

errors/errors.go

+	if err == nil {
+		return nil
+	}
+	errWithStack, hasStack := err.(withStackAware)


We can extract a function like

func hasStack(err error) bool

gregwebs · 2018-09-04T23:47:01Z

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 dep.

The current stack de-duping doesn't search the entire chain. I created a fork of pkg/errors with an AddStack function that does do that search:
https://github.com/pkg/errors/compare/master...gregwebs:dedup-stack-traces?expand=1

coocood · 2018-09-05T03:00:37Z

@gregwebs
Forked here https://github.com/pingcap/errors

gregwebs · 2018-09-05T05:20:26Z

Thanks, I created a PR here: pingcap/errors#1

jackysp · 2018-09-07T07:47:15Z

Gopkg.toml

@@ -92,3 +92,7 @@ required = ["github.com/golang/protobuf/jsonpb"]
 [[constraint]]
  branch = "master"
  name = "github.com/etcd-io/gofail"
+
+[[constraint]]


It's better to move the vendor change to another pr.

recommand review by commits, 1st comment add vendor, 2nd replace import and remove vendor, 3rd fix some testcase.

lysu · 2018-09-07T07:53:31Z

@coocood @zz-jason @jackysp Now, we use pingcap/errors and ready to review PTAL if free thx~

lysu · 2018-09-07T09:36:15Z

run-all-tests tidb-test=pr/617 tidb-private-test=pr/10

gregwebs · 2018-09-07T12:50:48Z

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.

lysu · 2018-09-07T14:42:51Z

@gregwebs do you mean we use dep's source configuration, just like this?

[[constraint]]
  name = "github.com/pkg/errors"
  revision = "f29524922bfee4bf03f9c426c716e639cac63ea9"
  source = "https://github.com/pingcap/errors.git"

I try it, but meet this

dep ensure
Solving failure: No versions of github.com/pingcap/pd met constraints:
	master: Could not introduce github.com/pingcap/pd@master, as it has a dependency on github.com/pkg/errors with constraint ^0.8.0, which has no overlap with existing constraint f29524922bfee4bf03f9c426c716e639cac63ea9 from (root)
	v2.0.5: Could not introduce github.com/pingcap/pd@v2.0.5, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.0.4: Could not introduce github.com/pingcap/pd@v2.0.4, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.0.3: Could not introduce github.com/pingcap/pd@v2.0.3, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.0.2: Could not introduce github.com/pingcap/pd@v2.0.2, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.0.1: Could not introduce github.com/pingcap/pd@v2.0.1, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.0.0: Could not introduce github.com/pingcap/pd@v2.0.0, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v1.0.8: Could not introduce github.com/pingcap/pd@v1.0.8, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v1.0.7: Could not introduce github.com/pingcap/pd@v1.0.7, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v1.0.6: Could not introduce github.com/pingcap/pd@v1.0.6, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v1.0.5: Could not introduce github.com/pingcap/pd@v1.0.5, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v1.0.3: Could not introduce github.com/pingcap/pd@v1.0.3, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v1.0.2: Could not introduce github.com/pingcap/pd@v1.0.2, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v1.0.0: Could not introduce github.com/pingcap/pd@v1.0.0, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.1.0-rc.1: Could not introduce github.com/pingcap/pd@v2.1.0-rc.1, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.1.0-beta: Could not introduce github.com/pingcap/pd@v2.1.0-beta, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.1.0-alpha: Could not introduce github.com/pingcap/pd@v2.1.0-alpha, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.0.0-rc.6: Could not introduce github.com/pingcap/pd@v2.0.0-rc.6, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.0.0-rc.5: Could not introduce github.com/pingcap/pd@v2.0.0-rc.5, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.0.0-rc.4: Could not introduce github.com/pingcap/pd@v2.0.0-rc.4, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.0.0-rc.3: Could not introduce github.com/pingcap/pd@v2.0.0-rc.3, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v2.0.0-rc.1: Could not introduce github.com/pingcap/pd@v2.0.0-rc.1, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v1.1.0-beta: Could not introduce github.com/pingcap/pd@v1.1.0-beta, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v1.1.0-alpha.1: Could not introduce github.com/pingcap/pd@v1.1.0-alpha.1, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	v1.1.0-alpha: Could not introduce github.com/pingcap/pd@v1.1.0-alpha, as it is not allowed by constraint master from project github.com/pingcap/tidb.
	master: Could not introduce github.com/pingcap/pd@master, as it has a dependency on github.com/pkg/errors with constraint ^0.8.0, which has no overlap with existing constraint f29524922bfee4bf03f9c426c716e639cac63ea9 from (root)

so we need make pd use pingcap version first? 🤣

gregwebs · 2018-09-07T15:02:22Z

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

lysu · 2018-09-10T09:37:51Z

/run-all-tests tidb-test=pr/617

coocood · 2018-09-10T11:08:09Z

/run-all-tests tidb-test=pr/617

lysu · 2018-09-10T15:32:59Z

/run-all-tests tidb-test=pr/617 tidb-private-test=pr/10

Gopkg.toml

needs one more reviewer.

zz-jason · 2018-09-10T15:46:08Z

terror/terror.go

 }

 // 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 {


return *Error?

why not warping a errors.AddStack() function call?

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....

I check FastGen's usage , it just use in duplicate key error situation which are no need stack info. @zz-jason

zz-jason · 2018-09-10T15:47:59Z

store/tikv/backoff.go

@@ -136,7 +136,7 @@ func (t backoffType) String() string {
 	return ""
 }

-func (t backoffType) TError() *terror.Error {
+func (t backoffType) TError() error {


*terror.Error ?

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.

zz-jason · 2018-09-10T15:49:02Z

store/tikv/2pc.go

@@ -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)


can we remove the errors.Trace()?

for this point, maybe better keep Trace beacause ErrResultUndetermined is reusable global variable without stack.

zz-jason · 2018-09-10T15:51:13Z

executor/write.go

 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)


remove the log?

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 🤣

You could try uber-go/multierr.

zz-jason · 2018-09-10T15:51:20Z

executor/write.go

@@ -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.


update the comment?

yes, it's no longer use errors.Wrap so change the comment

gregwebs · 2018-09-11T20:08:11Z

LGTM: All my comments of issues have been properly addressed.

zhexuany · 2018-09-12T01:51:47Z

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

lysu · 2018-09-12T05:24:25Z

conflict resolved @zhexuany

zz-jason · 2018-09-12T05:53:56Z

executor/executor_test.go

@@ -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)


can be simplified to "errors.Cause(err).(*terror.Error)"?

It's OK to not change this code, just casually ask.

yes and it's simple to change~

zz-jason · 2018-09-12T05:56:33Z

terror/terror.go

@@ -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 {


how about change the function name to "GenWithStack" and change "GenByArgs" to "GenWithStackByArgs"?

zz-jason

LGTM

lysu · 2018-09-12T07:23:34Z

/run-common-test tidb-test=pr/617 tidb-private-test=pr/10
/run-integration-common-test tidb-test=pr/617 tidb-private-test=pr/10

gregwebs · 2018-09-12T08:29:17Z

🎉 thank you!

lysu added the status/DNM label Jul 25, 2018

jackysp added the status/all tests passed label Jul 25, 2018

gregwebs mentioned this pull request Aug 18, 2018

integrate error codes with pkg/errors to provide error tracing tikv/pd#1200

Merged

zhexuany reviewed Aug 20, 2018

View reviewed changes

gregwebs mentioned this pull request Aug 23, 2018

switch from juju/errors to pkg/errors tikv/pd#1210

Merged

disksing reviewed Aug 27, 2018

View reviewed changes

lysu added component/tools and removed status/DNM labels Sep 4, 2018

jackysp reviewed Sep 4, 2018

View reviewed changes

lysu added status/all tests passed and removed status/all tests passed labels Sep 4, 2018

coocood reviewed Sep 4, 2018

View reviewed changes

lysu mentioned this pull request Sep 5, 2018

make tidb adaptor in errors. pingcap/errors#2

Merged

jackysp reviewed Sep 7, 2018

View reviewed changes

gregwebs mentioned this pull request Sep 7, 2018

Add a new RequeueError type pingcap/tidb-operator#80

Merged

gregwebs mentioned this pull request Sep 7, 2018

pkg/errors: use a lower bound tikv/pd#1238

Merged

gregwebs previously approved these changes Sep 10, 2018

View reviewed changes

zz-jason reviewed Sep 10, 2018

View reviewed changes

Gopkg.toml Show resolved Hide resolved

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 10, 2018

zz-jason reviewed Sep 10, 2018

View reviewed changes

lysu added status/all tests passed and removed status/all tests passed labels Sep 10, 2018

gregwebs mentioned this pull request Sep 11, 2018

add BuildChain, for associating two errors together pingcap/errors#6

Closed

lysu added 4 commits September 12, 2018 13:00

vendor: import pingcap/error as pkg/errors & upgrade pd client

55d150d

vendor/script: replace import, fmt and remove juju

a6ef99e

- 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

*: fix after script replace

091a280

Merge branch 'master' into dev/move_to_pkg_error

890e2b0

zz-jason reviewed Sep 12, 2018

View reviewed changes

lysu added 2 commits September 12, 2018 14:07

*: remove duplicate Trace() call nested in Cause()

e3f5615

*: rename Gen* to GenWithStack*

0fa1be2

zz-jason approved these changes Sep 12, 2018

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 12, 2018

lysu merged commit cb03f2b into pingcap:master Sep 12, 2018

lysu deleted the dev/move_to_pkg_error branch September 12, 2018 07:43

lysu changed the title ~~move from juju/errors to pkg/errors~~ move from juju/errors to pkg/errors Sep 12, 2018

move from juju/errors to pkg/errors #7151

move from juju/errors to pkg/errors #7151

Conversation

lysu commented Jul 25, 2018 • edited Loading

What have you changed? (mandatory)

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)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

gregwebs commented Aug 18, 2018

zhexuany left a comment

Choose a reason for hiding this comment

gregwebs commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Sep 4, 2018

lysu commented Sep 4, 2018 • edited Loading

jackysp Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhexuany commented Sep 4, 2018

Choose a reason for hiding this comment

gregwebs commented Sep 4, 2018

coocood commented Sep 5, 2018

gregwebs commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Sep 7, 2018

lysu commented Sep 7, 2018

gregwebs commented Sep 7, 2018

lysu commented Sep 7, 2018

gregwebs commented Sep 7, 2018

lysu commented Sep 10, 2018

coocood commented Sep 10, 2018

lysu commented Sep 10, 2018

Choose a reason for hiding this comment

lysu Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregwebs commented Sep 11, 2018

zhexuany commented Sep 12, 2018

lysu commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

lysu commented Sep 12, 2018

gregwebs commented Sep 12, 2018

move from `juju/errors` to `pkg/errors` #7151

move from `juju/errors` to `pkg/errors` #7151

lysu commented Jul 25, 2018 •

edited

Loading

lysu commented Sep 4, 2018 •

edited

Loading

jackysp Sep 4, 2018 •

edited

Loading

lysu Sep 10, 2018 •

edited

Loading