Add property/mutable state updating events #196

Sushisource · 2022-07-06T23:56:11Z

What changed?
Added new events that we want to use to make changes to mutable state

Also introduces a new top-level field to history event which indicates that the SDK may ignore an event. This is immediately useful for the ActivityPropertiesModifiedEvent, and also gives us a non-breaking upgrade path for any such events in the future which will function with SDKs that do not know how to interpret the new event, but do know it is safe to ignore.

Why?
Support new features

How did you test it?
:|

Potential risks
SDKs who are not updated to include these new protos and the corresponding logic will break if they encounter histories containing these events.

temporal/api/history/v1/message.proto

bergundy

This all LGTM.
I'd add the workflowservice APIs to see if we can embed these new events or reuse the definitions somehow.

cretz · 2022-07-07T13:03:59Z

temporal/api/history/v1/message.proto

+    // If set, update the workflow run timeout to this value.
+    google.protobuf.Duration new_workflow_run_timeout = 3 [(gogoproto.stdduration) = true];
+    // If set, update the workflow execution timeout to this value.
+    google.protobuf.Duration new_workflow_execution_timeout = 4 [(gogoproto.stdduration) = true];


How can I set this back to "unlimited"?

0 would suffice. I'll add it to the comments.

If the value is 0, does it mean set the execution timeout to unlimited or don't change the timeout?

0 means unlimited. Don't change would mean the field is unset entirely.

Good point, for all of these we need a way to differentiate between unlimited and unset.

There’s no way to differentiate unset from 0 (the default) in go

Hmmm.. maybe because it’s duration it’s nilable

yeah, duration is nilable but sooner or later we will face same problem with int or string. There are these guys but they are inconvenient to use.

How will handler of this event look like? Will there be bunch of if for nil/zero value for every field? Is it scalable?

I don't see what other way you're going to do it? Even if Go had proper option types or whatever... you'd still want to check all of them.

This strikes me as not a big deal. For future int/string type things where you need to be able to un-set it, we can easily just make a new message to make it nillable.

cretz · 2022-07-07T13:10:24Z

temporal/api/history/v1/message.proto

+    // information in any way that the SDK need be concerned with. If an SDK encounters an event
+    // type which it does not understand, it must error unless this is true. If it is true, it's
+    // acceptable for the event type and/or attributes to be uninterpretable.
+    bool sdk_may_ignore = 300;


While I understand the future purpose for this value and why we should get it in here, can you confirm it has no value yet? Or are we planning on setting this to true for the properties modified event attributes?

Also, do we want to specify how an SDK reacts to half understanding of the event? Meaning, what if it understands the current modified properties but not the later ones? So if you added new_heartbeat_timeout to ActivityPropertiesModifiedEventAttributes an an event came across with both that and new_retry_policy, should the SDK still handle the one field it understands? (this can be SDK-specific behavior, but worth discussing)

Finally, can we name this something else besides using "sdk" in the name? Maybe bool nonessential?

It's not nonessential to everything - the field is indeed SDK specific. It seems important to include that in the name.

It already applies to ActivityPropertiesModifiedEventAttributes with the one existing field. SDK does not care about that when processing workflow history, as activity info comes from the activity tasks. Same would apply to heartbeat timeout.

To generally address the question, yes, this would certainly be false if any individual field is non-ignorable.

the field is indeed SDK specific

Maybe "worker_may_ignore" or something like that? "sdk" just feels icky to put into history. Obviously just a naming thing.

To generally address the question, yes, this would certainly be false if any individual field is non-ignorable.

Specifically, is it ok if the SDK only knows about some of the modified properties and applies those? Or should the SDK apply none if it doesn't recognize them all?

+1 to worker_may_ignore.

Specifically, is it ok if the SDK only knows about some of the modified properties and applies those? Or should the SDK apply none if it doesn't recognize them all?

Good point, this will break if we add new fields to an event the SDK recognizes unless the SDK's proto library supports unkown fields.

https://developers.google.com/protocol-buffers/docs/proto3#unknowns
tokio-rs/prost#2

W.R.T this behavior, things are no different than any other thing anywhere else in the proto APIs right now. I think the SDK should attempt to apply everything as normal as long as it can interpret the event, even if maybe there are new fields it doesn't understand.

