Add `WORKFLOW_EXECUTION_OPTIONS_UPDATED` event type and remove FieldMask from `WorkflowExecutionOptionsUpdated` Event #478

carlydf · 2024-11-16T09:07:01Z

Add workflow options updated event type
Remove field mask from WorkflowExecutionOptionsUpdatedEvent and only store resulting options. When memo is added to WorkflowExecutionOptions, we will only store it in this event when it was upserted, so that we don't increase history size by storing memo on every OptionsUpdate. This works because Memo is never nil, only empty or populated, so a nil Memo in the event can mean "no change".
Change comments on ActivityPropertiesModifiedExternallyEventAttributes to indicate it was never used and is not intended to be used in the future.

To implement the UpdateWorkflowExecutionOptions API

Breaking changes

Server PR

…ons field

cretz

LGTM, but I added a note where I think we need to at least clarify some simple rules for SDK on the field mask if we're going to start using field masks in history (sorry I did not see this earlier).

cretz · 2024-11-18T13:48:10Z

temporal/api/history/v1/message.proto

 message WorkflowExecutionOptionsUpdatedEventAttributes {
    // Workflow Execution options that were sent in the UpdateWorkflowExecutionOptions request.
    // Partial updates are accepted and controlled by update_mask.
-    temporal.api.workflow.v1.WorkflowExecutionOptions workflow_execution_options_update = 1;
+    temporal.api.workflow.v1.WorkflowExecutionOptions options = 1;

    // Update mask that was sent in the UpdateWorkflowExecutionOptions request.
-    // Indicates which fields from `workflow_execution_options_for_update` were applied.
+    // Indicates which fields from `options` were applied.
    google.protobuf.FieldMask update_mask = 2;
 }


So I didn't notice or I guess didn't say anything when this first appeared, but processing a field mask may be a bit rough to ask every SDK to do. Is it too much history to provide the full set of options always as they now sit? Or maybe we can just document this field mask here that 1) it will always be set with the updated fields (i.e. even if the user did not set it at RPC time because https://google.aip.dev/134#request-message does not require it), and 2) it will always be a fixed, non-nested field (i.e. never have dots or wildcards or any of that.

hm yeah actually I was considering storing just the resulting options instead of the mask + request options.. I think the biggest thing would maybe be memo when/if we add memo to these? Do you know how big memo can be

it actually would be slightly less repeated work in the server if I store the merged options only and no field..

I don't have a strong preference. For practical reasons, I'd suggest to get rid of mask for now and punt the mask decision to a later time as and if more fields are added. This is so we can move forward with the versioning work in the very tight schedule that we have. (Right now, there is only one field in WorkflowExecutionOptions and this event has to change that only field.)

Correction: It would be hard to support old histories if we add the mask later. So we should add it now if we think it's a good thing. I do thing the mask is valuable because the UI (for example) can clearly show what options did change in that particular event.

Do you guys think it's ok if I want to say that asterisk and empty field mask is not supported by my UpdateWorkflowExecutionOptions RPC (contrary to https://google.aip.dev/134#request-message guidelines), or should I implement the guidelines?

@antlai-temporal and I are proposing this for the Go SDK:

["versioning_behavior_override"] -> invalid ["foo"] -> invalid ["*"] -> invalid [] -> valid, means "PUT nothing, GET the full current options" ["VersioningBehaviorOverride"] -> valid ["VersioningBehaviorOverride.Behavior"] -> invalid ["VersioningBehaviorOverride.anything"] -> invalid

The RPC will differ slightly in that:

["versioning_behavior_override"] -> valid ["VersioningBehaviorOverride"] -> invalid

My reason for not supporting * is above, also I think there is not much benefit, since it would be rare that a user wanted to update memo AND priority AND versioning behavior AND timeouts all at once.

My reason for not supporting dot notation / partial updates, is that it becomes fraught when the struct being partially-updated can have field-combinations that are invalid, and maybe two callers are updating it and don't realize that one person just changed it. The structs are simple enough that I don't see a convincing use case for supporting partial struct updates and adding a lot of complexity.

I think an empty field to update everything is really dangerous.

We're on a thread about the history field here but I suppose we're also talking about the RPC.

I disagree. People need to be able to put the whole object by default and patch non-default. I believe this is what's happening with activity options field mask use and what https://google.aip.dev/134#request-message says to follow (where the concept of using field masks basically came from). If you want the SDK API to disallow this, that's fine, but people expect to be able to opt-in to partial updates with field masks, not out. The only thing that could maybe make sense is failing on empty field mask. But turning an update into a getter with an empty field mask does not make sense IMO and may end up making us inconsistent with ourselves and others expected use of field masks in gRPC.

proposing this for the Go SDK

This doesn't seem to be related to Go SDK

Also, the field names in the field mask need to match exactly what they are in proto and need to be the same for RPC and history (granted the latter should disallow special chars and such, but still should reference actual proto fields). So it should be versioning_behavior_override in both.

Oh, I think we are interpreting this differently:

The field must be optional, and the service must treat an omitted field mask as an implied field mask equivalent to all fields that are populated (have a non-empty value).

My interpretation of that rule is that an "omitted field mask" is different from an "empty field mask". Where "omitted field mask" means update_mask == nil and "empty field mask" means update_mask.GetPaths() = []string{}

Apologies for jumping between the history event question and the RPC question, I'm just in the middle of implementing the RPC and want to decide what to accept there.

Makes sense, I was getting omitted vs empty mixed up too. IMO from an RPC POV, "omitted field mask" should be same as "full field mask", and "empty field mask" should probably be an error (there's no value in supporting this that I can think of). IMO from a history POV, the field mask should always be present and only have full field names with no wildcards or nesting (or we should decide that we don't want to take this field mask approach for history).

Also, if you wouldn't mind, can you get confirmation at least from @yiminc and @dnr that they are ok with this Google field mask structure inside history?

…t used' on WorkflowPropertiesModifiedExternallyEvent

cretz · 2024-11-19T23:19:41Z

temporal/api/history/v1/message.proto

 }

