Add OnConflictOptions Support #2415

justinp-tt · 2025-02-13T23:46:59Z

What was changed

Add OnConflictOptions support in the test server

Why?

Test server needs OnConflictOptions support

Checklist

Closes
SDK-3343
How was this tested:

Any docs updates needed?

temporal-test-server/src/main/java/io/temporal/internal/testservice/StateMachines.java

bergundy

Overall LGTM.

Could you please also add tests?

temporal-test-server/src/main/java/io/temporal/internal/testservice/StateMachines.java

...test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java

temporal-test-server/src/main/java/io/temporal/internal/testservice/StateMachines.java

temporal-test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowService.java

...test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java

rodrigozhou

We changed the server validation of StartWorkflowExecutionRequest.OnConflictOptions. Now, if AttachCompletionCallbacks is true, then AttachRequestId has to be true as well. Not sure if we need to make any changes here.

...test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java

…rvice/TestWorkflowMutableStateImpl.java Co-authored-by: Rodrigo Zhou <[email protected]>

temporal-test-server/src/main/java/io/temporal/internal/testservice/StateMachines.java

Quinn-With-Two-Ns · 2025-02-24T18:34:52Z

As part of this PR please remove the checks for the real server on these tests since this PR should enable them to pass on the test server:

Quinn-With-Two-Ns · 2025-02-24T18:37:35Z

...l-test-server/src/test/java/io/temporal/testserver/functional/WorkflowIdReusePolicyTest.java

@@ -118,6 +126,95 @@ public void secondWorkflowTerminatesFirst() {
    describe(execution2).assertStatus(WorkflowExecutionStatus.WORKFLOW_EXECUTION_STATUS_RUNNING);
  }

