Workflow update #142

cretz · 2023-10-18T14:56:36Z

What was changed

Add [WorkflowUpdate] and [WorkflowUpdateValidator] attributes
Support for workflow update in workflows including dynamic workflow update
Support for workflow update in client including WorkflowUpdateHandle and ExecuteWorkflowUpdateAsync extension that is just StartWorkflowUpdateAsync + GetResultAsync

Checklist

Closes [dotnet] Workflow update support #100

Fixes temporalio#100

src/Temporalio/Client/WorkflowHandleExtensions.cs

src/Temporalio/Client/WorkflowUpdateOptions.cs

src/Temporalio/Worker/WorkflowInstance.cs

bergundy

I didn't finish the entire review, I'll try to continue this tomorrow.

README.md

bergundy · 2023-10-18T16:24:00Z

README.md

-await handle.SignalWorkflowAsync(wf => wf.UpdateGreetingParamsAsync(signalArg));
+// Change the params via update
+var updateArg = new GreetingParams(Salutation: "Aloha", Name: "John");
+await handle.ExecuteUpdateAsync(wf => wf.UpdateGreetingParamsAsync(updateArg));


Worth showing that this can return a value.

I don't think that's necessary in this README, I think that can happen when this is properly documented. Note I am not showing you can set an ID either.

It's worth it IMHO, otherwise it's unclear how this is different from a signal. Of course it'll be properly documented later but for a Temporal noob this would be a bit confusing.

Agreed. No reason not to show it, and at least this reason to show it.

I think it's reading too much into tiny sample (aren't they gonna get confused w/ update vs query? what about showing that queries can accept parameters? what about showing validators?), but I will add it.

Hrmm, I'm struggling to fit the update-with-return value use case into the above sample. Any ideas on what I can change the GreetingWorkflow's UpdateGreetingParamsAsync to for demonstrating a return value?

(@Sushisource - you'll find the same challenge when you update the Python README)

src/Temporalio/Client/TemporalClient.Workflow.cs

src/Temporalio/Client/WorkflowHandleExtensions.cs

src/Temporalio/Client/WorkflowUpdateOptions.cs

src/Temporalio/Worker/WorkflowInstance.cs

cretz · 2023-10-19T13:51:49Z

Updates made based on team discussions

README.md

src/Temporalio/Client/WorkflowHandleExtensions.cs

src/Temporalio/Client/TemporalClient.Workflow.cs

Sushisource

Looking good! Just some minor stuff.

src/Temporalio/Client/WorkflowHandle.cs

Sushisource · 2023-10-20T16:52:12Z

src/Temporalio/Client/WorkflowUpdateHandle.cs

+        /// <param name="rpcOptions">Extra RPC options.</param>
+        /// <returns>Completed update task.</returns>
+        /// <remarks>WARNING: Workflow update is experimental and APIs may change.</remarks>
+        public Task GetResultAsync(TimeSpan? timeout = null, RpcOptions? rpcOptions = null) =>


I'm a bit inclined to say this should actually have a different name if it disregards the result. Like WaitForCompletedAsync.

But, ignoring that - why even bother with this separately. Can't someone just use the result-returning version and just not use the result?

I'm a bit inclined to say this should actually have a different name if it disregards the result. Like WaitForCompletedAsync.

This matches the workflow get result (and we tend to kinda call it that for things that can optionally return results like workflows and activities). Users may get confused that the presence of a return value changes the function name since other than return value they are the same.

But, ignoring that - why even bother with this separately. Can't someone just use the result-returning version and just not use the result?

In .NET Task and Task<Result> are distinctly defined and there is no "unit" or "void" type for these purposes in .NET. So every .NET call (e.g. Task.Run) has to differentiate.

Aaah, ok, makes sense.

Sushisource · 2023-10-20T16:53:50Z

src/Temporalio/Worker/Interceptors/WorkflowInboundInterceptor.cs

+        /// Intercept update validation. This is invoked every time validation is needed regardless
+        /// of whether a validator was defined.
+        /// </summary>
+        /// <param name="input">Input details of the call.</param>
+        public virtual void ValidateUpdate(HandleUpdateInput input) => Next.ValidateUpdate(input);


In my implementation I only invoke if the validator is defined. I can see doing it always, if you want some blanket way to reject based on... whatever... without defining a validator for every update. I'll change that in my impl.

👍 Yeah I went back and forth here too but I figured "why not"

Ok, after team discussion, here is what we want the SDK logic to be due to possible mutation of input and performance:

if core determines validation should occur

If validator present

decode args for validator (or reject on fail)

call validator (interceptor) w/ those args

decode args for update (or reject on fail)

accept

else

decode args for update (or task-fail on fail)

accept

call updator (interceptor) w/ those args

The logic has been updated to support this approach

Sorry to beat a dead horse, but I thought if we have to decode the args we would go through the interceptor

because it is a non trivial change in go and java to even know if a validator is present or not without going through the interceptor

Sorry to beat a dead horse
No problem, we need to get this right/consistent. Happy to discuss as much as is necessary.

So double-decoding args has already been settled to prevent validator mutability. We will discuss as a team whether to prevent interceptor call if validator not present.

Ok, after discussion, the logic in the bullets above is what we want. Don't run validate interceptor (and therefore don't decode args for it) if a validator is not present

