Job ordering updates #789
Job ordering updates #789
Conversation
Sushisource
force-pushed
the
update-seed-jobs-first
branch
from
608661f to
856a848
Compare
Sushisource
changed the title
core/src/worker/workflow/mod.rs
Outdated
| workflow_activation_job::Variant::StartWorkflow(_) => 1, | ||
| workflow_activation_job::Variant::UpdateRandomSeed(_) => 2, | ||
| workflow_activation_job::Variant::SignalWorkflow(_) => 3, | ||
| workflow_activation_job::Variant::DoUpdate(_) => 3, |
There was a problem hiding this comment.
I don't think this works. I think signal/update in the same task as start always need to come before start. It's important for data loss reason that signal/update handlers run before the primary method.
There was a problem hiding this comment.
Lengthy discussion outside of the PR summarized:
This change's impetus is to make the sending of information be appropriately ordered, and lang can always assume that a main routine ought to be started at the end of the first activation.
core/src/worker/workflow/mod.rs
Outdated
| workflow_activation_job::Variant::SignalWorkflow(_) => 2, | ||
| workflow_activation_job::Variant::DoUpdate(_) => 2, | ||
| workflow_activation_job::Variant::NotifyHasPatch(_) => 0, | ||
| workflow_activation_job::Variant::InitializeWorkflow(_) => 1, |
There was a problem hiding this comment.
I was under the impression the contract now was that this should always be the first job on the activation, so I wonder if this should be 0 and notify has patch should be 1
There was a problem hiding this comment.
Yeah, I suppose there's really no reason not to do that. Will change.
What was changed
Make
UpdateRandomSeedjobs come after start workflow, but before everything else.Why?
These could technically have consequences for later actions, and should go first. It's intuitive.
The more delicate change here is actually sorting
StartWorkflowbefore other stuff, which also seems intuitive, but a number of tests at least in core had some expectations around updates being able to come first. Frankly, that doesn't make a ton of sense, and I don't really expect this move to be any kind of real issue, but it might require some changes to langs that are usingStartWorkflowas a synonym for "do first iteration of the event loop"? If such changes are needed, they should be the ones described here: temporalio/sdk-python#606 (.NET may need an equivalent item) as none of these problems exist if all jobs are all read and applied to state first, before doing any iterating of the event loop.Checklist
Closes [Bug] UpdateRandomSeed jobs should go first with patches #790
How was this tested:
Updated existing tests
Any docs updates needed?