-
Notifications
You must be signed in to change notification settings - Fork 78
Proto changes to support activity pause by type options #613
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
441dcd3
d7a5344
7b363d9
53a2159
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -161,3 +161,11 @@ enum VersioningBehavior { | |||||
| // complete on the old Version. | ||||||
| VERSIONING_BEHAVIOR_AUTO_UPGRADE = 2; | ||||||
| } | ||||||
|
|
||||||
| // Pause option activity state is used to specify if we want to pause/unpause just the current activity, or just the future activities or both. | ||||||
| enum PauseOptionActivityState { | ||||||
|
Contributor
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. either create acitvity.proto, or add to "common".
Contributor
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. why this comment is here... |
||||||
| PAUSE_OPTION_ACTIVITY_STATE_UNSPECIFIED = 0; | ||||||
| PAUSE_OPTION_ACTIVITY_STATE_RUNNING = 1; | ||||||
| PAUSE_OPTION_ACTIVITY_STATE_FUTURE = 2; | ||||||
| PAUSE_OPTION_ACTIVITY_STATE_BOTH = 3; | ||||||
|
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.
Suggested change
|
||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1905,12 +1905,17 @@ message PauseActivityRequest { | |
| oneof activity { | ||
| // Only the activity with this ID will be paused. | ||
| string id = 4; | ||
| // Pause all running activities of this type. | ||
| // Pause activities of this type. | ||
| string type = 5; | ||
| // Pause all activities. | ||
| bool pause_all = 7; | ||
| } | ||
|
|
||
| // Reason to pause the activity. | ||
| string reason = 6; | ||
|
|
||
| // which activities to pause - current, future or both. | ||
|
Contributor
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. comment says "which activities to pause" - but previous oneof selector selects which activities to pause.
Contributor
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. same for "match all". I don't think "match all" and "for all, now and in the future" should coexist... it should be part of "type".
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. IMO they are independent filters and if both are present, both must be satisfied. The other is "which activities", this is "also limit by current state" IMO. |
||
| temporal.api.enums.v1.PauseOptionActivityState activity_state = 8; | ||
| } | ||
|
|
||
| message PauseActivityResponse { | ||
|
|
@@ -1929,9 +1934,9 @@ message UnpauseActivityRequest { | |
| oneof activity { | ||
| // Only the activity with this ID will be unpaused. | ||
| string id = 4; | ||
| // Unpause all running activities with of this type. | ||
| // Unpause activities of this type. | ||
| string type = 5; | ||
| // Unpause all running activities. | ||
| // Unpause all activities. | ||
| bool unpause_all = 6; | ||
| } | ||
|
|
||
|
|
@@ -1943,6 +1948,9 @@ message UnpauseActivityRequest { | |
|
|
||
| // If set, the activity will start at a random time within the specified jitter duration. | ||
| google.protobuf.Duration jitter = 9; | ||
|
|
||
| // which activities to unpause - current, future or both. | ||
|
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. Inconsistent with capitalization of comments
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. Safe to assume "any" is the default when unspecified? Can you clarify that in comment? |
||
| temporal.api.enums.v1.PauseOptionActivityState activity_state = 10; | ||
| } | ||
|
|
||
| message UnpauseActivityResponse { | ||
|
|
@@ -2208,8 +2216,8 @@ message ListWorkerDeploymentsResponse { | |
| google.protobuf.Timestamp create_time = 2; | ||
| temporal.api.deployment.v1.RoutingConfig routing_config = 3; | ||
| // Summary of the version that was added most recently in the Worker Deployment. | ||
| temporal.api.deployment.v1.WorkerDeploymentInfo.WorkerDeploymentVersionSummary latest_version_summary = 4; | ||
| // Summary of the current version of the Worker Deployment. | ||
| temporal.api.deployment.v1.WorkerDeploymentInfo.WorkerDeploymentVersionSummary latest_version_summary = 4; | ||
| // Summary of the current version of the Worker Deployment. | ||
| temporal.api.deployment.v1.WorkerDeploymentInfo.WorkerDeploymentVersionSummary current_version_summary = 5; | ||
| // Summary of the ramping version of the Worker Deployment. | ||
| temporal.api.deployment.v1.WorkerDeploymentInfo.WorkerDeploymentVersionSummary ramping_version_summary = 6; | ||
|
|
||
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.
Why is this in
workflow.protoand notactivity.proto?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.
Or maybe this could be generalized for workflows too? Would we want this for workflow pause?