feat: Unified activity operator APIs#743
Conversation
spkane31
changed the title
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
The comments I put on the PauseActivityExecution request applies to all of the new APIs.
| }; | ||
| option (temporal.api.protometa.v1.request_header) = { | ||
| header: "temporal-resource-id" | ||
| value: "workflow:{workflow_id}" |
There was a problem hiding this comment.
This header is conditional based on whether the API is targeting a workflow or an activity.
There was a problem hiding this comment.
See what RespondActivityTaskCompletedById did here.
| post: "/namespaces/{namespace}/activities/{activity_id}/pause" | ||
| body: "*" | ||
| additional_bindings { | ||
| post: "/api/v1/namespaces/{namespace}/activities/{activity_id}/pause" | ||
| body: "*" |
There was a problem hiding this comment.
You're missing the workflow HTTP endpoint. You forgot to copy them from here: https://github.com/temporalio/api/pull/640/changes/cdfef8a8a17582faa966a55d360b6ec2cfd8a251..aa94d3c75ee0759e460fabcf2f7862d74b079055#diff-bded41be6e20a31fc62611d9e8ff0725e1646c662caf8b9ce4e333461b94fbf5.
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
Approved but please do not merge until you have a working server implementation just to be sure that nothing is missed.
| string namespace = 1; | ||
|
|
||
| // If provided, targets a workflow activity for the given workflow ID. | ||
| // If empty, target a standalone activity. |
There was a problem hiding this comment.
| // If empty, target a standalone activity. | |
| // If empty, targets a standalone activity. |
There was a problem hiding this comment.
Fixing in all four spots
| // If empty, target a standalone activity. | ||
| string workflow_id = 2; | ||
| // The ID of the activity to target. Must be provided for a standalone activity. | ||
| // Mutually exclusive with workflow_activity_type. |
There was a problem hiding this comment.
Looks like mention of workflow_activity_type in comments is stale -- it doesn't exist in any of them.
| // Mutually exclusive with workflow_activity_type. |
There was a problem hiding this comment.
Also making to all 4 requests
| // Resource ID for routing. Contains "workflow:{workflow_id}" for workflow activities or "activity:{activity_id}" for standalone activities. | ||
| string resource_id = 9; |
There was a problem hiding this comment.
Is resource_id something stale that you've moved away from in favor of the multiple business_id logic in the generated history client routing options?
| string workflow_id = 2; | ||
| // The ID of the activity to target. Must be provided for a standalone activity. | ||
| // Mutually exclusive with workflow_activity_type. | ||
| string activity_id = 3; |
There was a problem hiding this comment.
Is activity_id required now that we don't have workflow_activity_type?
There was a problem hiding this comment.
Yes, updating docs
|
|
||
| // Resource ID for routing. Contains "workflow:{workflow_id}" for workflow activities or "activity:{activity_id}" for standalone activities. | ||
| string resource_id = 10; | ||
| } |
There was a problem hiding this comment.
Is it a design decision that there's no "reason" for Reset?
There was a problem hiding this comment.
There is not a reason field on the existing ResetActivityRequest so the decision was to be consistent.
Sushisource
left a comment
Sushisource
left a comment
There was a problem hiding this comment.
Nothing blocking, just a few minor things
| string reason = 6; | ||
|
|
||
| // Used to de-dupe pause requests. | ||
| string request_id = 7; |
There was a problem hiding this comment.
Why add if message is deprecated?
There was a problem hiding this comment.
Deprecated but not going to be removed just yet. I added the request_id to have the same functionality for the legacy and new code paths.
|
|
||
| // If set, the activity options will be restored to the defaults. | ||
| // Default options are then options activity was created with. | ||
| // They are part of the first SCHEDULE event. |
There was a problem hiding this comment.
| // They are part of the first SCHEDULE event. | |
| // They are part of the first schedule event. |
The all caps to me implies that it exactly equals some event name, but it doesn't. I'd just lower case it or actually use the full-on event name
There was a problem hiding this comment.
👍 a few other spots I'll make this change
| // * if there are more retries left - the activity will be paused. | ||
| // For long-running activities: | ||
| // - activities in paused state will send a cancellation with "activity_paused" set to 'true' in response to 'RecordActivityTaskHeartbeat'. | ||
| // - The activity should respond to the cancellation accordingly. |
There was a problem hiding this comment.
A bit more detail on "accordingly" would be nice
| // Flags: | ||
| // | ||
| // 'jitter': the activity will be scheduled at a random time within the jitter duration. | ||
| // If the activity currently paused it will be unpaused, unless 'keep_paused' flag is provided. | ||
| // 'reset_heartbeats': the activity heartbeat timer and heartbeats will be reset. | ||
| // 'keep_paused': if the activity is paused, it will remain paused. | ||
| // |
There was a problem hiding this comment.
IMO no need to re-iterate flag docs from the request message
| // If the activity was paused while waiting for retry, it will be scheduled immediately (* see 'jitter' flag). | ||
| // Once the activity is unpaused, all timeout timers will be regenerated. | ||
| // | ||
| // Flags: |
There was a problem hiding this comment.
Same about not duping flag docs
4 participants
What changed?
Add
{Pause/Unpause/Reset/Update}ActivityExecutionRPCs and corresponding request/response pairs that will target both workflow embedded and standalone activities. These all strictly target a single activity by ID, if a workflow_id is specified on the request the operation targets a workflow activity.Why?
Unified support for activity operations.
Breaking changes
None
Server PR
Does not break server