+  @Test
+  public void useExistingPolicy_RequestIdDeduplication() {


I think we should have test coverage that WORKFLOW_EXECUTION_OPTIONS_UPDATED is written to the workflows history.

bergundy · 2025-02-24T18:34:09Z

temporal-test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowService.java

-            && (status == WorkflowExecutionStatus.WORKFLOW_EXECUTION_STATUS_COMPLETED
-                || status == WorkflowExecutionStatus.WORKFLOW_EXECUTION_STATUS_CONTINUED_AS_NEW)) {
+        if (status == WORKFLOW_EXECUTION_STATUS_RUNNING) {
+          if (reusePolicy == WORKFLOW_ID_REUSE_POLICY_TERMINATE_IF_RUNNING


There's some edge cases that we handled in the server (or at least planned to) for reuse policy terminate if running and the different conflict policies.

Someone can address that in a follow up PR though.

CC @rodrigozhou

@rodrigozhou have you reviewed this code and verified the different edge cases?
Can wait for a follow up PR.

I'll do in a follow up PR, this one is already quite big.

temporal-test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowService.java

...test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java

Quinn-With-Two-Ns · 2025-02-24T18:48:43Z

...test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java

+
+      if (options.getAttachCompletionCallbacks()
+          && !request.getCompletionCallbacksList().isEmpty()) {
+        attrs.addAllAttachedCompletionCallbacks(request.getCompletionCallbacksList());


Do we resolve all these callbacks we are attaching? I can't find it anywhere in this PR

applyOnConflictOptions calls ctx.commitChanges(store); The save implementation makes a call to fireCallbacks. Is that where we would expect callbacks to be invoked?

I was assuming that we would trigger any callback attached to the workflow, that should be validated when tests are added.

It's already resolving. I just had to change to get the callbacks from this new attribute I added to TestWorkflowMutableStateImpl instead of reading from the startRequest.

bergundy

LGTM

bergundy · 2025-02-28T15:00:59Z

temporal-test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowService.java

-            && (status == WorkflowExecutionStatus.WORKFLOW_EXECUTION_STATUS_COMPLETED
-                || status == WorkflowExecutionStatus.WORKFLOW_EXECUTION_STATUS_CONTINUED_AS_NEW)) {
+        if (status == WORKFLOW_EXECUTION_STATUS_RUNNING) {
+          if (reusePolicy == WORKFLOW_ID_REUSE_POLICY_TERMINATE_IF_RUNNING


@rodrigozhou have you reviewed this code and verified the different edge cases?
Can wait for a follow up PR.

rodrigozhou · 2025-03-03T20:04:25Z

...test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java

+      try {
+        service.completeNexusOperation(
+            ref, ctx.getExecution().getWorkflowId(), startLink, completionEvent.get());
+      } catch (StatusRuntimeException e) {
+        // Callback destination not found should not block processing the callbacks nor
+        // completing the workflow.
+        if (e.getStatus().getCode() != Status.Code.NOT_FOUND) {
+          throw e;
+        }
+      }


@pdoerner This is what I did to address callback "not found".

I think if we do get NOT_FOUND (or really any error) we should log an error and/or store the failure somewhere so that it can be returned as part of the DescribeWorkflow response which has fields for callback's last failure, attempt time, etc

But we can also address that in a followup, doesn't necessarily have to be in this PR

rodrigozhou · 2025-03-03T20:08:06Z

temporal-test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowService.java

@@ -1465,6 +1488,7 @@ public String continueAsNew(
      String identity,
      ExecutionId continuedExecutionId,
      String firstExecutionRunId,
+      TestWorkflowMutableState previousExecutionState,


Need the previous mutable state so we can copy all callbacks to the continued as new workflow (L1511-1514 below).

justinp-tt added 2 commits February 13, 2025 17:44

Add OnConflictOptions support

dfc901d

Add OnConflictOptions support

85cff62

justinp-tt requested review from bergundy, rodrigozhou and Quinn-With-Two-Ns February 13, 2025 23:46

justinp-tt self-assigned this Feb 13, 2025

Quinn-With-Two-Ns reviewed Feb 14, 2025

View reviewed changes

temporal-test-server/src/main/java/io/temporal/internal/testservice/StateMachines.java Outdated Show resolved Hide resolved

Quinn-With-Two-Ns reviewed Feb 14, 2025

View reviewed changes

temporal-test-server/src/main/java/io/temporal/internal/testservice/StateMachines.java Outdated Show resolved Hide resolved

Add OnConflictOptions support

1a6be22

justinp-tt marked this pull request as ready for review February 14, 2025 14:36

justinp-tt requested a review from a team as a code owner February 14, 2025 14:36

bergundy reviewed Feb 14, 2025

View reviewed changes

temporal-test-server/src/main/java/io/temporal/internal/testservice/StateMachines.java Outdated Show resolved Hide resolved

...test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java Outdated Show resolved Hide resolved

justinp-tt added 2 commits February 14, 2025 11:12

Add OnConflictOptions support

5f08c48

Add OnConflictOptions support

5bfb3e5

rodrigozhou requested changes Feb 14, 2025

View reviewed changes

justinp-tt added 2 commits February 19, 2025 17:15

Add OnConflictOptions support

47cd098

Add OnConflictOptions support

3896f3d

Quinn-With-Two-Ns mentioned this pull request Feb 20, 2025

Add support for attaching to a running workflow #2424

Merged

rodrigozhou reviewed Feb 21, 2025

View reviewed changes

...test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java Outdated Show resolved Hide resolved

...test-server/src/main/java/io/temporal/internal/testservice/TestWorkflowMutableStateImpl.java Outdated Show resolved Hide resolved

justinp-tt and others added 2 commits February 21, 2025 12:13

Update temporal-test-server/src/main/java/io/temporal/internal/testse…

6d6345e

…rvice/TestWorkflowMutableStateImpl.java Co-authored-by: Rodrigo Zhou <[email protected]>

Add OnConflictOptions support

ca8f3d5

justinp-tt requested review from Quinn-With-Two-Ns and bergundy February 24, 2025 18:21

Quinn-With-Two-Ns reviewed Feb 24, 2025

View reviewed changes

temporal-test-server/src/main/java/io/temporal/internal/testservice/StateMachines.java Outdated Show resolved Hide resolved

Quinn-With-Two-Ns reviewed Feb 24, 2025

View reviewed changes

bergundy reviewed Feb 24, 2025

View reviewed changes

Quinn-With-Two-Ns reviewed Feb 24, 2025

View reviewed changes

justinp-tt added 2 commits February 24, 2025 14:47

Add OnConflictOptions support

b56db56

Merge branch 'master' into on-conflict-options

9151216

justinp-tt and others added 6 commits February 24, 2025 14:53

Add OnConflictOptions support

6c25aec

Add OnConflictOptions support

74aada4

Add OnConflictOptions support

0d5b919

Add OnConflictOptions support

81f9ec4

Add OnConflictOptions support

c6b9925

fixes

44f562e

justinp-tt assigned rodrigozhou and unassigned justinp-tt Feb 25, 2025

Merge branch 'master' into on-conflict-options

8158ab0

rodrigozhou requested review from bergundy and Quinn-With-Two-Ns February 28, 2025 05:05

tests

35bbf02

rodrigozhou force-pushed the on-conflict-options branch from 6ab7747 to 35bbf02 Compare February 28, 2025 05:06

bergundy approved these changes Feb 28, 2025

View reviewed changes

bergundy requested a review from rodrigozhou February 28, 2025 15:03

rodrigozhou added 2 commits February 28, 2025 13:38

fix callbacks in describe and continue as new

86ff638

unblock UseExisting

1e69607

rodrigozhou force-pushed the on-conflict-options branch from 171f29a to 5ef7c2f Compare March 3, 2025 20:01

fix tests

a507b5e

rodrigozhou reviewed Mar 3, 2025

View reviewed changes

rodrigozhou force-pushed the on-conflict-options branch from 5ef7c2f to a507b5e Compare March 3, 2025 20:44

revert unblock UseExisting

e3ae76a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OnConflictOptions Support #2415

Add OnConflictOptions Support #2415

justinp-tt commented Feb 13, 2025 •

edited by rodrigozhou

Loading

bergundy left a comment

rodrigozhou left a comment

Quinn-With-Two-Ns commented Feb 24, 2025

Quinn-With-Two-Ns Feb 24, 2025

bergundy Feb 24, 2025

bergundy Feb 28, 2025

rodrigozhou Feb 28, 2025

Quinn-With-Two-Ns Feb 24, 2025

justinp-tt Feb 25, 2025

bergundy Feb 26, 2025

rodrigozhou Feb 28, 2025

bergundy left a comment

bergundy Feb 28, 2025

rodrigozhou Mar 3, 2025

pdoerner Mar 4, 2025

pdoerner Mar 4, 2025

rodrigozhou Mar 3, 2025

Add OnConflictOptions Support #2415

Are you sure you want to change the base?

Add OnConflictOptions Support #2415

Conversation

justinp-tt commented Feb 13, 2025 • edited by rodrigozhou Loading

What was changed

Why?

Checklist

bergundy left a comment

Choose a reason for hiding this comment

rodrigozhou left a comment

Choose a reason for hiding this comment

Quinn-With-Two-Ns commented Feb 24, 2025

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

bergundy left a 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

justinp-tt commented Feb 13, 2025 •

edited by rodrigozhou

Loading