Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the exception type for using retriable task in allOf method #171

Closed
wants to merge 7 commits into from

Conversation

kaibocai
Copy link
Member

@kaibocai kaibocai commented Oct 16, 2023

Issue describing the changes in this PR

Resolve #169

This PR fixes the exception type for using RetriableTask in allOf method.

Description of the issue:

For RetriableTask in allOf method if the task failed for exception, CompositeTaskFailedException is not thrown out instead it's a TaskFailedException being thrown out. The reason is that the CompositeTaskFailedException that built in exceptionPath haven't get chance to be obtained at future.get(), ContextImplTask.this.processNextEvent() will first threw a TaskFailedException

For example:

 @FunctionName("Parallel")
    public List<String> parallelOrchestratorSad(
            @DurableOrchestrationTrigger(name = "ctx") TaskOrchestrationContext ctx,
            ExecutionContext context) {
        try {
            List<Task<String>> tasks = new ArrayList<>();
            RetryPolicy policy = new RetryPolicy(2, Duration.ofSeconds(15));
            TaskOptions options = new TaskOptions(policy);
            tasks.add(ctx.callActivity("AppendSad", "Input1", options, String.class));
            tasks.add(ctx.callActivity("AppendHappy", "Input2", options, String.class));
            return ctx.allOf(tasks).await();
        } catch (CompositeTaskFailedException e) {
            // Nothing will be caught for this type of exception. 
            for (Exception exception : e.getExceptions()) {
                if (exception instanceof TaskFailedException) {
                    TaskFailedException taskFailedException = (TaskFailedException) exception;
                    System.out.println("Task: " + taskFailedException.getTaskName() + " Failed for cause: " + taskFailedException.getErrorDetails().getErrorMessage());
                }
            }
        }
        return null;
    }
    
    @FunctionName("AppendHappy")
    public String appendHappy(
            @DurableActivityTrigger(name = "name") String name,
            final ExecutionContext context) {
        context.getLogger().info("AppendHappy: " + name);
        return name + "-test-happy";
    }
    
    @FunctionName("AppendSad")
    public String appendSad(
            @DurableActivityTrigger(name = "name") String name,
            final ExecutionContext context) {
        context.getLogger().info("Throw Test Exception: " + name);
        throw new NullPointerException("Test kaibocai exception");
    }

This PR ensures that any RetriableTask that is in allOf method is marked as isInCompoundTask so that later it uses this flag to decide whether to throw the exception or ignore it.

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes are added to the CHANGELOG.md
  • I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

@kaibocai kaibocai changed the title Fix the exception type for using retriable task in allOf method Fix the exception type for using retriable task in allOf method Oct 16, 2023
@kaibocai kaibocai marked this pull request as ready for review October 17, 2023 02:57
@kaibocai kaibocai requested a review from davidmrdavid October 18, 2023 18:40
Copy link
Member

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just leaving a question on the implementation strategy.

Comment on lines +1143 to +1163
/**
* Need to have two try-catch block, the first one we ignore the exception from the previous child task
* Once completed the future, method stack frame will pop out till `this.getChildTask().await();`
* in the first try-catch block. The child task now is previous child task we are awaiting on, for any exception
* throw by previous child task we should ignore, we only care about the last child task which is in next
* try-catch block.
*/
try {
// Always return the last child task result.
return this.getChildTask().await();
} catch (Exception exception) {
/**
* If this RetriableTask is not configured as part of an allOf method (CompoundTask),
* it throws an exception, marking the orchestration as failed. However, if this RetriableTask
* is configured within an allOf method (CompoundTask), any exceptions are ignored.
* This approach ensures that when awaiting the future of the allOf method,
* it throws the CompositeTaskFailedException defined in its exceptionPath.
*/
if (!this.isInCompoundTask) throw exception;
}
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm wondering if there might be a cleaner approach to implementing this. Ideally, I think the retriable task class should be unaware that it is inside a composite task. Otherwise, we're tightly coupling the logic.

Is it possible to instead try-catch the processNextEvent method here: https://github.com/microsoft/durabletask-java/blob/e938278aedbfadfd8b653bae9fe7afba30d05c18/client/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java#L1264C26-L1264C65

And ignore the TaskFailure exception that gets thrown during replay?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's doable, let me test that out.

@kaibocai
Copy link
Member Author

refer to #174

@kaibocai kaibocai closed this Oct 19, 2023
@kaibocai kaibocai deleted the kaibocai/fix-allOf-exception branch October 30, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handlings errors from activity functions when fan-out/fan-in pattern is used
2 participants