change the config propagation design of ts-config#766
Conversation
feiyang3cat
changed the title
| // This bound is not propagated to transitively related workflows. | ||
| // When bound is also needed for transitively related workflows, | ||
| // it is recommended to set disable_propagation to true | ||
| // and configure TimeSkippingConfig explicitly for transitively related workflows. |
There was a problem hiding this comment.
Reset workflows & continue-as-new don't produce another workflow execution the way child workflows do. Instead, they either fix the current workflow (reset) or extend the current wf for technical reasons (continue-as-new).
Therefore:
(1) The only flexibility to disable propagation applies to child workflows. disable_propagation → rename to disable_child_propagation
(2) Reset workflow shouldn't be highlighted here — reset rewinds execution to a specific event, and what happens next depends entirely on the event history. (removed to make the comments concise)
(3) For continue-as-new, propagation cannot be flexibly configured. The new workflow execution is treated as an extension of the original — they share the same bound. This should be called out explicitly in the comments.
feiyang3cat
force-pushed
the
api-ts-patch-2
branch
from
c6fcb5a to
4d643c6
Compare
feiyang3cat
force-pushed
the
api-ts-patch-2
branch
from
4d643c6 to
6d8c845
Compare
feiyang3cat
changed the title
| // using the same values as the parent workflow. | ||
| // If set, child workflows will be started with time skipping disabled. | ||
| // Regardless of this field, the start time of the child workflow uses the parent's virtual time. | ||
| bool disable_child_workflow_propagation = 3; |
There was a problem hiding this comment.
lol interesting that 3 is not used before.
There was a problem hiding this comment.
yes, the 3 was somehow saved by fate
| // When bound is also needed for transitively related workflows, | ||
| // it is recommended to set disable_propagation to true | ||
| // and configure TimeSkippingConfig explicitly for transitively related workflows. | ||
| // Bound scope: |
There was a problem hiding this comment.
Probably also clarify the behavior of bound on reset.
Will, for example, the skipped/elapsed duration get reset as well?
There was a problem hiding this comment.
For this part, I haven't found a clean way to document the full "propagation" behavior without being verbose or exposing too many internal details. As we agreed a sound design ties time skipping propagation closely to the type of workflow execution being triggered, and there are roughly three categories:
(1) Feature-wise propagation — config and virtual time are carried over to a new, distinct workflow execution:
- Child workflows
- Nexus (only if the Nexus operation maps to a workflow execution — so we drop this )
(2) Only implementation-wise propagation:
- Reset: returns execution to a point in the current execution's event history. The resulting config and virtual time depend on the target event and the state of the execution at that time.
- Continue-as-new: will be treated as one extended execution run, and all CANs share the config and inherit the virtual time from previous one;
- Retry: won't inherit the virtual time, and only inherit the initial configuration at the time previous workflow started
(3) Grey area
- Cron / Scheduler: best strategy TBD
So IMH, we postpone add an explanation of this whole strategy until all of them are soundly completed.
There was a problem hiding this comment.
alternatively, we may hold this pr until I finish the propagation part of the project
There was a problem hiding this comment.
IMO there's kind of no reason in general to have any of these API changes in master until we actually have the MVP working.
That said, we already do, and it's just updating a comment, so not a big deal either way.
But to my point in my other comment about not needing reserved - this is kinda why it's easier to just keep API stuff unmerged until we're happy with the implementation.
| reserved 2; | ||
| reserved "disable_propagation"; |
There was a problem hiding this comment.
Do you actually need to bother with reserving 2? We never shipped any of this so it should be fine to break
There was a problem hiding this comment.
Get it.
I will hold this PR until the propagation feature set is completed and ready to merge into server main.
And future updates to API will hold for a while until related features are ready and stable.
We took an approach of merging feature-set by feature-set of this project into main to avoid maintaining a long lasting pr and merging a super huge pr into the server.
There was a problem hiding this comment.
But it seems we cannot pass the lint check without reserving it.
| // When bound is also needed for transitively related workflows, | ||
| // it is recommended to set disable_propagation to true | ||
| // and configure TimeSkippingConfig explicitly for transitively related workflows. | ||
| // Bound scope: |
There was a problem hiding this comment.
IMO there's kind of no reason in general to have any of these API changes in master until we actually have the MVP working.
That said, we already do, and it's just updating a comment, so not a big deal either way.
But to my point in my other comment about not needing reserved - this is kinda why it's easier to just keep API stuff unmerged until we're happy with the implementation.
|
close this pr, and merge until the propagation feature set is completed |
3 participants
What changed?
(1) Only allowed the flexibility of disable_propagation for child workflows
(2) Updated comments to clarify the behavior of bound in TimeSkippingConfig
Why?
mainly for simplicity:
For reviewers: check the inline comment for detailed explanation