Getting an unspecified event type, or a oneof variant it does not understand, OTOH, should cause it to fail.

I renamed the field

While it's true that we don't check for unknown fields today, and probably shouldn't, I think an update-properties event may be a special case where we should. But I don't feel that strongly about it.

temporal/api/history/v1/message.proto

yycptt · 2022-07-11T05:16:31Z

temporal/api/history/v1/message.proto

+    // If set, update the workflow run timeout to this value.
+    google.protobuf.Duration new_workflow_run_timeout = 3 [(gogoproto.stdduration) = true];
+    // If set, update the workflow execution timeout to this value.
+    google.protobuf.Duration new_workflow_execution_timeout = 4 [(gogoproto.stdduration) = true];


If the value is 0, does it mean set the execution timeout to unlimited or don't change the timeout?

yycptt · 2022-07-11T05:17:12Z

temporal/api/history/v1/message.proto

@@ -634,6 +634,26 @@ message ChildWorkflowExecutionTerminatedEventAttributes {
    int64 started_event_id = 5;
 }

+message WorkflowPropertiesModifiedEventAttributes {


I think we may also need new cron schedule and new workflow memo here.

I'd add these as required, for cron schedule I'm not sure if we're going to keep this feature or deprecate it now that we have scheduled workflows.

I'd add these as required

What do you mean?

I meant as we get requests for those but @yycptt already mentions that there are plans to support updating a memo.

Not sure about cron schedule though

Added memo.

Note, there is also a want for a command for updating memo from inside the workflow akin to how search attribute upsert works today. I figure a general purpose "update workflow properties" command with just that value for now makes sense. We can do it in a separate PR, but I am just hoping it is generic and we don't make a memo-specific command.

alexshtin

I hate new_ prefix. I think it is obvious that we write new values to the history, not old ones.

alexshtin · 2022-07-12T00:22:10Z

temporal/api/history/v1/message.proto

+    // If set, update the workflow run timeout to this value.
+    google.protobuf.Duration new_workflow_run_timeout = 3 [(gogoproto.stdduration) = true];
+    // If set, update the workflow execution timeout to this value.
+    google.protobuf.Duration new_workflow_execution_timeout = 4 [(gogoproto.stdduration) = true];


yeah, duration is nilable but sooner or later we will face same problem with int or string. There are these guys but they are inconvenient to use.

alexshtin · 2022-07-12T00:31:51Z

temporal/api/history/v1/message.proto

+    // If set, update the workflow run timeout to this value.
+    google.protobuf.Duration new_workflow_run_timeout = 3 [(gogoproto.stdduration) = true];
+    // If set, update the workflow execution timeout to this value.
+    google.protobuf.Duration new_workflow_execution_timeout = 4 [(gogoproto.stdduration) = true];


How will handler of this event look like? Will there be bunch of if for nil/zero value for every field? Is it scalable?

Sushisource · 2022-07-15T17:54:32Z

I hate new_ prefix. I think it is obvious that we write new values to the history, not old ones.

Does anyone else have an opinion here? I don't care that much, but I feel it is slightly more clear, and the extra characters aren't exactly hurting anyone.

cretz · 2022-07-15T18:48:52Z

I hate new_ prefix.

Does anyone else have an opinion here?

I'm fine with the new_ prefix. It makes it clear that it's unpopulated if there is nothing new (as opposed to having all fields populated even if they didn't change). It also disambiguates it from future additions that may not have an "old" version of the field.

But I'm fine without the new_ prefix too.

rodrigozhou · 2022-07-15T19:47:32Z

In the replay, there's a check that verifies events with the corresponding command if the event is triggered by a command. It also checks if the attributes values matches.

Since I'm gonna add a command and re-use the event being added here, this event could be coming from either the API or the command. So, we need a way to differentiate them.

Here are a couple of thoughts:
a) Add a fields in the event attributes to indicate where it comes from.
b) Create an event to indicate the next event comes from API (I saw this in the code for another use case)

Thoughts?

cretz · 2022-07-15T20:23:13Z

