Add MultiOperation errors #158
Conversation
stephanos
force-pushed
the
multiops-errors
branch
from
488da9c
to
8ea7498
Compare
stephanos
force-pushed
the
multiops-errors
branch
3 times, most recently
from
adde912
to
74f3253
Compare
| @@ -71,7 +72,21 @@ func FromStatus(st *status.Status) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // Simple case. `st.Code()` to `serviceerror` is one to one mapping and there are no error details. | |||
| errDetails := extractErrorDetails(st) | |||
There was a problem hiding this comment.
The error parsing had to be changed so that it always parses the error details first. This adds an extra step for status codes such as Unavailable and Canceled, but is most likely negligible.
stephanos
force-pushed
the
multiops-errors
branch
2 times, most recently
from
ad53860
to
4df4a52
Compare
stephanos
force-pushed
the
multiops-errors
branch
from
9e919b6
to
6ced852
Compare
serviceerror/convert.go
Outdated
| case *failure.MultiOperationExecutionAborted: | ||
| return newMultiOperationAborted(st) | ||
| default: | ||
| return st.Err() |
There was a problem hiding this comment.
I would do same as on line 190, or add generic AbortedError with empty details.
serviceerror/multiOpAborted.go
Outdated
There was a problem hiding this comment.
multi_op_aborted.go please. And someone need to rename all other files in this package.
serviceerror/multiOpAborted.go
Outdated
| } | ||
|
|
||
| // NewMultiOperationAbortedError returns MultiOperationAborted. | ||
| func NewMultiOperationAbortedError(message string) error { |
There was a problem hiding this comment.
You need to be consistent in using Error suffix. I agree on not using Failure suffix for proto message and I would prefer to use Error suffix for error because it is essentially a error in go world. Or remove Error() method.
Makefile
Outdated
| @@ -41,7 +41,7 @@ $(PROTO_OUT): | |||
| ##### git submodule for proto files ##### | |||
| update-proto-submodule: | |||
| printf $(COLOR) "Update proto-submodule..." | |||
| git -c protocol.file.allow=always submodule update --init --force --remote $(PROTO_ROOT) | |||
| #git -c protocol.file.allow=always submodule update --init --force --remote $(PROTO_ROOT) | |||
There was a problem hiding this comment.
Don't forget to uncomment this.
stephanos
force-pushed
the
multiops-errors
branch
from
63c31bf
to
f696245
Compare
stephanos
force-pushed
the
multiops-errors
branch
from
f696245
to
e030e93
Compare
There was a problem hiding this comment.
I was intentionally waiting on API repo merge before giving approval here, but it looks like it was merged w/ no SDK approval because our branch protections and codeowners don't require it. I wonder if we should change this repo to be like the api repo w/ the restrictions there.
|
@cretz I agree, it's probably a good idea to require SDK approval here. I looked over the recent PRs and most - but not all - had SDK approval (but most didn't have Server approval). |
What changed?
Added two new error types, both for MultiOperationExecution.
The errors behavior was documented in temporalio/api#367.
Why?
To return these errors from the Server (see PR).
How did you test it?
Add new tests and Server implementation PR.
Potential risks
None