search

temporalio / sdk-java

Temporal Java SDK
https://temporal.io
Apache License 2.0
219 stars 146 forks source link

Better handling of WorkflowFailedException exception #324

Open mobiletoly opened 3 years ago

mobiletoly commented 3 years ago

Is your feature request related to a problem? Please describe. Let me quickly explain my current situation. I have a system with few microservices, just for a simplicity let's make it two microservices - (1) API Gateway and (2) Listing Service. My Account Service has workflows related to managing Listings (e.g. create a new listing). While API Gateway is a REST service. When I call Account Service from API Gateway - I obviously want to translate errors I receive from Account Service to some meaningful HTTP response code and response payload. It is easy to do with analyzing WorkflowFailedException's getCause() method but here is a gotcha. In workflow I can throw an exception (e.g. non-retriable exception) via ApplicationFailure.newNonRetryableFailure with some error type (e.g. "LISTING_NOT_FOUND") but this call may generate different exception types. E.g. if it is called from Activity - then exception thrown is ActivityFailure while if exception is thrown from workflow then exception will be ApplicationFailure. It forces me to add more boilerplate to my caller code, because now I have to check what type of exception if thrown, is it ActivityFailure or ApplicationFailure. For example if it is ApplicationType - I can check type directly, while if it is ActivityFailure, then I have to type a look even deeper inside ActivityFailure, find it cause, convert it to ApplicationFailure and check type. And it makes caller to be aware of internal logic of workflow, while caller does not really need to know if error happened in Activity or Application, it just needs to know error type, because this logic can change, today I call ApplicationFailure with LISTING_NOT_FOUND error type from Workflow, tomorrow I decide to do some optimization and move this throw into Activity.

Describe the solution you'd like Two options:

  1. WorkflowFailedException need to have some common fields from ApplicationFailure/ActivityFailure, e.g. getType() or
  2. Both ApplicationFailure and ActivityFailure derive from TemporalFailure, so some common fields can be added there.
mobiletoly commented 3 years ago

Actually third option could that if ApplicationFailure exception is thrown from Activity - it should not be converted to ActivityFailure and should remain ApplicationFailure, because that is what we explicitly tell to our code to do when throw exception in Activity via let's say throw ApplicationFailure.newNonRetryableFailure - we want application-level exception. But I don't know what would be the implications of this solution for existing code. Otherwise it is all very confusing (unless we have a really good documentation outlining this kind of issues) - should we even allow something like ApplicationFailure.newNonRetryableFailure from Activity code? should we introduce some sort of ActivityFailure.newNonRetryableFailure instead if we want to signal Activity-level errors and keep ApplicationFailure.newNonRetryableFailure for clearly application-level issues (e.g. business logic) that can be throws from Activity without being treated as Activity error?