Sushisource · 2023-10-20T16:55:27Z

src/Temporalio/Worker/WorkflowInstance.cs

+                        // Capture command count so we can ensure it is not altered during validation
+                        var origCmdCount = completion?.Successful?.Commands?.Count ?? 0;
+                        inbound.Value.ValidateUpdate(updateInput);


Seems this would be useful to put behind some kind of "read only mode" protector like exists in Python / Java?

Yeah, this was only the second time I needed such a thing and the error message is different enough and impl simpl;e enough that I didn't make reuseable (if .NET had simple yield-based contextmanager type thing like Python, definitely).

dandavison · 2023-10-21T15:55:42Z

src/Temporalio/Worker/WorkflowInstance.cs

+                    dynamic: updateDefn.Dynamic,
+                    dynamicArgPrepend: update.Name);
+
+                // Do validation. Whether or not this runs a validator, this should accept/reject.


Suggested change

// Do validation. Whether or not this runs a validator, this should accept/reject.

// Do validation. Whether or not this runs a validator, this must accept/reject.

I think you're overthinking the terms used in simple internal English comments as if they're an RFC heh

dandavison · 2023-10-21T20:25:49Z

src/Temporalio/Worker/WorkflowInstance.cs

+
+                        // We want to try to decode args here _inside_ the validator rejection
+                        // try/catch so we can prevent acceptance on invalid args
+                        argsForUpdate = DecodeUpdateArgs();


If you move this down a couple of lines to outside the if (runValidator), then it will be inside the validator rejection try/catch, but it will always run. Then you can eliminate the other call below in the handler section.

But I don't want to catch argument decoding failure validator-style w/ rejection if run validator is false. If run validator is false, I don't want to decode until later, where the failure is a task failure not a rejection.

dandavison · 2023-10-21T20:30:09Z

src/Temporalio/Worker/WorkflowInstance.cs

+                {
+                    // If the args were not already decoded because we didn't run the validation
+                    // step, decode them here
+                    argsForUpdate ??= DecodeUpdateArgs();


As mentioned above, you can eliminate this line by always decoding inside the validation try above. (Just a clean up -- I know the way you have it it won't be called more than necessary.)

I don't want to decode in the try above, I want this non-failure-exception to throw out of this method and therefore fail the task.

dandavison · 2023-10-21T20:42:04Z

src/Temporalio/Worker/WorkflowInstance.cs

+                        },
+                    });
+                    return Task.CompletedTask;
+                }


To check my understanding, exceptions other than Temporal FailureException here bubble up and fail the workflow task, correct?

Correct. So we only reject/fail update if there is an exception when run-validator is true or there is a failure exception. Otherwise we fail the task.

dandavison · 2023-10-22T07:16:40Z

src/Temporalio/Worker/WorkflowInstance.cs

