New Update-with-Start API #2337

stephanos · 2024-11-29T02:12:32Z

What was changed

Changed the Update-with-Start API to use a WithStartWorkflowOperation that expresses the workflow start.

stephanos · 2024-11-29T02:50:15Z

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

+                .setCode(Status.INVALID_ARGUMENT.getCode().value())
+                .setMessage("INVALID_ARGUMENT: WorkflowStartDelay is not supported.")
+                .build(),
+            null); // update aborted


Moved this up to the other Start Operation validation.

stephanos · 2024-11-29T02:56:11Z

temporal-sdk/src/main/java/io/temporal/client/WorkflowStubImpl.java

+    if (startOp.getStub() == null) {
+      throw new IllegalStateException(
+          "WithStartWorkflowOperation was created with different WorkflowStub");
+    }


So this is worth discussing: the Start Operation's getResult needs a class for the Workflow's result type to work. When using the typed stub to initialize it, it is inferred. When using the untyped, it is provided by the user explicitly.

However, when initialize it via the typed stub and using the untyped stub to run startUpdateWithStart, the type inference step was skipped and any call to startOp.getResult() fails.

I don't see a way around this other than removing the getResult shortcut entirely. Then this check can be safely removed.

I think it is worth considering if the untyped stub startUpdateWithStart API should even take a WithStartWorkflowOperation ?

I guess startUpdateWithStart is supposed to take a vargs , and you can't have two varags

Yup, @dandavison and I had that conversation and we were leaning towards using the WithStartWorkflowOperation for the untyped startUpdateWithStart API for these reasons:

(1) a signature like startUpdateWithStart(UpdateOptions<R> options, Object[] updateArgs, Object[] startArgs) is potentially dangerous because users might mix up the order of the args (counterpoint: the existing signalWithStart has the same problem)
(2) consistent use of the startOp parameter across typed and untyped APIs
(3) allows convenience function getResult()

(1) is the main reason, (2) and (3) are minor reasons.

Having said that, neither of us - so far - felt very strongly about it.

temporal-sdk/src/main/java/io/temporal/client/WorkflowInvocationHandler.java

stephanos · 2024-11-29T04:19:12Z

temporal-sdk/src/main/java/io/temporal/client/WorkflowStubImpl.java

+    if (options.getWorkflowIdConflictPolicy() == null) {
+      throw new IllegalStateException(
+          "Required parameter WorkflowIdConflictPolicy in WorkflowOptions is missing in WorkflowStub");


Important change: we're making the WorkflowIdConflictPolicy a required field for UwS across all SDKs.

We could also consider an error message like

WorkflowIdConflictPolicy is required in WorkflowOptions for Update-With-Start

Yeah +1 to this wording of the error

stephanos · 2024-11-29T04:32:09Z

temporal-sdk/src/main/java/io/temporal/client/WorkflowStubImpl.java

      WorkflowClientCallsInterceptor.WorkflowUpdateWithStartOutput<R> output =
          workflowClientInvoker.updateWithStart(input);
+
+      // gather outputs
      workflowExecution = output.getWorkflowStartOutput().getWorkflowExecution();
      populateExecutionAfterStart(workflowExecution);


In other SDKs where we'll have a Future to obtain the workflow handle from the start operation, that future is immediately fulfilled once the run id is known. This can happen before the update wait stage has been reached.

I wonder if we want to do sth similar here. We could assign the workflow execution to the stub as soon as the run id is known. But it's not clear to me that it would do much since calling other functions on the stub are not waiting for the execution to be available.

I think we shouldn't do anything right now and re-consider based on feedback.

dandavison · 2024-11-29T23:51:01Z

Summary of some parts of user-facing API in this PR:

WithStartWorkflowOperation:
- constructors
  - WithStartWorkflowOperation(startMethod, args...)
  - WithStartWorkflowOperation(WorkflowStub, Class<? extends R> resultClass, Object...)
- R getResult
static WorkflowClient methods
- WorkflowUpdateHandle<R> startUpdateWithStart(updateMethod, updateArgs..., UpdateOptions<R>, WithStartWorkflowOperation)
- R executeUpdateWithStart(updateMethod, updateArgs..., UpdateOptions<R>, WithStartWorkflowOperation)
WorkflowStub methods
- WorkflowUpdateHandle<R> startUpdateWithStart(UpdateOptions<R>, Object[], WithStartWorkflowOperation)
- R executeUpdateWithStart(UpdateOptions<R>, Object[], WithStartWorkflowOperation)
Interceptor
- WorkflowUpdateWithStartInput<R> => WorkflowUpdateWithStartOutput<R>, both of which contain a start and update component

So the untyped stub methods obtain the update name from the UpdateOptions, and the WorkflowClient methods take an updateMethod, but also take UpdateOptions<R>. Can you confirm this is all as intended?

stephanos · 2024-11-30T00:59:41Z

@dandavison yup, that's all correct!

So the untyped stub methods obtain the update name from the UpdateOptions, and the WorkflowClient methods take an updateMethod, but also take UpdateOptions. Can you confirm this is all as intended?

That's true. I believe it's the same for the existing startUpdate APIs in WorkflowClient (plus the WithStartWorkflowOperation at the end).

Now I wonder how it picks the final update name: the method's annotation or the update options' updateName? 👀 - ah, it returns an error Update name in the options doesn't match the method name: nonsense != func. I'll have to add that, too. done!

dandavison · 2024-11-29T19:26:43Z

temporal-sdk/src/main/java/io/temporal/client/WorkflowClient.java

@@ -857,300 +857,652 @@ static <R, A1, A2, A3, A4, A5, A6> WorkflowUpdateHandle<R> startUpdate(
        updateMethod, arg1, arg2, arg3, arg4, arg5, arg6, options);
  }

