Add OnConflictOptions to StartWorkflowExecutionRequest

[rodrigozhou]

READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.

What changed?
Add OnConflictOptions to StartWorkflowExecutionRequest

Why?
When the workflow id conflict policy is USE_EXISTING, OnConflictOptions add the ability to change the existing running workflow.

Breaking changes

Server PR
temporalio/temporal#7080

[bergundy]

Approved with a couple of small comments.

[bergundy]

I'd mention that we plan to add options here as needed.

[rodrigozhou]

I'm adding explicitly the behaviors we want to implement at first already, so I think no need to update comment.

[cretz]

If set to non-nil empty object, it will attach the completion callbacks to the existing running workflow

Why not make this an explicit option on the object? The code has to opt-in to this anyways by providing a non-null object, might as well opt-in to which things they want to attach. It seems strange to have some that are attached by default and maybe later some that are affected by options.

[rodrigozhou]

Sure, I'm ok with being explicit. cc: @bergundy

[rodrigozhou]

I've added attach_completion_callbacks and attach_links. I'm not adding attach_request_id because request id is used for deduping, so I think it make sense that if OnConflictOptions is set, then we need to always attach the request id to dedup the request, and not redo work.

[cretz]

I think it make sense that if OnConflictOptions is set, then we need to always attach the request id to dedup the request, and not redo work

Not following the request ID thing. I do not think the presence or absence of this field should change the behavior (i.e. an empty value and a null value should behave the same). It is confusing to just have an empty object do something different.

[rodrigozhou]

The purpose of the request ID is to dedup the request in case the client retries the same request, but the server already processed the request.

If the OnConflictOptions field is not nil, then likely the user wants to do something with the existing running workflow (eg: add callbacks). So, we want to be able to dedup this request in case the client retries the request, ie., we need to attach the request ID to the workflow.

[cretz]

If the OnConflictOptions field is not nil, then likely the user wants to do something with the existing running workflow (eg: add callbacks).

You don't know how the options will grow or be used in the future. IMO we need to require the user explicitly say they want to attach request ID or we need to always attach the request ID on use-existing. To sometimes do it based on null-ness of some options message is folly IMO. In many langs we may always populate this option and just not set any fields (or at least the code for building the proto shouldn't have to understand the advanced nuances of setting an empty proto message vs no message).

[cretz]

Will this message be used anywhere else except start workflow?

[bergundy]

Maybe in schedules?

[cretz]

There is no ID conflict policy in schedules (by intention as I understand it due to how schedules set IDs)

[cretz]

Is this needed for signal-with-start?

[bergundy]

I don't think so, because for signal with start we're thinking that you care more about the signal than the start and we don't plan on attaching the Nexus operation to the workflow's lifecycle.

[cretz]

But this is a general purpose concept not specific to Nexus. In general everywhere a conflict policy is applied it'd make sense to apply conflict options IMO.

[rodrigozhou]

In the case of signal with start, the goal is to signal the workflow if it's already running, not update it.
Supporting OnConflictOptions in this case would mean that the behavior of signal with start would be to update and signal the workflow? Would this make sense?

[cretz]

I think the general idea of this being paired with a conflict policy makes sense for consistency reasons. I think your question could be rephrased as "does conflict policy make sense on signal with start", but since it was decided it does however that was decided, I think these options should go alongside them.

[bergundy]

I don't see a use case for attaching callbacks and links to a running workflow on signal-with-start.
We plan to attach a link on the WorkflowExecutionSignaled event (and probably on the WorkflowExecutionStarted event if the current request started a new workflow). But we do not have plans to attach callbacks on signal-with-start.
I'd rather only add fields that we plan on using.

[cretz]

On conflict options is about what do to on conflict. Whether a use case exists for these first couple of options I don't think affects the fact that everywhere a conflict policy is applied, its conflict options should apply.

[bergundy]

I would still rather add this later. We don't plan on adding server support for this quite yet.

[cretz]

Suggested change

	bool attach_completion_callbacks = 1;
	bool attach_links = 2;
	bool append_completion_callbacks = 1;
	bool append_links = 2;

I think this may be clearer terminology (assuming it is actually "appending")

[bergundy]

This works for me although I prefer the term attach over append but don't have a strong opinion on that.

[rodrigozhou]

For callbacks, "append" makes sense since we adding to the workflow execution.
But for links, this is set in the history event, it's not "appending" an existing list/object.
I'm ok with changing for callbacks, but I'd leave "attach" for links.

[cretz]

I think what each of these do to existing values may need to be documented here because it is clearly confusing. What does "attach" mean for links? If links are set in the history event and immutable, then where in history are these new ones set? Or are there now two kinds of links, start links and post-start links, and they manifest in different ways?

[bergundy]

Agree we should document these options.

The link would essentially be linked to the new event but conceptually they're considered attached to the workflow.

flowchart LR
    CallerWorkflowA --> HandlerWorkflow
    CallerWorkflowB --> HandlerWorkflow

Loading

The callback is added to the list of completion callbacks.

[cretz]

I think it make sense that if OnConflictOptions is set, then we need to always attach the request id to dedup the request, and not redo work

Not following the request ID thing. I do not think the presence or absence of this field should change the behavior (i.e. an empty value and a null value should behave the same). It is confusing to just have an empty object do something different.