-
Notifications
You must be signed in to change notification settings - Fork 80
[Workflow Pause] Proto changes #653
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
7efd5cb
fbd9785
b076f2a
e261a75
414ead2
1892424
6a5e637
cef26c1
16af4ae
b2a5959
5d8eed6
9a804df
51d85ac
6159855
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,7 @@ enum WorkflowExecutionStatus { | |
| WORKFLOW_EXECUTION_STATUS_TERMINATED = 5; | ||
| WORKFLOW_EXECUTION_STATUS_CONTINUED_AS_NEW = 6; | ||
| WORKFLOW_EXECUTION_STATUS_TIMED_OUT = 7; | ||
| WORKFLOW_EXECUTION_STATUS_PAUSED = 8; | ||
|
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. Verifying that we have consensus for adding this new status... this is fine with me FTR.
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. I think I am the only holdout. I still think it breaks Temporal expectation that a workflow is either running or closed, and that running has nothing to do with whether tasks are being processed (or signals or activities or timers). Having said that, I think unfortunately people are wanting to break these expectations. I also think it's confusing to assume the entire workflow is in a paused status just because some things are paused. When a user asks us to support, say, pausing only signals/updates, it's gonna look real strange that we chose to treat pause as a status instead of what it truly is - a collection of paused aspects.
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. We had the consensus in the internal discussion about this. The reasoning was that pausing a workflow would set an expectation that the workflow status to change to PAUSED. Otherwise "workflow pause" operation appears broken.
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.
👍
I don't think this is true that it would set this expectation any more than "cancel" would set an expectation of a workflow status changing to "cancel requested". Basically, there is no such thing as pausing a workflow, just pausing certain parts. I want to warn about user confusion by marking what Temporal deems to be a started/running workflow as not running.
To clarify here, you mean the workflow status would remain running if we only paused signals, updates, etc? Or we would set it to paused even if some of it is paused? That's what we're doing with this change, e.g. the timers are not paused, but we are acting as if the whole workflow is paused when it is not.
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. I'm actually not completely sure. It depends on how we support these.
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.
Note, you are making a permanent "can't take back" change here for something you may not be sure about in successive phases. But if we are defining status "paused" as "workflow tasks are paused" and not some general definition of "pause", ok (even if I do disagree with the premise).
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. In all cases when a workflow status is set to paused, workflow tasks would remain paused. Other aspects are TBD. So, "paused" status is consistent with workflow task pause.
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. I'm not necessarily convinced that will always be the case (e.g. user wanting to pause interactions like update but still process activity completions), but if we think so, ok. |
||
| } | ||
|
|
||
| enum PendingActivityState { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -320,7 +320,7 @@ message WorkflowTaskFailedEventAttributes { | |
| temporal.api.enums.v1.WorkflowTaskFailedCause cause = 3; | ||
| // The failure details | ||
| temporal.api.failure.v1.Failure failure = 4; | ||
| // If a worker explicitly failed this task, this field contains the worker's identity. | ||
| // If a worker explicitly failed this task, this field contains the worker's identity. | ||
| // When the server generates the failure internally this field is set as 'history-service'. | ||
| string identity = 5; | ||
| // The original run id of the workflow. For reset workflow. | ||
|
|
@@ -885,6 +885,26 @@ message WorkflowExecutionUpdateAdmittedEventAttributes { | |
| temporal.api.enums.v1.UpdateAdmittedEventOrigin origin = 2; | ||
| } | ||
|
|
||
| // Attributes for an event marking that a workflow execution was paused. | ||
| message WorkflowExecutionPausedEventAttributes { | ||
| // The identity of the client who paused the workflow execution. | ||
| string identity = 1; | ||
| // The reason for pausing the workflow execution. | ||
| string reason = 2; | ||
| // The request ID of the request that paused the workflow execution. | ||
| string request_id = 3; | ||
| } | ||
|
|
||
| // Attributes for an event marking that a workflow execution was unpaused. | ||
| message WorkflowExecutionUnpausedEventAttributes { | ||
|
Comment on lines
+889
to
+899
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. I think you forgot to add these to the actual oneof below
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. I had forgotten them. Added now.
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. I think we might want to wait to merge this until the implementation is present so we can confirm these types of things work with the code
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. I will not merge until the corresponding server PR using this is not ready. |
||
| // The identity of the client who unpaused the workflow execution. | ||
| string identity = 1; | ||
| // The reason for unpausing the workflow execution. | ||
| string reason = 2; | ||
| // The request ID of the request that unpaused the workflow execution. | ||
| string request_id = 3; | ||
| } | ||
|
|
||
| // Event marking that an operation was scheduled by a workflow via the ScheduleNexusOperation command. | ||
| message NexusOperationScheduledEventAttributes { | ||
| // Endpoint name, must exist in the endpoint registry. | ||
|
|
@@ -1103,6 +1123,8 @@ message HistoryEvent { | |
| WorkflowExecutionOptionsUpdatedEventAttributes workflow_execution_options_updated_event_attributes = 60; | ||
| NexusOperationCancelRequestCompletedEventAttributes nexus_operation_cancel_request_completed_event_attributes = 61; | ||
| NexusOperationCancelRequestFailedEventAttributes nexus_operation_cancel_request_failed_event_attributes = 62; | ||
| WorkflowExecutionPausedEventAttributes workflow_execution_paused_event_attributes = 63; | ||
| WorkflowExecutionUnpausedEventAttributes workflow_execution_unpaused_event_attributes = 64; | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
@gow - don't you have to update
WorkflowIdConflictPolicycomment to no longer talk about "running" but instead "running and not paused" or "open" or something? AndStartWorkflowExecutionRequest.workflow_id_conflict_policyand so on?Also, you may want to create a side issue to come back and update any API docs (and ask docs team to update all docs and ask SDK team to update all SDKs and maybe UI) to update anything that is now affected by our backwards-incompatible reclassification of what running means (went from non-closed to non-closed-non-paused).
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.
Yes. I'll create tasks to track the documentation changes.