In the replay, there's a check that verifies events with the corresponding command if the event is triggered by a command.

@rodrigozhou - I think we can just have the "check" in each SDK just make sure that if they made a command the event is present, but otherwise ignore the event. The only concern I can see is what if there is a command-based event and an API-based event in the same task, how will you know which to verify against the command? Since this event is unique in this way, I would support an "initiator" enum on the event to say whether it was done in or out of workflow (or bool).

bergundy · 2022-07-16T02:51:46Z

Great point @rodrigozhou, we should add an initiator attribute as @cretz suggested.
Would be good to have the record anyways

Sushisource · 2022-07-19T20:30:44Z

@rodrigozhou et all. Good point about the initiator field, but let's add that when it's needed for sending the command from SDK. No direct need to add it to this PR.

temporal/api/history/v1/message.proto

bergundy · 2022-07-19T22:46:38Z

@rodrigozhou et all. Good point about the initiator field, but let's add that when it's needed for sending the command from SDK. No direct need to add it to this PR.

I would add this now rather than later, otherwise SDK versions that don't take this field into account would assume the initiator is a workflow and would throw a non-determinism error.

Sushisource · 2022-07-19T23:28:57Z

@rodrigozhou After some discussion I think the command should have it's own event. It's icky for us to re-use the same event for the command b/c it leads to some poor type design where impossible states are representable. They might be a bit duplicative but that's totally fine.

Sushisource commented Jul 6, 2022

View reviewed changes

temporal/api/history/v1/message.proto Outdated Show resolved Hide resolved

Sushisource requested a review from yiminc July 6, 2022 23:56

bergundy approved these changes Jul 7, 2022

View reviewed changes

cretz reviewed Jul 7, 2022

View reviewed changes

yycptt reviewed Jul 11, 2022

View reviewed changes

alexshtin requested changes Jul 12, 2022

View reviewed changes

yycptt mentioned this pull request Jul 12, 2022

Support upsert memo temporalio/temporal#3091

Merged

Sushisource added 5 commits July 19, 2022 13:31

Add property/mutable state updating events

e47d28c

Review fixes

f9e1eed

Rename field

4c141aa

Add memo

161f667

Merge fixes

7de3e44

Sushisource force-pushed the add-property-updating-events branch from 6123c46 to 7de3e44 Compare July 19, 2022 20:33

Sushisource requested review from a team as code owners July 19, 2022 20:33

alexshtin approved these changes Jul 19, 2022

View reviewed changes

Fix memo comment

6166664

Sushisource enabled auto-merge (squash) July 19, 2022 22:11

Sushisource merged commit f24b135 into master Jul 19, 2022

Sushisource deleted the add-property-updating-events branch July 19, 2022 22:12

bergundy reviewed Jul 19, 2022

View reviewed changes

temporal/api/history/v1/message.proto Show resolved Hide resolved

bergundy added a commit that referenced this pull request Jul 19, 2022

Minor adjustments from #196

71227ee

bergundy mentioned this pull request Jul 19, 2022

Minor adjustments from #196 #208

Merged

bergundy added a commit that referenced this pull request Jul 19, 2022

Minor adjustments from #196

1fcafc9

bergundy added a commit that referenced this pull request Jul 21, 2022

Minor adjustments from #196

aa4fb8e

bergundy added a commit that referenced this pull request Jul 21, 2022

Minor adjustments from #196 (#208)

db530b8

Add property/mutable state updating events #196

Add property/mutable state updating events #196

Conversation

Sushisource commented Jul 6, 2022

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

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

Sushisource Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource Jul 11, 2022 • edited Loading

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 Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

alexshtin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource commented Jul 15, 2022

cretz commented Jul 15, 2022

rodrigozhou commented Jul 15, 2022

cretz commented Jul 15, 2022 • edited Loading

bergundy commented Jul 16, 2022

Sushisource commented Jul 19, 2022

bergundy commented Jul 19, 2022

Sushisource commented Jul 19, 2022

Sushisource Jul 7, 2022 •

edited

Loading

Sushisource Jul 11, 2022 •

edited

Loading

cretz Jul 12, 2022 •

edited

Loading

cretz commented Jul 15, 2022 •

edited

Loading