-
Notifications
You must be signed in to change notification settings - Fork 78
[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
base: master
Are you sure you want to change the base?
Changes from all commits
7efd5cb
fbd9785
b076f2a
e261a75
414ead2
1892424
6a5e637
cef26c1
16af4ae
b2a5959
5d8eed6
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 |
|---|---|---|
|
|
@@ -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; | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
Verifying that we have consensus for adding this new status... this is fine with me FTR.
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.
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.
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.
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.
If (and when) we support pausing of individual aspects we would have individual status for them to make it clear that only certain aspects are paused.
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.
👍
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.
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.
I'm actually not completely sure. It depends on how we support these.
If pausing these individual aspects is a top level operation then it would have a separate status (like activity pause). If it is an option to workflow pause then the workflow status would be changed.
I'm also not sure if we will support pausing just the individual aspects without pausing the workflow tasks.
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.
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).