Wrap underlying cause for conditional update error #4797
Conversation
service/history/execution/context.go
Outdated
| } | ||
|
|
||
| func (e *conflictError) Error() string { | ||
| return fmt.Sprintf("conditional update failed: %s", e.Cause.Error()) |
There was a problem hiding this comment.
| return fmt.Sprintf("conditional update failed: %s", e.Cause.Error()) | |
| return fmt.Sprintf("conditional update failed: %v", e.Cause) |
Since I haven't actually looked into it in detail before: this should probably be ... %v", e.Cause.
Because:
fmt.Errorf does this: https://github.com/golang/go/blob/master/src/fmt/errors.go#L17-L30
and, with the Error() impl which just returns that message, is basically "eagerly create the full message".
TIL.
That wrapErrs flag later reaches here in the fmt printer: https://github.com/golang/go/blob/master/src/fmt/print.go#L574-L587
which just swaps it with %v for stringifying, and holds onto the error value (which is ignored by Errorf).
TIL 2.
That's... simple and fairly backwards-compatible, but yow. Worse than I expected. Potentially unnecessarily costly and destroys a lot of information for no good reason (doesn't check for or support %+v or %#v because it can't).
service/history/execution/context.go
Outdated
| // NewConflictError is only public because it used in workflow/util_test.go | ||
| // TODO: refactor those tests | ||
| func NewConflictError(cause error) error { | ||
| return &conflictError{cause} | ||
| } |
There was a problem hiding this comment.
personal preference: require a *testing.T argument for any test-only code. even if it's unused, it makes it rather unambiguous that it's not intended for any other kind of use.
There was a problem hiding this comment.
That is actually a neat idea 👍 . I'm going to do that from now on.
Pull Request Test Coverage Report for Build edaf25ea-7de7-4406-b49d-7e057268957c
💛 - Coveralls |
What changed?
Instead of using value error
execution.ErrConflictlets use error typeexecution.conflictErrorwhich wraps underlying cause.Why?
Currently it can be very hard to debug certain failures when only thing that is visible in logs or returned via some endpoints is
code:internal message:conditional update failedNow, underlying cause will be included in error message as well.
How did you test it?
Potential risks
Release notes
Documentation Changes