add logics to support suborchestration--add one intergration test - #20
kaibocai (kaibocai) wants to merge 2 commits into
Conversation
- Renamed ErrorDetails to FailureDetails - Made FailureDetails a property of OrchestrationMetadata - Removed JSON serialization of errors - Split error testing into new JUnit class - Misc. cleanup
| return this.callActivity(name, input, Void.class); | ||
| } | ||
|
|
||
| <V> Task<V> callSubOrchestrator(String name, Object input, String instanceId, Class<V> voidClass); |
There was a problem hiding this comment.
compared with dotnet, I see there is one more param called Version, Chris Gillum (@cgillum) please let me know if this is necessary. Thank you.
There was a problem hiding this comment.
Let's not worry about this for now. I haven't figured out the right way to incorporate version information. Durable Functions for .NET also doesn't support the version parameter.
Chris Gillum (cgillum)
left a comment
There was a problem hiding this comment.
Awesome progress! A few copy/paste typos that need to be cleaned up. I have a few suggestions as well.
Also, you can go ahead and rebase from main now that my other PR is merged (thanks for reviewing it).
| return this.callActivity(name, input, Void.class); | ||
| } | ||
|
|
||
| <V> Task<V> callSubOrchestrator(String name, Object input, String instanceId, Class<V> voidClass); |
There was a problem hiding this comment.
Let's not worry about this for now. I haven't figured out the right way to incorporate version information. Durable Functions for .NET also doesn't support the version parameter.
| return this.callSubOrchestrator(name, input, null); | ||
| } | ||
|
|
||
| default <V>Task<V> callSubOrchestrator(String name, Object input, Class<V> voidClass){ |
There was a problem hiding this comment.
For consistency with activity methods:
| default <V>Task<V> callSubOrchestrator(String name, Object input, Class<V> voidClass){ | |
| default <V>Task<V> callSubOrchestrator(String name, Object input, Class<V> returnType){ |
| return this.callActivity(name, input, Void.class); | ||
| } | ||
|
|
||
| <V> Task<V> callSubOrchestrator(String name, Object input, String instanceId, Class<V> voidClass); |
There was a problem hiding this comment.
Developers can use any class type they want here - not just Void.
| <V> Task<V> callSubOrchestrator(String name, Object input, String instanceId, Class<V> voidClass); | |
| <V> Task<V> callSubOrchestrator(String name, Object input, String instanceId, Class<V> returnType); |
| } | ||
|
|
||
| if (instanceId == null) { | ||
| instanceId = UUID.randomUUID().toString(); |
There was a problem hiding this comment.
I just realized that it can be problematic for us to use random APIs in this code. We should really be using a deterministic UUID generating function. This is another feature that we need to add to the TaskOrchestrationContext class implementation. However, let's not worry about this for now. Can you add a // TODO saying "replace this with a deterministic GUID generation so that it's safe for replay". Maybe we should open an issue on GitHub to track as well so that we don't forget. I think I also need to make this change in .NET.
There was a problem hiding this comment.
Here's a bug I created for .NET: microsoft/durabletask-dotnet#9
| OrchestratorAction taskAction = this.pendingActions.remove(taskId); | ||
| if (taskAction == null) { | ||
| String message = String.format( | ||
| "Non-deterministic orchestrator detected: a history event scheduling an activity task with sequence ID %d and name '%s' was replayed but the current orchestrator implementation didn't actually schedule this task. Was a change made to the orchestrator code after this instance had already started running?", |
There was a problem hiding this comment.
| "Non-deterministic orchestrator detected: a history event scheduling an activity task with sequence ID %d and name '%s' was replayed but the current orchestrator implementation didn't actually schedule this task. Was a change made to the orchestrator code after this instance had already started running?", | |
| "Non-deterministic orchestrator detected: a history event scheduling a sub-orchestration task with sequence ID %d and name '%s' was replayed but the current orchestrator implementation didn't actually schedule this task. Was a change made to the orchestrator code after this instance had already started running?", |
| int taskId = subOrchestrationInstanceCompletedEvent.getTaskScheduledId(); | ||
| TaskRecord<?> record = this.openTasks.remove(taskId); | ||
| if (record == null) { | ||
| this.logger.warning("Discarding a potentially duplicate TaskCompleted event with ID = " + taskId); |
There was a problem hiding this comment.
| this.logger.warning("Discarding a potentially duplicate TaskCompleted event with ID = " + taskId); | |
| this.logger.warning("Discarding a potentially duplicate SubOrchestrationInstanceCompleted event with ID = " + taskId); |
| // TODO: Structured logging | ||
| // TODO: Would it make more sense to put this log in the activity executor? | ||
| this.logger.fine(() -> String.format( | ||
| "%s: Activity '%s' (#%d) completed with serialized output: %s", |
There was a problem hiding this comment.
| "%s: Activity '%s' (#%d) completed with serialized output: %s", | |
| "%s: Sub-orchestrator '%s' (#%d) completed with serialized output: %s", |
|
|
||
| @ParameterizedTest | ||
| @ValueSource(booleans = {true, false}) | ||
| void activityException(boolean handleException) { |
There was a problem hiding this comment.
I suggest we write an integration test like this one but for sub-orchestrations instead of activities.
|
create a new pr here: #21 |
This PR contains changes for issue #5 :
this pr based on branch cgillum/error-handling, will wait for pr #19 to merge first, then merge to main branch. Now target to branch cgillum/error-handling for reviewing