Add OnConflictOptions to StartWorkflowExecution by rodrigozhou · Pull Request #7080 · temporalio/temporal

rodrigozhou · 2025-01-14T19:09:18Z

What changed?

Add OnConflictOptions to StartWorkflowExecution.

if the workflow id conflict policy is not USE_EXISTING, then no-op;
if the field is nil, then no-op;
otherwise, then it will add a WorkflowExecutionOptionsUpdatedEvent to the existing running workflow.

Why?

Ability to attach completion callbacks to existing running workflows.

How did you test it?

WIP: writing tests

Potential risks

Documentation

Is hotfix candidate?

bergundy

I want to see functional test coverage for as many of the possible cases. I don't feel comfortable merging this otherwise.
Some tests off the top of my head:

Test that the event is created as expected with links, callbacks, and request ID attached.
Test that callbacks appear in the describe response.
Test request ID based deduping works for the attached request ID

bergundy · 2025-01-15T18:31:24Z

common/persistence/data_interfaces.go

+		RequestID          string
+		RunID              string
+		State              enumsspb.WorkflowExecutionState
+		Status             enumspb.WorkflowExecutionStatus
+		LastWriteVersion   int64
+		StartTime          *time.Time
+		AttachedRequestIDs []string


Please document the difference between the RequestID and AttachedRequestIDs fields and when to use either.

bergundy · 2025-01-15T18:35:39Z

go.mod

 	go.opentelemetry.io/otel/sdk/metric v1.31.0
 	go.opentelemetry.io/otel/trace v1.31.0
-	go.temporal.io/api v1.43.1-0.20241219191931-461db5a23056
+	go.temporal.io/api v1.43.2-0.20250115015852-6bdd96365d41


We should merge the API PR before this server PR.

proto/internal/temporal/server/api/persistence/v1/executions.proto

bergundy · 2025-01-15T19:00:35Z

service/frontend/workflow_handler.go

+	if err := wh.validateOnConflictOptions(request.OnConflictOptions); err != nil {
+		return nil, err
+	}


Redundant for now but 🤷

Yes, I know is unnecessary for now. Just being preemptive adding the function to validate.

service/history/history_engine2_test.go

bergundy · 2025-01-15T19:54:31Z

service/history/historybuilder/event_factory.go

 	versioningOverride *workflowpb.VersioningOverride,
+	attachRequestID string,
+	attachCompletionCallbacks []*commonpb.Callback,
+	links []*commonpb.Link,


Maybe instead accept a historypb.WorkflowExecutionOptionsUpdatedEventAttributes and links here to avoid this getting out of control?

Hm, I think it would make sense to use historyservice.UpdateWorkflowExecutionOptionsRequest, but it would add some logic in the event factory (do the mask processing here), which I'm not sure it makes sense.
I'd leave as it is for now, and refactor later. I'm already planning to refactor the startworkflow api to call the updateworkflowwithoptions api in which I can address this issue together.

bergundy · 2025-01-15T19:54:58Z

service/history/historybuilder/history_builder.go

 	versioningOverride *workflowpb.VersioningOverride,
+	attachRequestID string,
+	attachCompletionCallbacks []*commonpb.Callback,


Same as above, probably better to accept the attributes here.

bergundy · 2025-01-15T19:55:24Z

service/history/workflow/mutable_state.go

+			versioningOverride *workflowpb.VersioningOverride,
+			attachRequestID string,
+			attachCompletionCallbacks []*commonpb.Callback,


Same, accept the event attributes here instead.

service/history/workflow/mutable_state_impl.go

rodrigozhou · 2025-01-27T18:51:00Z

common/persistence/serialization/blob.go

+func WorkflowExecutionRequestIDsToBlob(requestIDs *persistencespb.WorkflowExecutionRequestIDs) (*commonpb.DataBlob, error) {
+	return proto3Encode(requestIDs)
+}
+
+func WorkflowExecutionRequestIDsFromBlob(blob []byte, encoding string) (*persistencespb.WorkflowExecutionRequestIDs, error) {
+	if blob == nil {
+		return nil, nil
+	}
+	result := &persistencespb.WorkflowExecutionRequestIDs{}
+	return result, proto3Decode(blob, encoding, result)
+}


This is code duplication for now (from the serializer.go file).
We need to refactor the code to pass the serializer object to the DB plugins to be able to serialize/deserialize fields from DB.

Will you do the refactor? Not blocking this PR, but ideally we'd at least track this.