+  // TODO: UwS


[comment to be deleted]

dandavison · 2024-11-29T19:27:53Z

temporal-sdk/src/main/java/io/temporal/client/WorkflowClient.java

+  static <R> WorkflowUpdateHandle<R> startUpdateWithStart(
+      Proc updateMethod,
+      @Nonnull UpdateOptions<R> options,
+      @Nonnull WithStartWorkflowOperation<?> withStartOperation) {


I think we can name the parameter the straightforward/obvious name: startOperation.

dandavison · 2024-11-29T21:50:36Z

temporal-sdk/src/main/java/io/temporal/client/WorkflowClient.java

+        () -> updateMethod.apply(arg1, arg2, arg3, arg4, arg5, arg6), options, withStartOperation);
+  }
+
+  // TODO: UwS


[to be deleted]

dandavison · 2024-11-29T22:07:28Z

temporal-sdk/src/main/java/io/temporal/client/WorkflowStubImpl.java

+    if (options.getWorkflowIdConflictPolicy() == null) {
+      throw new IllegalStateException(
+          "Required parameter WorkflowIdConflictPolicy in WorkflowOptions is missing in WorkflowStub");


We could also consider an error message like

WorkflowIdConflictPolicy is required in WorkflowOptions for Update-With-Start

Quinn-With-Two-Ns · 2024-11-30T03:44:42Z

temporal-sdk/src/main/java/io/temporal/client/WorkflowClient.java

+  static <A1, R> R executeUpdateWithStart(
+      Func1<A1, R> updateMethod,
+      A1 arg1,
+      @Nonnull UpdateOptions<R> options,


It is a bit odd to use UpdateOptions here since almost none of the options here make any sense to set... The only option is makes sense to set is the update ID.

I guess when we have durable on admitted support we will need to have a WaitStage parameter for that...

That's absolutely true. I'd support simplifying this executeUpdateWithStart(updateMethod, args ..., startOp). We can always add a new overload when we need it.

I really want to avoid adding overloads to these methods if we can avoid it because the number of methods will explode with all the combinations for different numbers of parameters. I would support adding a TypedUpdateOptions that only takes updateID and WaitStage and is used for all the update APIs on WorkflowClient

Quinn-With-Two-Ns · 2024-12-02T05:24:48Z

A general comment can we verify exceptions are consistent with update? While working on another PR https://github.com/temporalio/sdk-java/pull/2339/files#diff-deb992241240a3c5bff6838f447cddb38ed7978751405c2efe1daf563a00259bR481 I noticed that theupdateWithStart API was returning a WorkflowServiceException instead of a WorkflowUpdateException. My PR fixed it for this test, but something we should double check I think.

stephanos · 2024-12-02T20:00:23Z

temporal-sdk/src/main/java/io/temporal/client/UpdateOptions.java

+  void validateWaitForCompleted() {
+    if (waitForStage != null && waitForStage != WorkflowUpdateStage.COMPLETED) {
+      throw new IllegalArgumentException(
+              "waitForStage must be unspecified or " + WorkflowUpdateStage.COMPLETED);
+    }
+  }


So I added this because I think users would rather ensure that the UwS call waits for "completed" when they call executeUpdateWithStart, than have it be "accepted" (accidentally) and potentially impacting their experience (since an additional polling step might be needed).

This also seems better than silently setting it, but I don't feel strongly about this.

Whatever we decide, the behavior for the "execute update" API(s) should do the same IMO.

the behavior for the "execute update" API(s) should do the same IMO

The execute update APi in java is just calling the method on the stub hehe, but that uses COMPLETED

stephanos · 2024-12-02T20:22:06Z

I noticed that theupdateWithStart API was returning a WorkflowServiceException instead of a WorkflowUpdateException.

@Quinn-With-Two-Ns My understanding is that the method invocations of startUpdate, startUpdateWithStart and executeUpdateWithStart on WorkflowStubImpl throw a WorkflowServiceException if anything goes wrong with the RPC call (e.g. it was invalid). But then the getResult or getResultAsync().get() calls throw WorkflowUpdateException if there was an issue obtaining the result. Is that not correct?

Quinn-With-Two-Ns · 2024-12-02T20:35:08Z

@stephanos

WorkflowStubImpl throw a WorkflowServiceException if anything goes wrong with the RPC call (e.g. it was invalid).

Yes, but if the update is rejected or failed that is not an RPC issue and should not be surfaced as such. It should return an WorkflowUpdateException

stephanos · 2024-12-02T22:04:10Z

@Quinn-With-Two-Ns From what I can tell, WorkflowUpdateException is only thrown when the update request or the poll update request receive a failure outcome.

UwS does the same, and returns a WorkflowServiceException for anything else.

This includes when there's (a) server validation/rate limiting error (for start or update) or (b) the update never becomes durable and times out (client-side). Which seems to match the vanilla update behavior.

Quinn-With-Two-Ns · 2024-12-05T18:25:53Z

temporal-sdk/src/main/java/io/temporal/common/interceptors/WorkflowClientCallsInterceptor.java

@@ -233,21 +234,20 @@ public WorkflowStartOutput getWorkflowStartOutput() {

  final class WorkflowUpdateWithStartInput<R> {
    private final WorkflowStartInput workflowStartInput;
-    private final UpdateWithStartWorkflowOperation<R> updateOperation;
+    private final StartUpdateInput<R> workflowUpdateInput;


@cretz pointed out in the Go SDK PR interceptors shouldn't reuse input/output from other interceptors, which I agree with , but the Java SDK has done that historically. I don't think we need to block the PR on this , but we should resist this before we make UwS stable in the Java SDK.

I can file a ticket so we remember 👍

dandavison · 2024-12-05T23:09:53Z

temporal-sdk/src/main/java/io/temporal/client/WorkflowStub.java

+   *
+   * @param updateOptions options that will be used to configure and start a new update request
+   * @param updateArgs update method arguments
+   * @param starArgs workflow start arguments


typo, also in parameter names in code.

dandavison

LGTM

stephanos force-pushed the uws-new-api branch 2 times, most recently from 2239e6d to f9e0ccb Compare November 29, 2024 02:48

stephanos commented Nov 29, 2024

View reviewed changes

stephanos force-pushed the uws-new-api branch from f9e0ccb to 18a33d6 Compare November 29, 2024 02:52

stephanos commented Nov 29, 2024

View reviewed changes

stephanos force-pushed the uws-new-api branch from 18a33d6 to cc8f73a Compare November 29, 2024 02:59

Quinn-With-Two-Ns reviewed Nov 29, 2024

View reviewed changes

temporal-sdk/src/main/java/io/temporal/client/WorkflowInvocationHandler.java Outdated Show resolved Hide resolved

stephanos force-pushed the uws-new-api branch from cc8f73a to e1aedb4 Compare November 29, 2024 04:02

stephanos commented Nov 29, 2024

View reviewed changes

stephanos force-pushed the uws-new-api branch 2 times, most recently from 776f4ea to b645a0f Compare November 29, 2024 04:53

stephanos marked this pull request as ready for review November 30, 2024 01:54

stephanos requested a review from a team as a code owner November 30, 2024 01:54

dandavison reviewed Nov 30, 2024

View reviewed changes

Quinn-With-Two-Ns reviewed Nov 30, 2024

View reviewed changes

stephanos commented Dec 2, 2024

View reviewed changes

stephanos added 10 commits December 2, 2024 12:06

New Update-with-Start API

26c6b5b

introduce UpdateWithStartOptions

c353737

typo

c7c56ff

add some docs

d4156d3

fix parameter name: updateMethod

c8a4014

re-use UpdateOptions merge code

9d4fc28

fix docstrings

29dee54

more docstrings

9733179

rename helper

41e5097

rename withStartOperation

21a7b3d

stephanos added 6 commits December 2, 2024 12:06

consistent param name

134e9a1

tweak conflict policy error message

20305c2

fix test

79051cf

rename to updateOptions

2206e6e

validate execute uses completed stage

bb336bf

formatting

654c08c

stephanos force-pushed the uws-new-api branch from 6c50c27 to 654c08c Compare December 2, 2024 20:06

stephanos added 2 commits December 2, 2024 13:46

add test comment

7b3b17b

remove unused variable

e695fed

stephanos added 3 commits December 4, 2024 11:53

remove Start Operation from untyped

ed0d8c5

make explicit

016ebed

add test for edge case

11b9b65

Quinn-With-Two-Ns approved these changes Dec 5, 2024

View reviewed changes

Quinn-With-Two-Ns reviewed Dec 5, 2024

View reviewed changes

dandavison reviewed Dec 5, 2024

View reviewed changes

dandavison approved these changes Dec 5, 2024

View reviewed changes

stephanos and others added 2 commits December 5, 2024 16:00

fix typo

ee95802

Merge branch 'master' into uws-new-api

170d9be

stephanos enabled auto-merge (squash) December 6, 2024 00:21

stephanos disabled auto-merge December 6, 2024 00:23

stephanos enabled auto-merge (squash) December 6, 2024 00:29

stephanos merged commit 9ac1af3 into temporalio:master Dec 6, 2024
8 checks passed

stephanos deleted the uws-new-api branch December 6, 2024 00:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Update-with-Start API #2337

New Update-with-Start API #2337

stephanos commented Nov 29, 2024 •

edited

Loading

stephanos Nov 29, 2024

stephanos Nov 29, 2024 •

edited

Loading

Quinn-With-Two-Ns Nov 29, 2024

Quinn-With-Two-Ns Nov 29, 2024

stephanos Nov 29, 2024

stephanos Nov 29, 2024

dandavison Nov 29, 2024

Quinn-With-Two-Ns Nov 30, 2024 •

edited

Loading

stephanos Nov 29, 2024

dandavison commented Nov 29, 2024

stephanos commented Nov 30, 2024 •

edited

Loading

dandavison Nov 29, 2024

dandavison Nov 29, 2024

dandavison Nov 29, 2024

dandavison Nov 29, 2024

Quinn-With-Two-Ns Nov 30, 2024

Quinn-With-Two-Ns Nov 30, 2024

stephanos Nov 30, 2024

Quinn-With-Two-Ns Dec 1, 2024

Quinn-With-Two-Ns commented Dec 2, 2024

stephanos Dec 2, 2024

stephanos Dec 2, 2024 •

edited

Loading

Quinn-With-Two-Ns Dec 2, 2024

stephanos commented Dec 2, 2024 •

edited

Loading

Quinn-With-Two-Ns commented Dec 2, 2024

stephanos commented Dec 2, 2024 •

edited

Loading

Quinn-With-Two-Ns Dec 5, 2024

stephanos Dec 5, 2024

dandavison Dec 5, 2024

dandavison left a comment

New Update-with-Start API #2337

New Update-with-Start API #2337

Conversation

stephanos commented Nov 29, 2024 • edited Loading

What was changed

Choose a reason for hiding this comment

stephanos Nov 29, 2024 • 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

Quinn-With-Two-Ns Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison commented Nov 29, 2024

stephanos commented Nov 30, 2024 • 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

Quinn-With-Two-Ns commented Dec 2, 2024

Choose a reason for hiding this comment

stephanos Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanos commented Dec 2, 2024 • edited Loading

Quinn-With-Two-Ns commented Dec 2, 2024

stephanos commented Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison left a comment

Choose a reason for hiding this comment

stephanos commented Nov 29, 2024 •

edited

Loading

stephanos Nov 29, 2024 •

edited

Loading

Quinn-With-Two-Ns Nov 30, 2024 •

edited

Loading

stephanos commented Nov 30, 2024 •

edited

Loading

stephanos Dec 2, 2024 •

edited

Loading

stephanos commented Dec 2, 2024 •

edited

Loading

stephanos commented Dec 2, 2024 •

edited

Loading