New Update-With-Start API

[dandavison]

New Update-With-Start API.

• Instead of using ExecuteWorkflow, update-with-start is invoked via a new method UpdateWithStartWorkflow on Client which returns an UpdateHandle
• The method receives a WithStartWorkflowOperation struct which defines the start operation, and exposes a method to obtain the workflow run:

The snippet below is from the accompanying sample: temporalio/samples-go#370:

tx := earlyreturn.Transaction{ID: uuid.New(), SourceAccount: "Bob", TargetAccount: "Alice", Amount: 100}

startWorkflowOp := c.NewWithStartWorkflowOperation(client.StartWorkflowOptions{
	ID:                       "early-return-workflow-ID-" + tx.ID,
	WorkflowIDConflictPolicy: enumspb.WORKFLOW_ID_CONFLICT_POLICY_FAIL,
	TaskQueue:                earlyreturn.TaskQueueName,
}, earlyreturn.Workflow, tx)

updateHandle, err := c.UpdateWithStartWorkflow(ctxWithTimeout, client.UpdateWithStartWorkflowOptions{
	UpdateOptions: client.UpdateWorkflowOptions{
		UpdateName:   earlyreturn.UpdateName,
		WaitForStage: client.WorkflowUpdateStageCompleted,
	},
	StartOperation: startWorkflowOp,
})
if err != nil {
	log.Fatalln("Error issuing update-with-start:", err)
}
var earlyReturnResult any
err = updateHandle.Get(ctxWithTimeout, &earlyReturnResult)
if err != nil {
	// The workflow will continue running, cancelling the transaction.

	// NOTE: If the error is retryable, a retry attempt must use a unique workflow ID.
	log.Fatalln("Error obtaining update result:", err)
}

workflowRun, err := startWorkflowOp.Get(ctxWithTimeout)

[cretz]

Suggested change

	NewWithStartWorkflowOperation(options StartWorkflowOptions, workflow interface{}, args ...interface{}) WithStartWorkflowOperation
	NewWithStartWorkflowOperation(options WithStartWorkflowOperationOptions) WithStartWorkflowOperation

Arguably this is more future proof, but understood if we don't want it

[dandavison]

This seems to be going too far to me: it doesn't seem right to have to create a WithStartWorkflowOperationOptions struct in order to create a WithStartWorkflowOperation given that the latter essentially is just a stateful version of the former.

[cretz]

(unresolved since I wanted to add a comment but it was resolved which disables the ability to discuss further)

it doesn't seem right to have to create a WithStartWorkflowOperationOptions struct in order to create a WithStartWorkflowOperation given that the latter essentially is just a stateful version of the former.

Creation methods of stateful things often accept options to create them and it helps us if later we need to add another option. I am ok if we all agree this isn't needed, but accepting an options object when creating something does save us from having a potential differently-named overload or adding mutation methods on the result later. I fear we will add some server-side option that only applies to multiop start workflow and we won't be able to represent it here.

[dandavison]

which disables the ability to discuss further

Apparently not :)

I am ok if we all agree this isn't needed

Indeed, the consensus with others was that it wasn't needed.

I fear we will add some server-side option that only applies to multiop start workflow and we won't be able to represent it here.

We would be able to add such an option to UpdateWithStartWorkflowOptions.

[cretz]

We would be able to add such an option to UpdateWithStartWorkflowOptions.

Until we use this WithStartWorkflowOperation for more than just update with start, which is why I figured it was named to not be related to update. But it's not a big deal.

[dandavison]

I might not be getting what you're saying but if we

add some server-side option that only applies to multiop start workflow

we would at least be able to add as a top-level field here:

type UpdateWithStartWorkflowOptions struct {
	StartWorkflowOperation WithStartWorkflowOperation
	UpdateOptions          UpdateWorkflowOptions
}

[cretz]

Mostly little stuff

[cretz]

(unresolved since I wanted to add a comment but it was resolved which disables the ability to discuss further)

it doesn't seem right to have to create a WithStartWorkflowOperationOptions struct in order to create a WithStartWorkflowOperation given that the latter essentially is just a stateful version of the former.

Creation methods of stateful things often accept options to create them and it helps us if later we need to add another option. I am ok if we all agree this isn't needed, but accepting an options object when creating something does save us from having a potential differently-named overload or adding mutation methods on the result later. I fear we will add some server-side option that only applies to multiop start workflow and we won't be able to represent it here.

[cretz]

Just out of curiosity, which header does this span get put on? Is it both the start request and the update request or just one?

[cretz]

May be worth noting that the interceptor can't do much with this. They can't even know what workflow it applies to. We don't have to do anything now, but I would expect at some point for interceptor authors to ask how to get details of the start the same way they can get details of the update.

[cretz]

This is input for another interceptor call, I don't think we should reuse it for this one. The interceptor input really just needs to be the values passed in by the user. Maybe something like:

type ClientUpdateWithStartWorkflowInput struct {
    StartOptions  *StartWorkflowOptions
    StartWorkflowType string
    StartWorkflowArgs []any
    UpdateID string
    UpdateName string
    UpdateArgs []any
    UpdateWaitForStage WorkflowUpdateStage
}

Or something like:

type ClientUpdateWithStartWorkflowInput struct {
    StartOptions  *StartWorkflowOptions
    StartWorkflowType string
    StartWorkflowArgs []any
    UpdateOptions *UpdateWorkflowOptions
}

Or something like:

type ClientUpdateWithStartWorkflowInput struct {
    UpdateOptions *UpdateWorkflowOptions
    StartOperation WithStartWorkflowOperation
}

and add Options() StartWorkflowOptions and WorkflowType() string and WorkflowArgs() []any to WithStartWorkflowOperation interface.

(this is my only remaining discussion before marking approved, everything else LGTM or is non-blocking)

[dandavison]

What problem are you trying to solve? This is how it's done in Java:

    public WorkflowUpdateWithStartInput(
        WorkflowStartInput workflowStartInput, StartUpdateInput<R> workflowUpdateInput) {

[cretz]

Each input type is only for the outer call being made, it is not reused in places where it may contain non-applicable fields (e.g. ClientUpdateWorkflowInput.RunID which makes no sense here). The whole point is so that we can grow the interceptor as needed for just this call. This is different than our stance on reusing option types for callers.

[Quinn-With-Two-Ns]

I'll note Java does reuse, but I think it is the only SDK that takes such an approach

https://github.com/temporalio/sdk-java/blob/c7fcf12f4fc1965cde8a70feb0a109f198298088/temporal-sdk/src/main/java/io/temporal/common/interceptors/WorkflowClientCallsInterceptor.java#L223

[dandavison]

Ah right I see, thank you @Quinn-With-Two-Ns and @cretz for the suggested schemas. I've switched to

type ClientUpdateWithStartWorkflowInput struct {
	UpdateOptions          *UpdateWorkflowOptions
	StartWorkflowOperation WithStartWorkflowOperation
}

9562e0e

[cretz]

We would be able to add such an option to UpdateWithStartWorkflowOptions.

Until we use this WithStartWorkflowOperation for more than just update with start, which is why I figured it was named to not be related to update. But it's not a big deal.