+                // Do validation. Whether or not this runs a validator, this should accept/reject.
+                try
+                {
+                    if (update.RunValidator)


Go and Java do not run validation when replaying. Do we need to put the same check in place here?

Core is doing that check for you and setting this boolean

Ah-ha. I had not understood that, thanks.

Workflow update support

dbd3a5d

Fixes temporalio#100

cretz requested a review from a team October 18, 2023 14:56

cretz commented Oct 18, 2023

View reviewed changes

src/Temporalio/Client/WorkflowHandleExtensions.cs Outdated Show resolved Hide resolved

cretz commented Oct 18, 2023

View reviewed changes

src/Temporalio/Client/WorkflowUpdateOptions.cs Outdated Show resolved Hide resolved

cretz commented Oct 18, 2023

View reviewed changes

src/Temporalio/Worker/WorkflowInstance.cs Show resolved Hide resolved

bergundy reviewed Oct 19, 2023

View reviewed changes

Updates based on team discussions about updates

a0e5587

dandavison reviewed Oct 19, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

src/Temporalio/Client/WorkflowHandleExtensions.cs Outdated Show resolved Hide resolved

src/Temporalio/Client/TemporalClient.Workflow.cs Show resolved Hide resolved

Minor suggested changes

7c8154f

cretz force-pushed the workflow-update branch from a1817d8 to 7c8154f Compare October 19, 2023 19:25

cretz requested a review from a team October 19, 2023 19:36

bergundy approved these changes Oct 20, 2023

View reviewed changes

Sushisource approved these changes Oct 20, 2023

View reviewed changes

Rework validation arg-decoding/calling/interceptor logic

e87695d

dandavison reviewed Oct 23, 2023

View reviewed changes

cretz mentioned this pull request Oct 23, 2023

Workflow Update temporalio/sdk-python#400

Merged

cretz added 2 commits October 25, 2023 16:04

Merge branch 'main' into workflow-update

c4b3d1e

Update core

3e84bbe

cretz merged commit ef02df3 into temporalio:main Oct 25, 2023
7 checks passed

cretz deleted the workflow-update branch October 25, 2023 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workflow update #142

Workflow update #142

cretz commented Oct 18, 2023

bergundy left a comment

bergundy Oct 18, 2023

cretz Oct 19, 2023

bergundy Oct 20, 2023

Sushisource Oct 20, 2023

cretz Oct 20, 2023 •

edited

cretz Oct 20, 2023 •

edited

cretz commented Oct 19, 2023

Sushisource left a comment

Sushisource Oct 20, 2023

cretz Oct 20, 2023

Sushisource Oct 20, 2023

Sushisource Oct 20, 2023

cretz Oct 20, 2023

cretz Oct 20, 2023 •

edited

cretz Oct 20, 2023

Quinn-With-Two-Ns Oct 20, 2023

Quinn-With-Two-Ns Oct 20, 2023

cretz Oct 23, 2023 •

edited

cretz Oct 23, 2023

Sushisource Oct 20, 2023

cretz Oct 20, 2023

dandavison Oct 21, 2023

cretz Oct 23, 2023

dandavison Oct 21, 2023

cretz Oct 23, 2023

dandavison Oct 21, 2023

cretz Oct 23, 2023

dandavison Oct 21, 2023

cretz Oct 23, 2023

dandavison Oct 22, 2023

cretz Oct 23, 2023

dandavison Oct 23, 2023

	// Do validation. Whether or not this runs a validator, this should accept/reject.
	// Do validation. Whether or not this runs a validator, this must accept/reject.

Workflow update #142

Workflow update #142

Conversation

cretz commented Oct 18, 2023

What was changed

Checklist

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

cretz Oct 20, 2023 • edited

Choose a reason for hiding this comment

cretz Oct 20, 2023 • edited

Choose a reason for hiding this comment

cretz commented Oct 19, 2023

Sushisource 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

cretz Oct 20, 2023 • edited

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

cretz Oct 23, 2023 • edited

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

cretz Oct 20, 2023 •

edited

cretz Oct 20, 2023 •

edited

cretz Oct 20, 2023 •

edited

cretz Oct 23, 2023 •

edited