proto/internal/temporal/server/api/persistence/v1/executions.proto

bergundy

Note that we also need to ensure that the WorkflowExecutionOptionsUpdated event is cherry picked (reapplied) on conflict resolution.

bergundy · 2025-01-27T21:19:59Z

common/persistence/cassandra/mutable_state_store.go

+		executionStateDatablob, err := serialization.WorkflowExecutionStateToBlob(
+			&persistencespb.WorkflowExecutionState{
+				RunId:           runID,
+				CreateRequestId: createRequestID,


Should we stop storing this now that we have the map?

I'd leave for a while first to transition to use the new field.

bergundy · 2025-01-27T21:24:04Z

common/persistence/data_interfaces.go

@@ -122,13 +122,18 @@ type (

 	// CurrentWorkflowConditionFailedError represents a failed conditional update for current workflow record
 	CurrentWorkflowConditionFailedError struct {


Should we just return the entire *persistencespb.WorkflowExecutionState object here?

Hm, yes, we can do that, but it's kind of a big refactor. I can do this in another PR.

Sounds good.

common/persistence/data_interfaces.go

common/persistence/sql/sqlplugin/history_execution.go

bergundy · 2025-01-27T21:30:13Z

common/testing/testvars/test_vars.go

 	"time"

-	"github.com/pborman/uuid"
+	"github.com/google/uuid"


Thanks! I've been wanting to get rid of this dependency and forbid it in our linter but haven't gotten around to it yet.

proto/internal/temporal/server/api/enums/v1/workflow.proto

proto/internal/temporal/server/api/persistence/v1/executions.proto

bergundy · 2025-01-27T22:02:46Z

service/history/historybuilder/event_factory.go

 	versioningOverride *workflowpb.VersioningOverride,
 	unsetVersioningOverride bool,
+	attachRequestID string,
+	attachCompletionCallbacks []*commonpb.Callback,


Still think you should just accept *historypb.WorkflowExecutionOptionsUpdatedEventAttributes here and avoid this function signature growing every time we add another attribute.

It's a bit complicated. This is the event factory, ie., that's the function that builds *historypb.WorkflowExecutionOptionsUpdatedEventAttributes. I want to change to get the history request, but then it would add some logic to translate the request input to event attributes, which is not something we do so far.

You should be able to accept the attributes in the event factory, not blocking this PR though.

service/history/workflow/mutable_state_impl.go

bergundy · 2025-01-27T22:06:09Z

service/history/api/startworkflow/api.go

+		err := api.ResolveWorkflowIDReusePolicy(
+			workflowKey,
+			currentWorkflowConditionFailed.Status,
+			currentWorkflowConditionFailed.RequestID,
+			s.request.StartRequest.GetWorkflowIdReusePolicy(),
+		)


Is currentWorkflowConditionFailed.RequestID the current request's ID? Should we be checking that the request IDs in the DB and request matches? (asking for lack of context on this part of the code).

currentWorkflowConditionFailed.RequestID is the existing current workflow, which can be running or completed already.

common/persistence/cassandra/mutable_state_store.go

rodrigozhou · 2025-01-29T02:43:28Z

common/testing/historyrequire/history_require.go

This is a fix in the history event attributes assertion.
If the field is expected to be nil, the assertion below at L527 will fail because actualV is something like var actualV any = (*T)(nil), and this is different than var expectedValue any = nil.

service/history/workflow/mutable_state_impl.go

common/persistence/cassandra/mutable_state_store.go

common/persistence/serialization/serializer.go

proto/internal/temporal/server/api/persistence/v1/executions.proto

service/history/workflow/mutable_state_impl.go

service/history/api/startworkflow/api.go

bergundy

Overall LGTM but I'm missing a few test cases:

Start without OnConflictOptions and same different request ID fails (for good measure)
Start fails due to validation (e.g. max callback limit reached), does not attach a request ID to the current executions table (e.g. a following request doesn't get duduped).
XDC test and/or reset test that verifies the update event is applied and start requests get properly deduped

common/persistence/data_interfaces.go

service/history/api/startworkflow/api.go

tests/workflow_test.go

yycptt · 2025-01-31T22:43:12Z

common/persistence/sql/execution_util.go

shall we change other places that uses current execution row to use the blob instead? e.g. GetCurrentExecution in sql/execution.go.

we still do double write so doesn't really matter right now.

I thought about the same, but I'll check and see if it's gonna be an improvement or not in another PR.

rodrigozhou · 2025-02-03T18:00:06Z

XDC test and/or reset test that verifies the update event is applied and start requests get properly deduped

@bergundy I created a separate PR that reapplies the WorkflowExecutionOptionsUpdated history event: #7221

_**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

rodrigozhou requested review from a team, bergundy and yycptt January 14, 2025 19:09

rodrigozhou mentioned this pull request Jan 14, 2025

Add OnConflictOptions to StartWorkflowExecutionRequest temporalio/api#510

Merged

rodrigozhou force-pushed the rodrigozhou/on-conflict-options branch from b3cd07b to 74c0732 Compare January 14, 2025 19:55

rodrigozhou force-pushed the rodrigozhou/refactor branch from 41335d0 to 3a5a6f6 Compare January 15, 2025 17:42

rodrigozhou force-pushed the rodrigozhou/on-conflict-options branch 2 times, most recently from d2b40ac to b7ed8a7 Compare January 15, 2025 18:11

bergundy reviewed Jan 15, 2025

View reviewed changes

rodrigozhou force-pushed the rodrigozhou/on-conflict-options branch from b7ed8a7 to 33229ff Compare January 27, 2025 18:17

rodrigozhou force-pushed the rodrigozhou/refactor branch from 3a5a6f6 to 9031420 Compare January 27, 2025 18:29

rodrigozhou commented Jan 27, 2025

View reviewed changes

proto/internal/temporal/server/api/persistence/v1/executions.proto Outdated Show resolved Hide resolved

rodrigozhou commented Jan 27, 2025

View reviewed changes

proto/internal/temporal/server/api/persistence/v1/executions.proto Outdated Show resolved Hide resolved

Base automatically changed from rodrigozhou/refactor to main January 27, 2025 21:31

bergundy reviewed Jan 27, 2025

View reviewed changes

rodrigozhou force-pushed the rodrigozhou/on-conflict-options branch from 33229ff to 51e80d1 Compare January 29, 2025 00:27

rodrigozhou requested a review from bergundy January 29, 2025 00:27

rodrigozhou force-pushed the rodrigozhou/on-conflict-options branch from 51e80d1 to 4c09e73 Compare January 29, 2025 02:33

rodrigozhou commented Jan 29, 2025

View reviewed changes

yycptt reviewed Jan 30, 2025

View reviewed changes

rodrigozhou force-pushed the rodrigozhou/on-conflict-options branch 2 times, most recently from abcee87 to 7ff6845 Compare January 31, 2025 01:11

bergundy reviewed Jan 31, 2025

View reviewed changes

yycptt approved these changes Jan 31, 2025

View reviewed changes

rodrigozhou force-pushed the rodrigozhou/on-conflict-options branch from 7ff6845 to 46835e2 Compare February 3, 2025 17:56

bergundy approved these changes Feb 3, 2025

View reviewed changes

rodrigozhou force-pushed the rodrigozhou/on-conflict-options branch from 9c3304b to 924f1d9 Compare February 3, 2025 20:38

rodrigozhou added 10 commits February 3, 2025 12:38

Add OnConflictOptions to StartWorkflowExecution

8bced3e

address comments

9c42653

add functional test

a9a91f3

address comments

61466ba

address comments

cac6747

add functional test

fd0cd20

address comments

5f344ab

address comments

3f0ae4f

address comments

f195ef7

update go.mod

d01c49a

rodrigozhou force-pushed the rodrigozhou/on-conflict-options branch from 924f1d9 to d01c49a Compare February 3, 2025 20:39

rodrigozhou merged commit 72c6218 into main Feb 3, 2025

rodrigozhou deleted the rodrigozhou/on-conflict-options branch February 3, 2025 21:53

danieljoos mentioned this pull request Feb 24, 2025

error converting NULL to string is unsupported after upgrade to v1.27.0 #7376

Closed

		@@ -122,13 +122,18 @@ type (

		// CurrentWorkflowConditionFailedError represents a failed conditional update for current workflow record
		CurrentWorkflowConditionFailedError struct {

Conversation

rodrigozhou commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed?

Why?

How did you test it?

Potential risks

Documentation

Is hotfix candidate?

Uh oh!

bergundy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rodrigozhou Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bergundy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodrigozhou Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rodrigozhou commented Jan 14, 2025 •

edited

Loading

rodrigozhou Jan 27, 2025 •

edited

Loading

rodrigozhou Jan 28, 2025 •

edited

Loading