+// Not used anywhere. Use case is replaced by WorkflowExecutionOptionsUpdatedEventAttributes
 message WorkflowPropertiesModifiedExternallyEventAttributes {


I see we decided to not reuse this event and add your field to the bottom. Works for me (and I agree we can't really delete this), though would have also been ok just adding your field. Unsure if others have opinions here or not.

antlai-temporal · 2024-11-19T23:24:33Z

temporal/api/history/v1/message.proto

-    // Indicates which fields from `workflow_execution_options_for_update` were applied.
-    google.protobuf.FieldMask update_mask = 2;
+    // Versioning override in the mutable state after event has been applied.
+    temporal.api.common.v1.VersioningBehaviorOverride versioning_override = 1;


This is moving to common.v1.VersioningOverride as discussed. Do you want to include that change in this PR?

Shahab has an open PR with that change, so I'm going to let it happen there to reduce churn / merge issues

…atewfevent

add workflow options updated event type and rename history event opti…

c53d5b9

…ons field

carlydf requested review from a team as code owners November 16, 2024 09:07

cretz reviewed Nov 18, 2024

View reviewed changes

Don't store mask in WorkflowExecutionOptionsUpdatedEvent, comment 'No…

739f1b7

…t used' on WorkflowPropertiesModifiedExternallyEvent

cretz approved these changes Nov 19, 2024

View reviewed changes

antlai-temporal reviewed Nov 19, 2024

View reviewed changes

antlai-temporal approved these changes Nov 19, 2024

View reviewed changes

carlydf changed the title ~~Add workflow options updated event type and rename history event options field~~ Add WORKFLOW_EXECUTION_OPTIONS_UPDATED event type and remove FieldMask from WorkflowExecutionOptionsUpdated Event Nov 19, 2024

Merge branch 'versioning-3' of github.com:temporalio/api into cdf/upd…

fd8887c

…atewfevent

carlydf merged commit 21d7a2f into versioning-3 Nov 20, 2024
1 check failed

carlydf deleted the cdf/updatewfevent branch November 20, 2024 00:01

Add WORKFLOW_EXECUTION_OPTIONS_UPDATED event type and remove FieldMask from WorkflowExecutionOptionsUpdated Event #478

Add WORKFLOW_EXECUTION_OPTIONS_UPDATED event type and remove FieldMask from WorkflowExecutionOptionsUpdated Event #478

Uh oh!

Conversation

carlydf commented Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

cretz Nov 18, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

ShahabT Nov 18, 2024 • 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!

carlydf Nov 18, 2024 • 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!

cretz Nov 19, 2024 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Add `WORKFLOW_EXECUTION_OPTIONS_UPDATED` event type and remove FieldMask from `WorkflowExecutionOptionsUpdated` Event #478

Add `WORKFLOW_EXECUTION_OPTIONS_UPDATED` event type and remove FieldMask from `WorkflowExecutionOptionsUpdated` Event #478

carlydf commented Nov 16, 2024 •

edited

Loading

cretz Nov 18, 2024 •

edited

Loading

ShahabT Nov 18, 2024 •

edited

Loading

carlydf Nov 18, 2024 •

edited

Loading

cretz Nov 19, 2024 •

edited

Loading