-
Notifications
You must be signed in to change notification settings - Fork 93
change the config propagation design of ts-config #766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -592,26 +592,29 @@ message WorkflowExecutionOptions { | |
| message TimeSkippingConfig { | ||
|
|
||
| // Enables or disables time skipping for this workflow execution. | ||
| // By default, this field is propagated to transitively related workflows (child workflows/start-as-new/reset) | ||
| // at the time they are started. | ||
| // Changes made after a transitively related workflow has started are not propagated. | ||
| bool enabled = 1; | ||
|
|
||
| // If set, the enabled field is not propagated to transitively related workflows. | ||
| bool disable_propagation = 2; | ||
| reserved 2; | ||
| reserved "disable_propagation"; | ||
|
|
||
| // Optional bound that limits how long time skipping remains active. | ||
| // Once the bound is reached, time skipping is automatically disabled. | ||
| // It can later be re-enabled via UpdateWorkflowExecutionOptions. | ||
| // By default, the time skipping configuration (enabled and bound) is propagated to child workflows, | ||
| // 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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol interesting that 3 is not used before.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, the 3 was somehow saved by fate |
||
|
|
||
| // Optional bound that limits how far virtual time can advance while time skipping is active. | ||
| // Once the bound is reached, time skipping is automatically disabled, | ||
| // but can be re-enabled via UpdateWorkflowExecutionOptions. | ||
| // | ||
| // This is particularly useful in testing scenarios where workflows | ||
| // are expected to receive signals, updates, or other events while | ||
| // timers are in progress. | ||
| // This is useful in testing scenarios where a workflow is expected to receive | ||
| // signals, updates, or other external events while timers are in progress. | ||
| // | ||
| // 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. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| // Bound scope: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably also clarify the behavior of bound on reset.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
(2) Only implementation-wise propagation:
(3) Grey area
So IMH, we postpone add an explanation of this whole strategy until all of them are soundly completed.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. alternatively, we may hold this pr until I finish the propagation part of the project
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| // - Each bound is independent for each workflow execution. | ||
| // When a bound is propagated to a child workflow, the child's bound is only applied to that child execution. | ||
| // - Continue-as-new is an exception: continued workflow executions are treated as extensions of the | ||
| // original execution, so the bound is shared across all executions in the chain. | ||
| oneof bound { | ||
|
|
||
| // Maximum total virtual time that can be skipped. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it seems we cannot pass the lint check without reserving it.