StateMachine rewrite #172

mfateev · 2020-08-07T14:33:19Z

Complete rewrite of the workflow task handling state machine. It was caused by the need to fix immediate cancellation issues that weren't possible to solve using previous design.

src/main/java/io/temporal/activity/ActivityOptions.java

mastermanu · 2020-08-07T16:15:58Z

src/main/java/io/temporal/internal/grpc/GrpcTracingInterceptor.java

@@ -49,7 +49,7 @@
        next.newCall(method, callOptions)) {
      @Override
      public void sendMessage(ReqT message) {
-        log.trace("Invoking " + method.getFullMethodName() + "with input: " + message);
+        log.trace("Invoking \"" + method.getFullMethodName() + "\" with input: " + message);


[Nit] dumb question (based on similar change above), when do we use single quote (') vs double quote (") for these types of messages?

There is no guideline here. I would support coming up with one and making sure that all the log messages obey it.

mastermanu · 2020-08-07T16:17:34Z

src/main/java/io/temporal/internal/metrics/MetricsType.java

@@ -42,13 +42,19 @@
      TEMPORAL_METRICS_PREFIX + "workflow_task_schedule_to_start_latency";
  public static final String WORKFLOW_TASK_EXECUTION_LATENCY =
      TEMPORAL_METRICS_PREFIX + "workflow_task_execution_latency";
+  // Total latency of a workflow task which can include multiple synchronous decision tasks


even with the comment, it is still not obvious to me what the difference is between execution latency and execution total latency?

[Nit] also styling question on when to use "//" comment vs "/** */" comment delimiters, as is done below on lines 48, 51

Fixed comments.

To understand this comment you need to know about workflow task heartbeat feature. Basically a workflow task when reporting completion can set ForceCreateNewWorkflowTask property to true on RespondWorkflowTaskCompletedRequest. In this case, the new workflow task is created immediately and returned as a response to the RespondWorkflowTaskCompletedRequest call. Such call acts as a heartbeat that extends the duration of a workflow task. Thus a single total execution latency of a workflow task can be larger than latencies of multiple such synchronously created tasks.

mastermanu · 2020-08-07T16:26:23Z

src/main/java/io/temporal/internal/replay/ExecuteLocalActivityParameters.java

-    return elapsedTime;
+  @Override
+  public String toString() {
+    return "ExecuteLocalActivityParameters{" + "activityTask=" + activityTask + '}';


[Nit] no single or double quote delimiters around activityTask here?

Also you can remove the first plus sign and concatenate the first two strings (e.g. "ExecuteLocalActivityParameters{activityTask=" + activityTask + "}"

mastermanu · 2020-08-07T16:35:01Z

src/main/java/io/temporal/internal/replay/ReplayWorkflowContext.java


-  Consumer<Exception> signalWorkflowExecution(
+  Functions.Proc1<Exception> signalExternalWorkflowExecution(


Curious why some of these methods have full comments (such as startChildWorkflow), but then some like this one (signalExternalWorkflowExecution) don't

mastermanu · 2020-08-07T16:48:43Z

src/main/java/io/temporal/internal/replay/ReplayWorkflowTaskHandler.java

      // Only fail workflow task on the first attempt, subsequent failures of the same workflow task
      // should timeout. This is to avoid spin on the failed workflow task as the service doesn't
      // yet increase the retry interval.
-      if (workflowTask.getAttempt() > 0) {
+      if (workflowTask.getAttempt() > 1) {


hopefully we added a unit test to cover this as this should have been caught when I was syncing to Alex's latest proto changes

Good catch. We had a test that ensured that the worker doesn't spin on the task failure. But after I added an additional check that the task failure is reported exactly once I found that it wasn't doing so. Here is the new unit test and the fix. The unit test fails with attempts condition changed.

mastermanu · 2020-08-07T16:52:08Z

src/main/java/io/temporal/internal/replay/WorkflowExecutor.java

+   * Handles a single workflow task.
+   *
+   * @param workflowTask task to handle
+   * @return true if new task should be created synchronously as local activities are still running.


[Nit] what does it mean by return "true if new task should be created synchronously"? Does this mean not null?

See my earlier comment about force creating workflow tasks.

mastermanu · 2020-08-07T16:53:03Z

src/main/java/io/temporal/internal/replay/WorkflowExecutor.java


    public WorkflowTaskResult(
        List<Command> commands,
        Map<String, WorkflowQueryResult> queryResults,
-        boolean forceCreateNewWorkflowTask,
-        boolean finalCommand) {
+        boolean finalCommand,


we swapped the order to two arguments with the same type...hopefully all the callers have been updated

Agree that it is error-prone. Changed WorkflowTaskResult to use the Builder pattern.

mastermanu · 2020-08-07T16:56:01Z

src/main/java/io/temporal/internal/statemachines/ActionOrEventType.java

+import io.temporal.api.enums.v1.CommandType;
+import io.temporal.api.enums.v1.EventType;
+
+class ActionOrEventType<Action> {


Name of class implies two possible configurations, but it looks like its ActionOrEventTypeOrCommandType (three)

Refactored StateMachine nomenclature to match the standard one.

mastermanu · 2020-08-07T17:04:33Z

src/main/java/io/temporal/internal/statemachines/ActivityStateMachine.java

+        .add(
+            State.SCHEDULE_COMMAND_CREATED,
+            CommandType.COMMAND_TYPE_SCHEDULE_ACTIVITY_TASK,
+            State.SCHEDULE_COMMAND_CREATED)


is this a typo / copy paste error (SCHEDULE_COMMAND_CREATED twice)? Maybe I'm interpreting this incorrectly? I assumed first line is current state, subsequent line is potential transition, second line is final state and last line is an additional callback to run. Or should we add another state between "CREATED" and "SCHEDULE_COMMAND_CREATED"?

This is practically saying that this event is not causing a state transition. We could introduce another event here, but it doesn't really bring any value as no action is executed.

mastermanu · 2020-08-07T17:14:15Z

src/main/java/io/temporal/internal/statemachines/ActivityStateMachine.java

+            State.STARTED_ACTIVITY_CANCEL_COMMAND_CREATED,
+            ActivityStateMachine::createRequestCancelActivityTaskCommand)
+        .add(
+            State.STARTED_ACTIVITY_CANCEL_COMMAND_CREATED,


for this state and the cancel_command_Created state, we don't have transitions for a timed out event?

I've added unit tests that demonstrate that these transitions are not possible. The basic idea is that the workflow code always executes in the context of an event loop. It causes activity timeouts to be delivered before the code that causes the timeout which generates the cancel command.

mastermanu · 2020-08-07T17:16:04Z

src/main/java/io/temporal/internal/statemachines/ActivityStateMachine.java

+    SCHEDULED_ACTIVITY_CANCEL_EVENT_RECORDED,
+    STARTED_ACTIVITY_CANCEL_COMMAND_CREATED,
+    STARTED_ACTIVITY_CANCEL_EVENT_RECORDED,
+    COMPLETED_CANCEL_REQUESTED,


where are these 3 *_CANCEL_REQUESTED states used?

Good catch. Removed.

mastermanu · 2020-08-07T17:28:05Z

src/main/java/io/temporal/internal/statemachines/ActivityStateMachine.java

+        Failure.newBuilder()
+            .setActivityFailureInfo(failureInfo)
+            .setCause(timedOut.getFailure())
+            .setMessage("Activity task timedOut")


should we have string constants file for these error messages so that its easy for a technical writer to review or so we can do translations (in the future)

Would you file an issue for this? I think it is a great idea, but we want to do it holistically with the whole SDK.

Hopefully we will never do translations :-)

mastermanu · 2020-08-07T17:34:41Z

src/main/java/io/temporal/internal/statemachines/CancelExternalStateMachine.java

+            .setWorkflowId(requestCancelAttributes.getWorkflowId())
+            .setRunId(requestCancelAttributes.getRunId())
+            .build();
+    completionCallback.apply(null, new CancelExternalWorkflowException(execution, "", null));


we don't have any information as to why cancellation failed?

Added message.

mastermanu · 2020-08-07T17:41:23Z

src/main/java/io/temporal/internal/statemachines/ChildWorkflowStateMachine.java

+            State.CANCELED,
+            ChildWorkflowStateMachine::cancelStartChildCommand)
+        .add(
+            State.START_EVENT_RECORDED,


can't get a cancel event type here?

ChildWorkflow is canceled using CancelExternalStateMachine.

src/main/java/io/temporal/internal/statemachines/ChildWorkflowStateMachine.java

mastermanu · 2020-08-07T18:41:21Z

src/main/java/io/temporal/internal/statemachines/DynamicTransitionTarget.java

+        return state;
+      }
+    }
+    throw new IllegalStateException(


shouldn't we do this check before applying the callback?

After any such exception, the whole StateMachines object should be thrown away.

… state-machine-review

mastermanu · 2020-08-09T22:25:55Z

src/main/java/io/temporal/internal/statemachines/ActivityStateMachine.java

+            State.SCHEDULED_ACTIVITY_CANCEL_COMMAND_CREATED,
+            ActivityStateMachine::createRequestCancelActivityTaskCommand)
+        .add(
+            State.SCHEDULED_ACTIVITY_CANCEL_COMMAND_CREATED,


[Nit] swap the order of these two state transitions just for consistency (have the self-transition appear first)

alexshtin · 2020-08-11T22:23:13Z

src/main/java/io/temporal/activity/ActivityCancellationType.java

@@ -24,7 +24,7 @@

 /**
 * Defines behaviour of the parent workflow when {@link CancellationScope} that wraps child workflow
- * execution request is cancelled. The result of the cancellation independently of the type is a
+ * execution request is canceled. The result of the cancellation independently of the type is a


We have the same mess on GoSDK and server side.

temporalio/sdk-go#227

alexshtin

I am not in charge to approve this but it is impressive. One day we should clean up Go SDK also.

mastermanu · 2020-08-12T20:46:54Z

src/main/java/io/temporal/internal/statemachines/LocalActivityStateMachine.java

+                  ExplicitEvent.SCHEDULE,
+                  State.REQUEST_PREPARED,
+                  LocalActivityStateMachine::sendRequest)
+              .add(State.REQUEST_PREPARED, ExplicitEvent.GET_REQUEST, State.REQUEST_SENT)


is there a better name we can come up with than "GET_REQUEST?" maybe "CREATE_REQUEST" or CREATEORGET_REQUEST or something like that? And how do I interpret what REQUEST_SENT means? To me that sounds like something was sent to the server, but this is a local activity, so not sure if there is a more appropriate name

Renamed to MARK_AS_SENT

mastermanu · 2020-08-12T20:51:09Z

src/main/java/io/temporal/internal/statemachines/LocalActivityStateMachine.java

+              .add(State.REQUEST_PREPARED, ExplicitEvent.GET_REQUEST, State.REQUEST_SENT)
+              .add(
+                  State.REQUEST_SENT,
+                  ExplicitEvent.NON_REPLAY_WORKFLOW_TASK_STARTED,


this basically means local activity execution started? or that we've started executing the workflow? is there a scenario where this same event gets invoked twice? can we come up with a more friendly name?

This means that WORKFLOW_TASK_STARTED event for a non replay task has started. Not sure what would be the better name.

mastermanu · 2020-08-12T20:55:17Z

src/main/java/io/temporal/internal/statemachines/LocalActivityStateMachine.java

+                  State.REQUEST_SENT)
+              .add(
+                  State.REQUEST_SENT,
+                  ExplicitEvent.HANDLE_RESPONSE,


and is this for explicitly reporting the local activity result? if so, can we have better naming for explicit event here?

renamed to HANDLE_RESULT

mastermanu · 2020-08-12T21:02:30Z

src/main/java/io/temporal/internal/statemachines/LocalActivityStateMachine.java

+                  State.MARKER_COMMAND_RECORDED,
+                  LocalActivityStateMachine::notifyResultFromEvent)
+              .add(
+                  // This is to cover the following edge case:


had a hard time following this. when calling getExecutionState wouldn't it still be as if the "first time" it ran? OR does the force workflowtask is started create a persistent checkpoint

Forced workflow task is a workflow task that is recorded in the history the same way as a normal workflow task.

mastermanu · 2020-08-13T03:50:15Z

src/main/java/io/temporal/worker/WorkerOptions.java

@@ -143,8 +144,8 @@ public Builder setMaxConcurrentLocalActivityExecutionSize(
     * every 10 seconds. This can be used to protect down stream services from flooding. The zero
     * value of this uses the default value. Default is unlimited.
     */
-    public Builder setTaskQueueActivitiesPerSecond(double taskQueueActivitiesPerSecond) {
-      this.taskQueueActivitiesPerSecond = taskQueueActivitiesPerSecond;
+    public Builder setMaxTaskQueueActivitiesPerSecond(double maxTaskQueueActivitiesPerSecond) {


why not have negative check here? Or do we treat that as 0? Interesting that we have different behavior than above (although I guess w/different defaults that makes sense)

The above option is used only locally. So validation should be done by the SDK. The maxTaskQueueActivitiesPerSecond is an option sent to the service. We made a design decision to not validate on the client any server-side options.

shawnhathaway · 2020-08-14T14:30:45Z

src/main/java/io/temporal/internal/statemachines/ChildWorkflowStateMachine.java

+                  ChildWorkflowStateMachine::createStartChildCommand)
+              .add(
+                  State.START_COMMAND_CREATED,
+                  CommandType.COMMAND_TYPE_START_CHILD_WORKFLOW_EXECUTION,


What are the side effects of not defining the no-op transitions that are command based?

Only defined transitions are allowed. Workflow task fails if command or event is applied to the state that doesn't have a transition defined for it.

shawnhathaway · 2020-08-14T14:32:21Z

src/main/java/io/temporal/internal/statemachines/EntityStateMachineBase.java

+   */
+  @Override
+  public void handleCommand(CommandType commandType) {
+    stateMachine.handleCommand(commandType, (Data) this);


Why are events registered/validated, but commands are not?

It is pretty evolved. The short answer is that this validation is required to support mutable side effect and versions which can have events missing for the given state machine. So if event doesn't match then it still can be valid as it just doesn't apply to this specific state machine. The same doesn't apply to commands as they always have to match the state machine that created them.

State Machine Rewrite temporalio#172 left a couple of state machine code in the class that is not responsible for handling state transitions anymore and this code is not invoked.

State Machine Rewrite #172 left a couple of state machine code in the class that is not responsible for handling state transitions anymore and this code is not invoked.

StateMachine refactoring

13c2b0f

mfateev requested review from mastermanu, shawnhathaway, samarabbas and alexshtin August 7, 2020 14:33

mastermanu reviewed Aug 7, 2020

View reviewed changes

src/main/java/io/temporal/activity/ActivityOptions.java Show resolved Hide resolved

mastermanu reviewed Aug 7, 2020

View reviewed changes

src/main/java/io/temporal/internal/statemachines/ChildWorkflowStateMachine.java Show resolved Hide resolved

mastermanu reviewed Aug 7, 2020

View reviewed changes

mfateev added 7 commits August 8, 2020 09:45

PR feedback and broken unit test

0c08449

Fixed workflow task failure reporting

250f70d

WorkflowTaskResult now is created through a Builder

68db6c6

StateMachine nomenclature cleanup

1b1254b

Merge branch 'master' of github.com:temporalio/temporal-java-sdk into…

8243071

… state-machine-review

initial command refactoring

4df71d6

Added ActivityStateMachineTest and fixed cancellation failure

e1e3fa6

mastermanu reviewed Aug 9, 2020

View reviewed changes

alexshtin reviewed Aug 11, 2020

View reviewed changes

alexshtin reviewed Aug 12, 2020

View reviewed changes

mfateev added 3 commits August 11, 2020 17:47

Simplified local activity handling

dd642f0

allOf, anyOf and iterator

1cc7b0c

event loop

abf241c

mastermanu reviewed Aug 12, 2020

View reviewed changes

mfateev added 2 commits August 12, 2020 20:07

Refactored ReplayWorkflowExecutor

65ffd00

Renamed StatefulTaskHandler to WorkflowRunTaskHandler

919cad6

mastermanu reviewed Aug 13, 2020

View reviewed changes

mfateev added 4 commits August 12, 2020 21:25

Fixed local activity retries

d6918a0

PR comments

92a3e36

PR feedback

0a4dc38

Fixed Saga defaults

81a5fb3

mastermanu approved these changes Aug 14, 2020

View reviewed changes

Fixed sticky task and cache invalidation

4ddab73

shawnhathaway approved these changes Aug 14, 2020

View reviewed changes

mfateev merged commit d357d79 into temporalio:master Aug 14, 2020

mfateev deleted the state-machine-review branch August 14, 2020 17:28

This was referenced Aug 22, 2020

Log activity failures #79

Closed

Workflow.wrap and Activity.wrap fail on chained Throwable #149

Closed

meiliang86 mentioned this pull request Oct 10, 2020

StateMachine rewrite cadence-workflow/cadence-java-client#547

Open

Spikhalskiy mentioned this pull request Jul 7, 2022

Cleanup dead state machines code #1302

Merged

Spikhalskiy added a commit that referenced this pull request Jul 7, 2022

Cleanup dead state machines code (#1302)

ae5065d

State Machine Rewrite #172 left a couple of state machine code in the class that is not responsible for handling state transitions anymore and this code is not invoked.


		Consumer<Exception> signalWorkflowExecution(
		Functions.Proc1<Exception> signalExternalWorkflowExecution(

StateMachine rewrite #172

StateMachine rewrite #172

Conversation

mfateev commented Aug 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastermanu Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastermanu Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastermanu Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastermanu Aug 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexshtin left a comment

Choose a reason for hiding this comment

mastermanu Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastermanu Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastermanu Aug 7, 2020 •

edited

Loading

mastermanu Aug 7, 2020 •

edited

Loading

mastermanu Aug 7, 2020 •

edited

Loading

mastermanu Aug 9, 2020 •

edited

Loading

mastermanu Aug 12, 2020 •

edited

Loading

mastermanu Aug 12, 2020 •

edited

Loading