Skip to content

Fix orchestration stuck using anyOf/allOf with retry policy #153

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.

Sign up for GitHub

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

Closed
kaibocai wants to merge 4 commits into from
Closed

Fix orchestration stuck using anyOf/allOf with retry policy #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
becef0c
fix orchestration stuck using anyOf/allOf with retry policy
kaibocai Jul 12, 2023
cab6d6d
Update CHANGELOG.md
kaibocai Jul 12, 2023
e50dd9a
clean up code
kaibocai Jul 12, 2023
a4b7990
update end2end test
kaibocai Jul 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
* Support restartInstance and pass restartPostUri in HttpManagementPayload ([#108](https://github.com/microsoft/durabletask-java/issues/108))
* Improve `TaskOrchestrationContext#continueAsNew` method so it doesn't require `return` statement right after it anymore ([#149](https://github.com/microsoft/durabletask-java/pull/149))

### Fix
* Fix retry policy not working with anyOf/allOf pattern ([#153](https://github.com/microsoft/durabletask-java/pull/153))

## v1.1.1

### Updates
Expand Down
2 changes: 1 addition & 1 deletion client/src/main/java/com/microsoft/durabletask/Task.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
* @param <V> the return type of the task
*/
public abstract class Task<V> {
final CompletableFuture<V> future;
CompletableFuture<V> future;

Task(CompletableFuture<V> future) {
this.future = future;
Expand Down
26 changes: 22 additions & 4 deletions client/src/main/java/com/microsoft/durabletask/TaskOrchestrationExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,7 @@ private class RetriableTask<V> extends CompletableTask<V> {
private final TaskOrchestrationContext context;
private final Instant firstAttempt;
private final TaskFactory<V> taskFactory;
private Task<V> currentTask;

private int attemptNumber;
private FailureDetails lastFailure;
Expand All @@ -988,25 +989,40 @@ private RetriableTask(
TaskFactory<V> taskFactory,
@Nullable RetryPolicy retryPolicy,
@Nullable RetryHandler retryHandler) {
super(new CompletableFuture<>());
this.context = context;
this.taskFactory = taskFactory;
this.policy = retryPolicy;
this.handler = retryHandler;
this.firstAttempt = context.getCurrentInstant();
this.totalRetryTime = Duration.ZERO;
this.taskFactory = taskFactory;
// make sure the taskFactory#create method is called when create RetriableTask,
// so when use with anyOf/allOf the pendingActions is not empty when return to the sidecar.
this.currentTask = taskFactory.create();
// to make sure the RetriableTask future is same as currentTask,
// so they complete at the same time
setFuture(this.currentTask.future);
Copy link
Member

@cgillum cgillum Jul 12, 2023

Choose a reason for hiding this comment

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

code style consistency nit:

Suggested change
setFuture(this.currentTask.future);
this.setFuture(this.currentTask.future);

}

private void setFuture(CompletableFuture<V> future) {
this.future = future;
}

@Override
public V await() {
Instant startTime = this.context.getCurrentInstant();
while (true) {
Task<V> currentTask = this.taskFactory.create();
// For first try, currentTask is not null, this will be skipped.
if (this.currentTask == null) {
Copy link
Member

@cgillum cgillum Jul 12, 2023

Choose a reason for hiding this comment

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

Maintainability nit: Can you add a comment here explaining when this.currentTask == null will be true?

this.currentTask = this.taskFactory.create();
// to make sure the RetriableTask future is same as currentTask,
// so they complete at the same time
setFuture(this.currentTask.future);
}

this.attemptNumber++;

try {
return currentTask.await();
return this.currentTask.await();
} catch (TaskFailedException ex) {
this.lastFailure = ex.getErrorDetails();
if (!this.shouldRetry()) {
Expand All @@ -1026,6 +1042,8 @@ public V await() {
}

this.totalRetryTime = Duration.between(startTime, this.context.getCurrentInstant());
//empty currentTask.
this.currentTask = null;
Copy link
Member

@cgillum cgillum Jul 12, 2023

Choose a reason for hiding this comment

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

Rather than setting this.currentTask = null and then relying on the null-check at the top of the loop, wouldn't it be simpler to just reset the current task here and remove the null-check?

// Generate a new task/future pair for the next attempt
this.currentTask = this.taskFactory.create();
setFuture(this.currentTask.future);

}
}

Expand Down
48 changes: 48 additions & 0 deletions samples-azure-functions/src/main/java/com/functions/ParallelFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package com.functions;

import com.microsoft.azure.functions.annotation.*;
import com.microsoft.azure.functions.*;

import java.time.Duration;
import java.util.*;

import com.microsoft.durabletask.*;
import com.microsoft.durabletask.azurefunctions.DurableActivityTrigger;
import com.microsoft.durabletask.azurefunctions.DurableClientContext;
import com.microsoft.durabletask.azurefunctions.DurableClientInput;
import com.microsoft.durabletask.azurefunctions.DurableOrchestrationTrigger;

public class ParallelFunctions {
@FunctionName("StartParallelOrchestration")
public HttpResponseMessage startParallelOrchestration(
@HttpTrigger(name = "req", methods = {HttpMethod.GET, HttpMethod.POST}, authLevel = AuthorizationLevel.ANONYMOUS) HttpRequestMessage<Optional<String>> request,
@DurableClientInput(name = "durableContext") DurableClientContext durableContext,
final ExecutionContext context) {
context.getLogger().info("Java HTTP trigger processed a request.");

DurableTaskClient client = durableContext.getClient();
String instanceId = client.scheduleNewOrchestrationInstance("Parallel");
context.getLogger().info("Created new Java orchestration with instance ID = " + instanceId);
return durableContext.createCheckStatusResponse(request, instanceId);
}

@FunctionName("Parallel")
public List<String> parallelOrchestrator(
@DurableOrchestrationTrigger(name = "ctx") TaskOrchestrationContext ctx) {
RetryPolicy retryPolicy = new RetryPolicy(2, Duration.ofSeconds(5));
TaskOptions taskOptions = new TaskOptions(retryPolicy);
Copy link
Member

@cgillum cgillum Jul 12, 2023

Choose a reason for hiding this comment

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

This test covers the base case where activities succeed, but shouldn't we also add a test for cases where at least one function call fails and is retried? Otherwise I feel like we're not really validating the correctness of this PR.

Also, would it make sense to have this test coverage in the main client library?

Copy link
Member Author

@kaibocai kaibocai Jul 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also, would it make sense to have this test coverage in the main client library?

You mean adding similar test cases for integration tests that run on durable sidecar right?

List<Task<String>> tasks = new ArrayList<>();
tasks.add(ctx.callActivity("Append", "Input1", taskOptions, String.class));
tasks.add(ctx.callActivity("Append", "Input2", taskOptions, String.class));
tasks.add(ctx.callActivity("Append", "Input3", taskOptions, String.class));
return ctx.allOf(tasks).await();
}

@FunctionName("Append")
public String append(
@DurableActivityTrigger(name = "name") String name,
final ExecutionContext context) {
context.getLogger().info("Append: " + name);
return name + "-test";
}
}
10 changes: 7 additions & 3 deletions samples-azure-functions/src/test/java/com/functions/EndToEndTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,16 @@ public void setupHost() {
post(hostHealthPingPath).then().statusCode(200);
}

@Test
public void basicChain() throws InterruptedException {
@ParameterizedTest
@ValueSource(strings = {
"StartOrchestration",
"StartParallelOrchestration"
})
public void generalFunctions(String functionName) throws InterruptedException {
Set<String> continueStates = new HashSet<>();
continueStates.add("Pending");
continueStates.add("Running");
String startOrchestrationPath = "/api/StartOrchestration";
String startOrchestrationPath = "/api/" + functionName;
Response response = post(startOrchestrationPath);
JsonPath jsonPath = response.jsonPath();
String statusQueryGetUri = jsonPath.get("statusQueryGetUri");
Expand Down