Conversation
| message TimeSkippingConfig { | ||
|
|
||
| // If set, enables automatic time-skipping for this workflow execution. | ||
| // It can also be disabled by setting this field to false. |
There was a problem hiding this comment.
@Sushisource Hey! I've cleaned this pr and ready for review.
And this config may need special attention since it is changed significantly from previous version Chad proposed.
(1)deleted most of the fine-grained control (like max timer-firing count, max duration to skip) from the previous version. The reason is it is ambiguous when complicated configuration is propagated to child-workflows.
(2) I have also changed the nested config to one single layer because this configuration is only used for automatic time-skipping, manual time-skipping will have a separate request and response payload
(3) we allow disabling time-skipping for in-flight workflows since it is not hard for the server to have this feature, and it will be a nice-to-have feature same as the SDK TestServer.
I think we can consider wrapping up this pr early, and put manual time-skipping in a separate pr maybe.
Pls feel free to disagree, happy to refine.
There was a problem hiding this comment.
I think this looks totally reasonable - but I also don't see any reason to rush the PR while work is in progress unless we intend to ship a version without manual skipping first. If we don't, let's use the opportunity to develop it and decide if we want to make any changes to this config that relate to manual timeskipping.
83f6639 to
e1e1c03
Compare
| message TimeSkippingConfig { | ||
|
|
||
| // If set, enables automatic time-skipping for this workflow execution. | ||
| // It can also be disabled by setting this field to false. |
There was a problem hiding this comment.
I think this looks totally reasonable - but I also don't see any reason to rush the PR while work is in progress unless we intend to ship a version without manual skipping first. If we don't, let's use the opportunity to develop it and decide if we want to make any changes to this config that relate to manual timeskipping.
9721ff7 to
c686992
Compare
| // An event that indicates that the previously paused workflow execution has been unpaused. | ||
| EVENT_TYPE_WORKFLOW_EXECUTION_UNPAUSED = 59; | ||
| // An event that indicates that some duration was skipped for this workflow execution. | ||
| EVENT_TYPE_WORKFLOW_EXECUTION_TIME_SKIPPED = 60; |
There was a problem hiding this comment.
You know this just occurred to me and I don't think we've talked about it - if time skipping is enabled on a workflow with an old SDK version processing it, it's going to break on these new event types.
That's fine I think, we will tell people they need to upgrade, but we need to make sure that is clear in the docs & GTM material we create.
There was a problem hiding this comment.
get it. the docs & GTM material mainly refer to API comments (here) and https://docs.temporal.io/ ?
There was a problem hiding this comment.
if we always set worker_may_ignore for TimeSkippingEvents, it won't break workers right? because in the demo, I set worker_may_ignore = true and used current SDK which has no time-skipping feature and didnt break it
There was a problem hiding this comment.
I think for sdk version old enough, it doesn't even know about the worker_may_ignore flag, hence the comment. but yeah this is a new feature so we can make it clear re. the sdk version required.
| // when true, it means the automatic time-skipping was enabled with a bound | ||
| // and now the bound is reached and the time-skipping is disabled automatically. | ||
| bool bound_reached_and_time_skipping_disabled = 2; |
There was a problem hiding this comment.
| // when true, it means the automatic time-skipping was enabled with a bound | |
| // and now the bound is reached and the time-skipping is disabled automatically. | |
| bool bound_reached_and_time_skipping_disabled = 2; | |
| // when true, automatic time-skipping was enabled with a bound | |
| // and now that bound has been reached and time-skipping is disabled automatically. | |
| bool disabled_after_bound = 2; |
This is my best idea for a better name. The comment makes things clear, so I don't think we need a super wordy field name.
837c381 to
012f767
Compare
| string request_id = 3; | ||
| } | ||
|
|
||
| // Attributes for an event indicating that virtual time was advanced for a workflow execution, |
There was a problem hiding this comment.
⭐ highlight the change after review meeting
There was a problem hiding this comment.
We should also add to the comment here that worker_may_ignore should always be set true for this event
api/temporal/api/history/v1/message.proto
Line 1127 in 2547d30
There was a problem hiding this comment.
added but I remember an early proposal was to break the workers and remind them to upgrade. So I guess our strategy changed?
There was a problem hiding this comment.
Sorry, I meant we should add this to the comment here, not on the field. Otherwise that field will have an ever-increasing list in the comment.
And I think that was before we realized workers don't need to handle this at all. Even old SDKs will be able to successfully replay histories with time-skipping, which could be useful if users want to try this without needing to upgrade SDKs.
There was a problem hiding this comment.
I guess I located here correctly this time?
There was a problem hiding this comment.
It should be on this message, yeah, I don't see it here yet though?
| string request_id = 3; | ||
| } | ||
|
|
||
| // Attributes for an event indicating that virtual time was advanced for a workflow execution, |
There was a problem hiding this comment.
We should also add to the comment here that worker_may_ignore should always be set true for this event
api/temporal/api/history/v1/message.proto
Line 1127 in 2547d30
| // An event that indicates that the previously paused workflow execution has been unpaused. | ||
| EVENT_TYPE_WORKFLOW_EXECUTION_UNPAUSED = 59; | ||
| // An event that indicates that some duration was skipped for this workflow execution. | ||
| EVENT_TYPE_WORKFLOW_EXECUTION_TIME_SKIPPED = 60; |
There was a problem hiding this comment.
I think for sdk version old enough, it doesn't even know about the worker_may_ignore flag, hence the comment. but yeah this is a new feature so we can make it clear re. the sdk version required.
| // If set, the enabled field is not propagated to child workflows. | ||
| bool disable_propagation = 2; |
There was a problem hiding this comment.
should clarify if this will propagate via continue as new or reset as well.
There was a problem hiding this comment.
I am thinking we make it simple now: we don't propagate bound and propagate enable for all these three cases. And this feature is not supported for nexus operation so no propagation for now. make sense?
There was a problem hiding this comment.
yeah that make sense. Just want to make sure ^ is in the comment.
|
|
||
| message DeleteActivityExecutionResponse { | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
emm it seems not to be a new line >?
There was a problem hiding this comment.
not sure i follow. it means a newline char is removed at file end. not something critical.
| // Configuration for automatic time skipping during a workflow execution. | ||
| // When enabled, virtual time advances automatically whenever there are | ||
| // no in-flight activities, child workflows, or Nexus operations. | ||
| message TimeSkippingConfig { |
There was a problem hiding this comment.
do we need to add this in to child workflow command?
There was a problem hiding this comment.
Specifically, I don't imagine we will need to set childWorkflow time-skipping config thru command when users use SDK testing env. And in general, I think we may keep this as complex as the current version and only consider adding more when SDK really needs it.
There was a problem hiding this comment.
no strong opinion either way given we support time skipping config propagation. Please track the potential work item somewhere though.
There was a problem hiding this comment.
oh yes, I will follow up on this, but I want to change the topic a bit. here what I think we should follow up is on how to provide more user-friendly configurations for complex testing scenarios with child wfs/start-as-new/etc, than a specific tech solution to add config in commands.
And that will be an later phase working with SDK integrations.
525ec42 to
cabd8ee
Compare
ac4dd6b to
2bf473b
Compare
…pping, and change event name
2bf473b to
9da7862
Compare
| 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) |
There was a problem hiding this comment.
| // By default, this field is propagated to transitively related workflows (child workflows/start-as-new/reset) | |
| // By default, this field is propagated to transitively related workflows (child workflows/continue-as-new/reset) |
| // An event that indicates that the previously paused workflow execution has been unpaused. | ||
| EVENT_TYPE_WORKFLOW_EXECUTION_UNPAUSED = 59; | ||
| // An event that indicates time skipping advanced time or was disabled automatically after a bound was reached. | ||
| EVENT_TYPE_WORKFLOW_EXECUTION_TIME_SKIPPING_STATE_CHANGED = 60; |
There was a problem hiding this comment.
The name TIME_SKIPPING_STATE_CHANGED makes me feel there this should the event upon enabling time skipping as well, but instead it will be WorkflowExecutionOptionsUpdated.
I actually don't have strong opinion re. if we need to have an event when time skipping is disabled but there's no actual time skipping occurred, so the old timeSkipped event name works for me as well.
What changed?
new feature: automatic time-skipping configuration and new event type added
Breaking changes
new event type added but right now no workflow history will have this new event type
Server PR
not expect to break server, no new rpc-handler and new fields are all optional