Add workflow_start_delay #264
Conversation
|
|
|
Hrmm, this is a user request that predates schedules right? And even if they didn't want to use schedules, IMO it doesn't seem like our public API and then every SDK needs to be updated for this feature just to save that user a sleep step. Maybe our scheduler needs a single-workflow-after-offset feature. |
|
This request actually comes from @tusharroy25. Re. Schedule. I am not sure if schedule support this or not, but using Schedule for one-time delayed workflow execution seems weird. |
My fear is that that continual one-offs for how to schedule workflows in the future will pile up outside of the actual scheduling. It's kinda silly we can't use the schedule-future-workflow feature to solve our schedule future workflow use case. Maybe we need to support a fixed time on our schedule trigger? Or maybe we need to accept a set of fixed times in our schedule spec? For now, technically this can be solved with a calendar spec of the exact time you want schedule and just set the action limit to 1. If this is too unwieldy we should consider making it easier. FWIW, I am ok with this feature if decided worth it by others, I just want to make sure we're not missing a generic opportunity to schedule future workflows. Also should we put this on |
|
Fwiw, I think using schedules for this is a good idea in theory. We can easily add a list of fixed times option to the spec, and an option to automatically exit if there are no more times. The main caveat is that it's quite a bit more expensive than this method, since we deliberately traded efficiency for flexibility with the initial schedules implementation. If we reimplement it with lower-level primitives then that objection would go away (that would be something like 1+ years out if it happens) This has nothing to do with phase, that's for adjusting the phase of a regular interval (from the epoch), this is for one-offs (from call time). As far as NewWorkflowExecutionInfo: I had a proposal a while ago to consolidate the various proto messages there (internal link). I think it does belong there, but it's maybe silly since the only user of that message for now (schedules) would probably force it to be zero, since it doesn't make sense to combine with schedules. |
|
I think what's here is fine, I am just providing a counter points. Other counter points:
I could probably have many more questions. I think there are potentially simpler more flexible options here, but I understand this has already been discussed and decided. |
| temporal.api.common.v1.SearchAttributes search_attributes = 18; | ||
| temporal.api.common.v1.Header header = 19; | ||
| // Time to wait before starting the first workflow run. | ||
| google.protobuf.Duration workflow_start_delay = 20 [(gogoproto.stdduration) = true]; |
There was a problem hiding this comment.
This one doesn't actually make much sense to me. The point is to start a workflow and immediately send it a signal, which usually means that the caller wants it to handle the signal as soon as possible. We already have a bunch of inconsistent/buggy behavior around delayed workflow start + signals, so I'd rather just leave this out and make this impossible to specify.
There was a problem hiding this comment.
Talked internally, this is fine, but please change the comment to include more details
| temporal.api.common.v1.SearchAttributes search_attributes = 18; | ||
| temporal.api.common.v1.Header header = 19; | ||
| // Time to wait before starting the first workflow run. | ||
| google.protobuf.Duration workflow_start_delay = 20 [(gogoproto.stdduration) = true]; |
There was a problem hiding this comment.
Talked internally, this is fine, but please change the comment to include more details
| // StartWorkflowExecution. | ||
| temporal.api.failure.v1.Failure continued_failure = 18; | ||
| temporal.api.common.v1.Payloads last_completion_result = 19; | ||
| // Time to wait before starting the first workflow run. Cannot be used with `cron_schedule`. |
There was a problem hiding this comment.
| // Time to wait before starting the first workflow run. Cannot be used with `cron_schedule`. | |
| // Time to wait before dispatching the first workflow task. Cannot be used with `cron_schedule`. | |
| // If the workflow gets a signal before the delay, a workflow task will be dispatched and the rest | |
| // of the delay will be ignored. |
| temporal.api.common.v1.Memo memo = 17; | ||
| temporal.api.common.v1.SearchAttributes search_attributes = 18; | ||
| temporal.api.common.v1.Header header = 19; | ||
| // Time to wait before starting the first workflow run. |
There was a problem hiding this comment.
| // Time to wait before starting the first workflow run. | |
| // Time to wait before dispatching the first workflow task. Note that the | |
| // signal will be delivered with the first workflow task. If the workflow gets | |
| // another signal before the delay, a workflow task will be dispatched | |
| // immediately and the rest of the delay will be ignored. |
There was a problem hiding this comment.
If the workflow gets another signal, via SignalWithStartWorkflow, before the delay, a workflow task will be dispatched ...
Signal via SignalWorkflowExecution won't unblock the workflow.
What changed?
Added ability to specify a delay when starting a workflow. Cannot be used with workflows that have a
cron_scheduleWhy?
temporalio/temporal#3378
Breaking changes
None