Rework Local Activity scheduling by Spikhalskiy · Pull Request #1507 · temporalio/sdk-java

Spikhalskiy · 2022-11-08T04:02:04Z

What was changed

This PR implements a timer thread that independently of the local activity executors performs lifecycle management of local activities and allows to enforce timeouts.
The interface between LA workers and workflow code / state machines is reworked to allow LA workers to communicate different failure reasons / retry back to the workflow.
startToClose timeout and scheduleToClose timeout supports are implemented.
The ground is prepared for adding scheduleToStart timeout for local activities #1512
The workflow/LA interface is improved to prepare for #1261

Why?

Previously Local Activities scheduling and management code were trivial.
One thread executes the activities, waits with sleep until the next attempt, and executes the next attempt.
While being simple, there is a bunch of Cons to this approach:

Executor thread is retained for the whole time of LA execution from the start until the finish.
This model allows us to enforce neither startToClose nor scheduleToClose timeouts because the only thread that manages an activity invocation is tied to the execution of the activity's code.
The is no way to start a new attempt until the end of the previous attempt.

Closes #1004

Sushisource · 2022-11-23T17:06:42Z

+    String type =
+        e instanceof ApplicationFailure
+            ? ((ApplicationFailure) e).getType()
+            : e.getClass().getName();


Is it possible to set an explicit do not retry field in Java or is it only the string matching?

String matching as it gets converted to protobuf's RetryPolicy, at least at this moment.

Sushisource · 2022-11-23T17:11:43Z

@@ -286,10 +285,10 @@ private void processLocalActivityRequests(long startTimeNs) throws InterruptedEx
        // much sense. I believe we should add ScheduleToStart timeout for the local activities
        // as well.


Todo now addressed?

Nope! #1512 Will be in a separate PR.

Sushisource · 2022-11-23T17:12:06Z

-            localActivityTaskPoller.apply(
-                new LocalActivityTask(laRequest, localActivityCompletionSink),
-                Duration.ofNanos(maxWaitTimeNs));
+        laRequest.setScheduleToStartTimeout(Duration.ofNanos(maxWaitTimeNs));


Can user not set sched-to-start timeout? (Honestly, makes sense)

It's old logic. It will be gone in #1512.
We can discuss, but I don't see any "very good" solutions to not set scheduleToStart. I guess keep workflow tasks open will be expected by users if they have some unexpected backfilling in their systems. An alternative reasonable solution may be to set it to localRetryThreshold. The second approach is safer from the load control, but may be not expected by users.

Sushisource · 2022-11-23T17:14:26Z

-  private final PollActivityTaskQueueResponse.Builder activityTask;
+  public static final long NOT_SCHEDULED = -1;
+
+  // It doesn't have all the fields published yet.


What doesn't? This builder? Change to "This builder won't..."

Sushisource · 2022-11-23T17:23:04Z

+  private static final RetryState LOCAL_RETRY_LIMIT_RETRY_STATE =
+      RetryState.RETRY_STATE_IN_PROGRESS;


Why re-bind this here?

It used in couple of places and I wanted to write a long comment on this value choice. And I don't want the comment to be duplicated.

Sushisource · 2022-11-23T17:36:41Z

+        // Making sure that the result handling code following this statement is mutual exclusive
+        // with the start to close timeout handler.
+        boolean startToCloseTimeoutFired =
+            startToCloseTimeoutFuture != null && !startToCloseTimeoutFuture.cancel(false);
+


Since this is coming after handle, does this mean the start-to-close timeout cannot "interrupt" the running activity if it fires - but does it tell the workflow about the timeout right away?

So here we're just saying "OK we (elsewhere) told the WF the LA timed out, and now we're just discarding the result when the LA eventually finishes"?

Do we try to notify the LA of cancel / interrupt it at all? I'm guessing not since there's not a good mechanism for it

Correct. RIGHT NOW there is no interruption or cancellations of local activities in java-sdk. We have a task for it #1303 and I guess it's coming at some moment.

Sushisource · 2022-11-23T17:41:12Z

