Skip to content
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

add BuildChain, for associating two errors together #6

Closed
wants to merge 3 commits into from

Conversation

gregwebs
Copy link

The nee for this came up in pingcap/tidb#7151 (comment)

@coocood
Copy link
Member

coocood commented Sep 11, 2018

@lysu PTAL

errors.go Outdated
}

func (err *chain) Cause() error {
return err.prevErr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we recursively find the root cause?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No: this is the causer interface that goes just one level deeper.

errors.go Outdated
}

func (err *chain) Error() string {
return err.currentErr.Error() + "\n" + err.prevErr.Error()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error should not got preErr.

e.g. TruncateErr, error info should only contain a column, b row value be truncate for end-user

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that makes sense since this is joining two separate errors. The previous will show up in the formatter.

@@ -335,3 +335,29 @@ func Find(origErr error, test func(error) bool) error {
})
return foundErr
}

// Chain ties together two errors, giving them a Causer interface and a concatenated Error message.
type chain struct {
Copy link
Collaborator

@lysu lysu Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need override ErrorStack or %+v to print errors info before wrap recursively.

e.g.

func main() {
	_, err := time.ParseDuration("dfasdf")
	err = resetErrDataTooLong("c", 1,  err)
	err = resetErrDataTooLong("b", 2, err)
	fmt.Println(errors.ErrorStack(err))
}

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

should output:

time: invalid duration dfasdf
/home/robi/Code/go/src/github.com/pingcap/tidb/executor/test/zz.go:24: [types:1406]Data too long for column 'c' at row 1
/home/robi/Code/go/src/github.com/pingcap/tidb/executor/test/zz.go:24: [types:1406]Data too long for column 'b' at row 2

I think time: invalid duration dfasdf is what we need for this method

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, the formatter is added.

@lysu
Copy link
Collaborator

lysu commented Sep 11, 2018

IMHO, I still think do log-then-rethrow is simple solution.

}

func (err *chain) Cause() error {
return err.prevErr
Copy link
Collaborator

@lysu lysu Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @gregwebs , I check juju's impl again. It seem will return .error in Cause(), and Error() will go preErr when error == nil...should we follow them ~?

expect this two question, others are look good to me

@gregwebs
Copy link
Author

The implementation for vertical composition of distinct errors is turning out to be complex and fragile. I think it is better to just do annotations vertically. For combining errors, we can use ErrorGroup and combine horizontally. You could use uber-go/multierr.

@gregwebs gregwebs closed this Sep 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants