Major error handling refactoring #101
Conversation
| this.nonRetryable = nonRetryable; | ||
| } | ||
|
|
||
| public boolean isNonRetryable() { |
There was a problem hiding this comment.
Is there pros to this naming vs isRetryable? Seems off having a negative on a bool property
There was a problem hiding this comment.
The pattern I always use is to have default value as the default value of the type. For boolean the default is false.
| @@ -226,7 +226,7 @@ public void workflowCacheEvictionDueToThreads() { | |||
| int count = 100; | |||
| ActivitiesWorkflow[] workflows = new ActivitiesWorkflow[count]; | |||
| WorkflowParams w = new WorkflowParams(); | |||
| w.TemporalSleep = Duration.ofSeconds(1); | |||
| w.TemporalSleepMillis = 1000; | |||
There was a problem hiding this comment.
For defining time intervals - Do we prefer units on ints rather than strongly typed Duration?
There was a problem hiding this comment.
We prefer Duration unless it has to be passed across process boundary and serialized/deserialized. We can add serialization of Duration to the Jackson json serializer, but then it might not play well with Go and other SDKs.
| public Optional<Payloads> getErrJson() { | ||
| return Strings.isNullOrEmpty(headers.errReason) ? Optional.empty() : result; | ||
| public Optional<Failure> getFailure() { | ||
| return failure; | ||
| } | ||
|
|
||
| public Optional<Payloads> getResult() { | ||
| return result; | ||
| } | ||
|
|
||
| public long getReplayTimeMillis() { |
There was a problem hiding this comment.
Mixed time types? ReplayTimeMillis vs Backoff
There was a problem hiding this comment.
ReplayTimeMillis is a specific time of replay (timestamp). Backoff is a duration which is time interval.
| // Use exception type name as the reason | ||
| List<Class<? extends Throwable>> doNotRetry = retryOptions.getDoNotRetry(); | ||
| String[] doNotRetry = retryOptions.getDoNotRetry(); |
There was a problem hiding this comment.
What is the reason for using string rather than types? Does previous usage cause the Illegal reflective access warnings?
There was a problem hiding this comment.
This is to be compatible with other SDKs. An activity can be implemented in Go and return an error type which is not Java exception name.
Refactoring to the new Failure proto API.
This is to support propagation of chained errors across multiple SDKs. The biggest incompatible change from the application developer point of view that type of failure is returned as a cause of the ActivityFailure or WorkflowFailure instead of using child exceptions.