[don't merge] time skipping sleep: need review and server PR#786
[don't merge] time skipping sleep: need review and server PR#786feiyang3cat wants to merge 1 commit into
Conversation
feiyang3cat
force-pushed
the
patch/ts
branch
3 times, most recently
from
64911c4 to
f733324
Compare
feiyang3cat
force-pushed
the
patch/ts
branch
2 times, most recently
from
7431f2f to
e24f538
Compare
feiyang3cat
changed the title
feiyang3cat
changed the title
feiyang3cat
force-pushed
the
patch/ts
branch
2 times, most recently
from
9ca984a to
ae2e298
Compare
| // setting `enabled` to true via UpdateWorkflowExecutionOptions. | ||
| // The current workflow execution is a chain of runs (retries, cron, continue-as-new); | ||
| // child workflows are separate executions, so this sleep won't affect them. | ||
| google.protobuf.Duration sleep = 2; |
There was a problem hiding this comment.
other names like sleep_timer, timer, timeskipping_timer
| } | ||
| }, | ||
| "description": "Configuration for time skipping during a workflow execution.\nWhen enabled, virtual time advances automatically whenever there is no in-flight work.\nIn-flight work includes activities, child workflows, Nexus operations, signal/cancel external workflow operations,\nand possibly other features added in the future.\nUser timers are not classified as in-flight work and will be skipped over.\nWhen time advances, it skips to the earlier of the next user timer or the configured bound, if either exists.\n\nPropagation behavior of time skipping:\nThe enabled flag, bound fields, and accumulated skipped duration are propagated to related executions as follows:\n(1) Child workflows and continue-as-new: both the configuration and the accumulated skipped duration are\n inherited from the current execution. The configured bound is shared between the inherited skipped\n duration and any additional duration skipped by the new run.\n(2) Retry and cron: the configuration and accumulated skipped duration are inherited as recorded when the\n current workflow started; the accumulated skipped duration of the current run is not propagated.\n(3) Reset: the new run retains the time-skipping configuration of the current execution. Because reset replays\n all events up to the reset point and re-applies any UpdateWorkflowExecutionOptions changes made after that\n point, the resulting run ends up with the same final time-skipping configuration as the previous run." | ||
| "description": "The configuration for time skipping of a workflow execution (a chain of runs including retries, cron, continue-as-new).\nWhen time skipping is enabled, virtual time advances automatically whenever there is no in-flight work.\nIn-flight work includes activities, child workflows, Nexus operations, signal/cancel external workflow operations,\nand possibly other features added in the future.\nUser timers are not classified as in-flight work and will be skipped over; the virtual clock may also skip to the\ntime point of the registered sleep when there is no in-flight work.\n\nFor child workflows, by default, if the parent execution is skipping time, the child execution will also skip time,\nbut the parent's sleep won't affect the child execution. A flag is provided to disable propagation of the\n\"enabled\" flag to child workflows; regardless of that flag, a child workflow inherits the virtual time from the\nparent execution as its start time." |
There was a problem hiding this comment.
"to the time point of the registered sleep" -- does this mean to end point of the sleep period?
There was a problem hiding this comment.
-
oh I see, this is better expression. yes. will change to "to the end point of the sleep period"
-
and I think I may need to add another thing is if the workflow execution (the whole chain of runs) completes before having a chance to skip time, the sleep may never take place -> check this example
| "sleep": { | ||
| "type": "string", | ||
| "description": "Maximum total virtual time that can be skipped." | ||
| "description": "Optionally register a sleep for the current workflow execution.\nWhen the sleep completes, time skipping is disabled and this action is recorded in\nthe WorkflowExecutionTimeSkippingTransitionedEvent, but it can be re-enabled by\nsetting `enabled` to true via UpdateWorkflowExecutionOptions.\nThe current workflow execution is a chain of runs (retries, cron, continue-as-new);\nchild workflows are separate executions, so this sleep won't affect them." |
There was a problem hiding this comment.
Is the implication here that the sleep will actually take place, and not be skipped?
There was a problem hiding this comment.
yeah, totally, to be specific sleep counts both natural time passing and time skipping.
for example, the user calls sleep(1h) at 10am, and then the sleep is expected to take place in 11am if the workflow execution hasn't completed. And at 11am (virtual workflow time) the sleep takes place, but for the past 1hour the workflow may takes 30 minutes busy with activities (and this is real 30 minutes), and then waiting for some user timer/condition so we skip 30min(this is skipped) to fire the 1hour.
There was a problem hiding this comment.
let me write some sleep examples
| "description": "Enables or disables time skipping for this workflow execution." | ||
| }, | ||
| "maxSkippedDuration": { | ||
| "sleep": { |
There was a problem hiding this comment.
do you think users might confuse this sleep with sleeps in their actual workflow code?
I was thinking it might be useful to tie this more closely to the skipping? but I see that you're moving away from that!
3 participants
What changed?
boundtosleep timerto match the previous semantics exposed by the SDK Testing serverdisable_propagation_for_childto give users a way to stop propagationupdate_timetoUpdateWorkflowExecutionOptionsRequestWhy?
sleepis a simpler and cleaner concept thanboundfor user experience and backward compatibilityTimeSkippingTransitionedEventmatch the sleep durationBreaking changes
maxSkippedDuration-> the server has already implemented this feature need to delete the code firstmaxElapsedDurationtosleep-> the server need to do renamingServer PR
TBD