+        }
+
+        if (executionThrowable instanceof Error) {
+          // TODO Error inside Local Activity shouldn't be failing the local activity call.


Meant to be done in this PR?

It's old existing behavior. It's incorrect, but it's not trivial to fix correctly. And directly relevant to the goal of this PR. I opened a separate task for it: #1533

Sushisource · 2022-11-23T17:42:03Z

+        if (isRetryPolicyNotSet(activityTask)) {
+          executionContext.callback(
+              failed(
+                  activityId,
+                  RetryState.RETRY_STATE_RETRY_POLICY_NOT_SET,
+                  executionFailure,
+                  currentAttempt,
+                  null));
+          return;
+        }
+
+        RetryOptions retryOptions = RetryOptionsUtils.toRetryOptions(activityTask.getRetryPolicy());
+
+        if (RetryOptionsUtils.isNotRetryable(retryOptions, executionThrowable)) {
+          executionContext.callback(
+              failed(
+                  activityId,
+                  RetryState.RETRY_STATE_NON_RETRYABLE_FAILURE,
+                  executionFailure,
+                  currentAttempt,
+                  null));
+          return;
+        }
+
+        if (RetryOptionsUtils.areAttemptsReached(retryOptions, currentAttempt)) {
+          executionContext.callback(
+              failed(
+                  activityId,
+                  RetryState.RETRY_STATE_MAXIMUM_ATTEMPTS_REACHED,
+                  executionFailure,
+                  currentAttempt,
+                  null));
+          return;
+        }


Could combine these all by just setting a retry state local var and then passing that into the failed call -- looks like the next few too

Good eye, will refactor it.

Sushisource · 2022-11-23T17:45:52Z

+    public void run() {
+      LocalActivityExecutionContext executionContext = attemptTask.getExecutionContext();


FYI: I realized yesterday Core isn't retrying start-to-close timeouts right now, we opened a bug to do it

Sushisource · 2022-11-23T17:51:47Z

+        executionContext.callback(
+            failed(
+                activityId,
+                LOCAL_RETRY_LIMIT_RETRY_STATE,


Is this the state where we schedule a timer?

Where we WILL schedule a timer in one of the upcoming PRs, yeah.

Issue temporalio#1004

Spikhalskiy force-pushed the issue-1004 branch 18 times, most recently from c0dd625 to 3d2cfa2 Compare November 15, 2022 01:31

Spikhalskiy force-pushed the issue-1004 branch 12 times, most recently from 644f23a to 777486e Compare November 18, 2022 17:11

Spikhalskiy force-pushed the issue-1004 branch 4 times, most recently from 04e9496 to 8fdcec1 Compare November 19, 2022 00:19

Spikhalskiy mentioned this pull request Nov 20, 2022

More activity timeout failure compliance tests and improvements #1525

Merged

Spikhalskiy force-pushed the issue-1004 branch from dce5bc6 to 1189151 Compare November 22, 2022 05:17

Spikhalskiy marked this pull request as ready for review November 22, 2022 05:17

Spikhalskiy requested review from Sushisource, bergundy, cretz, mfateev and mmcshane as code owners November 22, 2022 05:17

Spikhalskiy force-pushed the issue-1004 branch 11 times, most recently from 7d8607a to a8a4b72 Compare November 23, 2022 17:02

Sushisource reviewed Nov 23, 2022

View reviewed changes

Spikhalskiy force-pushed the issue-1004 branch from a8a4b72 to 1341bd3 Compare November 23, 2022 21:12

Rework Local Activity scheduling

562b191

Issue temporalio#1004

Spikhalskiy force-pushed the issue-1004 branch from 1341bd3 to 562b191 Compare November 23, 2022 22:07

Sushisource approved these changes Nov 23, 2022

View reviewed changes

Spikhalskiy merged commit c5f0ebb into temporalio:master Nov 24, 2022

Spikhalskiy deleted the issue-1004 branch November 24, 2022 04:44

		@@ -286,10 +285,10 @@ private void processLocalActivityRequests(long startTimeNs) throws InterruptedEx
		// much sense. I believe we should add ScheduleToStart timeout for the local activities
		// as well.

		private static final RetryState LOCAL_RETRY_LIMIT_RETRY_STATE =
		RetryState.RETRY_STATE_IN_PROGRESS;

		public void run() {
		LocalActivityExecutionContext executionContext = attemptTask.getExecutionContext();

Conversation

Spikhalskiy commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was changed

Why?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Spikhalskiy Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Spikhalskiy commented Nov 8, 2022 •

edited

Loading

Spikhalskiy Nov 23, 2022 •

edited

Loading