Skip to content

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.

Sign up for GitHub

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

Open
gow wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Proto changes to support activity pause by type options #613

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
441dcd3
Proto changes to support activity pause by type options
gow Jul 11, 2025
d7a5344
Revert "Proto changes to support activity pause by type options"
gow Jul 14, 2025
7b363d9
Use PauseOptionActivityStatus
gow Jul 14, 2025
53a2159
PauseOptionActivityStatus -> PauseOptionActivityState
gow Jul 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions openapi/openapiv2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7054,6 +7054,27 @@
}
}
},
"PauseActivityRequestPauseByType": {
"type": "object",
"properties": {
"name": {
"type": "string"
},
"option": {
"$ref": "#/definitions/PauseActivityRequestPauseByTypeOption"
}
}
},
"PauseActivityRequestPauseByTypeOption": {
"type": "string",
"enum": [
"PAUSE_BY_TYPE_OPTION_UNSPECIFIED",
"PAUSE_BY_TYPE_OPTION_ALL",
"PAUSE_BY_TYPE_OPTION_RUNNING",
"PAUSE_BY_TYPE_OPTION_FUTURE"
],
"default": "PAUSE_BY_TYPE_OPTION_UNSPECIFIED"
},
"PauseInfoManual": {
"type": "object",
"properties": {
Expand Down Expand Up @@ -7513,9 +7534,9 @@
"type": "string",
"description": "Only the activity with this ID will be paused."
},
"type": {
"type": "string",
"description": "Pause all running activities of this type."
"pauseByType": {
"$ref": "#/definitions/PauseActivityRequestPauseByType",
"description": "Pause by type with option to pause all, running, or future activities of this type."
},
"reason": {
"type": "string",
Expand Down
20 changes: 17 additions & 3 deletions openapi/openapiv3.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9081,12 +9081,26 @@ components:
id:
type: string
description: Only the activity with this ID will be paused.
type:
type: string
description: Pause all running activities of this type.
pauseByType:
allOf:
- $ref: '#/components/schemas/PauseActivityRequest_PauseByType'
description: Pause by type with option to pause all, running, or future activities of this type.
reason:
type: string
description: Reason to pause the activity.
PauseActivityRequest_PauseByType:
type: object
properties:
name:
type: string
option:
enum:
- PAUSE_BY_TYPE_OPTION_UNSPECIFIED
- PAUSE_BY_TYPE_OPTION_ALL
- PAUSE_BY_TYPE_OPTION_RUNNING
- PAUSE_BY_TYPE_OPTION_FUTURE
type: string
format: enum
PauseActivityResponse:
type: object
properties: {}
Expand Down
25 changes: 21 additions & 4 deletions temporal/api/workflowservice/v1/request_response.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1905,12 +1905,28 @@ message PauseActivityRequest {
oneof activity {
// Only the activity with this ID will be paused.
string id = 4;
// Pause all running activities of this type.
string type = 5;
// Pause by type with option to pause all, running, or future activities of this type.
// (-- api-linter: core::0140::prepositions=disabled --)
PauseByType pause_by_type = 7;
}
reserved 5;
reserved "type";

// Reason to pause the activity.
string reason = 6;

// (-- api-linter: core::0123::resource-annotation=disabled --)
message PauseByType{
Copy link
Copy Markdown
Contributor

@cretz cretz Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pedantic, but usually there is a space between identifier and open brace

string name = 1;
PauseByTypeOption option = 2;
}

enum PauseByTypeOption{
Copy link
Copy Markdown
Contributor

@cretz cretz Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression we were considering visibility-like queries for deciding what activities certain actions would affect, is that the case?

Copy link
Copy Markdown
Contributor Author

@gow gow Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for activity pause/unpause APIs. The primary purpose is to allow someone to pause all activities of a certain type. The current implementation explicitly pauses just the running instances of an activity and doesn't pause any future instances.
Regarding rules & actions - nothing changes. Rules will still use queries and identify the activities (by id) to be paused.

Copy link
Copy Markdown
Contributor

@cretz cretz Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for activity pause/unpause APIs

Mentioned in other comment, but this is not applying to unpause that I can see

The primary purpose is to allow someone to pause all activities of a certain type

What if I want to pause all activities regardless of type? In unpause that is a boolean, not here?

The current implementation explicitly pauses just the running instances of an activity and doesn't pause any future instances.

So how can I pause future instances without being forced to provide a type? Or pause all?

We should be consistent here. Why does whether I want my predicate to apply to all or running or future have to do with what that predicate is?

Copy link
Copy Markdown
Contributor Author

@gow gow Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we conflating rules and individual activity operations here? My understanding of these individual activity operations is that they operate on specified activity/s. Activities are identified by id or by type in the API input. When identified by id it's implicit that they are currently running activities (so no additional options apply here). When identified by type they could be currently running or the ones started in future. So we need additional options (running, future, all). So actions differ based on how you identify the activity.

So how can I pause future instances without being forced to provide a type?

If we want to do this via activity pause/unpause APIs we can support wildcard * to identify all activities? The question is how do we identify/represent "all activities" in these APIs.

Workflow rules have predicates that are evaluated and automated action is taken. This is were typically actions are independent of predicates.
Let me know if my understanding of how these APIs and rules would be employed is incorrect.

Copy link
Copy Markdown
Contributor

@cretz cretz Jul 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of these individual activity operations is that they operate on specified activity/s.

Why can't I apply activity operations on all activities for a workflow? Why are we limiting to only certain IDs or types?

When identified by id it's implicit that they are currently running activities (so no additional options apply here). When identified by type they could be currently running or the ones started in future.

Regardless of how identified (id, type, all, selector in the future, etc), I should be able to specify an activity state they should apply to.

Can I pause all running activities? Can I pause all future activities? Can I pause all activities regardless of whether future or running?

If we want to do this via activity pause/unpause APIs we can support wildcard * to identify all activities?

No, unpause already has this, a boolean to apply to all. We're strangely inconsistent with ourselves on how to apply something to all activities (or all running or all future). We can become consistent by adding a boolean here if we want for "all" or we can change that one.

Copy link
Copy Markdown
Contributor

@cretz cretz Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might I want to choose all, running, or future for more than just activity type? For instance, how do I pause all future activities regardless of type? Whether the predicate applies to all, running, or future doesn't seem to relate to the predicate itself.

Copy link
Copy Markdown
Contributor Author

@gow gow Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are part of single activity operation APIs (pause/unpause/reset). Pausing all activities within a workflow is under the scope of workflow pause (which is being speced currently).

Copy link
Copy Markdown
Contributor

@cretz cretz Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about only pausing future activities regardless of type? It is confusing to only be able to do some activity pausing via workflow pause. I don't want to pause my workflow, I want to pause activities. I think we need to think about this from a devx/user POV. I want to pause all activities, I want to pause running activities, I want to pause future activities, I want to pause running activities of a certain type, etc etc need to be a cohesive, easy to understand interface. To tell someone half of that is done elsewhere is not good devx.

Copy link
Copy Markdown
Contributor Author

@gow gow Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. All cases you mentioned are covered except being able to specify "all" activity types. Like mentioned above, we can add a wildcard * to allow users to indicate that they want to pause all activities. Thoughts?

Copy link
Copy Markdown
Contributor

@cretz cretz Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mentioned in other reply, we should be consistent with unpause. Why have a boolean for all there and a special string for all here? And why should I even have to set a type at all if I don't care about type?

Copy link
Copy Markdown
Contributor

@cretz cretz Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unpause has a concept of type also, does this same concern apply there? Also, unpause has a concept of "all" there too, does this same feature need to apply here?

Copy link
Copy Markdown
Contributor Author

@gow gow Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I'm not planning to support selectively unpausing activity by type. We can add it in the future if needed?

Copy link
Copy Markdown
Contributor

@cretz cretz Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already support unpausing activity by type today, see the API. So today we support pausing and unpausing activities with the same predicate capabilities. This now makes pause selection different than unpause selection and therefore confusing from a user understanding POV.

Copy link
Copy Markdown
Contributor Author

@gow gow Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree about the inconsistency between pause and unpause API. My initial reasoning was that there is no use case to selectively unpause just running, future, all (when unpausing by type). So I left it out. I'll add similar unpause options to unpause API as well.

Copy link
Copy Markdown
Contributor

@cretz cretz Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add similar unpause options to unpause API as well.

👍 Will be interested in what this looks like when done. I think you'll find that the enumerate state of the activity is wholly unrelated to its type and therefore PauseByType doesn't make sense because it doesn't sense to combine the two since they are distinct filters.

Copy link
Copy Markdown
Contributor Author

@gow gow Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. Added PauseOptionActivityState (open to other name suggestions). Now one can pause/unpause any combination of

  • activity: {id, type, all}
  • activity_state: {running, future, both}

There are some invalid combinations like {activity: id, activity_state: future}. But these will be handled in the API implementation.

Copy link
Copy Markdown
Contributor

@cretz cretz Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me!

PAUSE_BY_TYPE_OPTION_UNSPECIFIED = 0;
PAUSE_BY_TYPE_OPTION_ALL = 1;
PAUSE_BY_TYPE_OPTION_RUNNING = 2;
PAUSE_BY_TYPE_OPTION_FUTURE = 3;
}
}

message PauseActivityResponse {
Expand Down Expand Up @@ -1981,6 +1997,7 @@ message ResetActivityRequest {
// They are part of the first SCHEDULE event.
bool restore_original_options = 9;


}

message ResetActivityResponse {
Expand Down Expand Up @@ -2208,8 +2225,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;
Expand Down
Loading