Add Nexus request and response payloads to cancel activities#708
Add Nexus request and response payloads to cancel activities#708
Conversation
rkannan82
force-pushed
the
kannan/activity-cancel
branch
3 times, most recently
from
4b2e0d1 to
54edb8e
Compare
temporal/api/worker/v1/message.proto
Outdated
| // Container for batching multiple control tasks delivered to a worker in one Nexus operation. | ||
| message WorkerControlTasks { | ||
| repeated WorkerControlTask tasks = 1; | ||
| } | ||
|
|
||
| // A single control task for a worker. | ||
| message WorkerControlTask { | ||
| // Timestamp when this task was created. | ||
| google.protobuf.Timestamp create_time = 1; | ||
|
|
||
| oneof task { | ||
| CancelActivityTask cancel_activity = 2; | ||
| } | ||
| } | ||
|
|
||
| // Request to cancel running activities on this worker. | ||
| message CancelActivityTask { | ||
| // The workflow execution that owns the activities. | ||
| temporal.api.common.v1.WorkflowExecution workflow_execution = 1; | ||
| // The scheduled event IDs of activities to cancel. | ||
| // If empty, cancel all activities for this workflow running on this worker. | ||
| repeated int64 scheduled_event_ids = 2; | ||
| // Human-readable reason for cancellation. | ||
| string reason = 3; | ||
| } |
There was a problem hiding this comment.
Arguably we shouldn't have to use proto for this, but even if we do, can these be defined at the same time as the exact Nexus service?
There was a problem hiding this comment.
Sorry, what did you mean by "define at the same time as Nexus service"? I have now documented the nexus service and command name.
temporal/api/worker/v1/message.proto
Outdated
| } | ||
|
|
||
| // Container for batching multiple control tasks delivered to a worker in one Nexus operation. | ||
| message WorkerControlTasks { |
There was a problem hiding this comment.
If this is meant to be the Nexus input/request, it should be named as such and you need a output/response of an operation (even if it is empty at first). You should also define even what the service name and operation are expected to be. Having an input model only with no output model, no service name, and no operation name makes me concerned this isn't fully designed yet.
There was a problem hiding this comment.
I think this should be a new file, as it's defining a new nexus service, not the typical grpc service, see the previous discussion here, #622 (comment),
specifically what spencer said
For now I would suggest moving these commands into another proto file inside worker/v1 like worker_nexus_service_commands.proto and a big block comment at the top can say the service name, and the operation names, and how they correspond to workflowservice rpcs
I think we were all on board with Yuri's design of worker_nexus_service_commands.proto, and we should do something similar here where the new file contains the block comment giving more information about the new nexus service
// (--
/////////////////////////////////////////////////////////////////////
// This file contains:
// - Conventions between server and worker.
// - Definitions for commands and payloads for server-worker communication via Nexus
... and so on
There was a problem hiding this comment.
Please take another look. I have incorporated Yuris' PR style in here.
temporal/api/worker/v1/message.proto
Outdated
| } | ||
|
|
||
| // Container for batching multiple control tasks delivered to a worker in one Nexus operation. | ||
| message WorkerControlTasks { |
There was a problem hiding this comment.
IMO input to the worker Nexus service should provide the worker instance key the control call is targeting. This is a shared Nexus worker.
On a side note, I think we will want to implement a form of this end-to-end before we merge what we think the models needed will be.
There was a problem hiding this comment.
agree that we should have something in server and SDK working (in branches) with this definition before we merge
Add WorkerControlTasks, WorkerControlTask, and CancelActivityTask to worker/v1/message.proto for pushing control messages to workers via Nexus control queue.
rkannan82
force-pushed
the
kannan/activity-cancel
branch
2 times, most recently
from
afb40a2 to
b920471
Compare
rkannan82
changed the title
- Move Nexus command payloads to worker_nexus_service_commands.proto - Rename to CancelActivitiesRequestPayload/ResponsePayload - Use task_tokens instead of workflow_execution + scheduled_event_ids - Add file-level documentation for Nexus conventions
rkannan82
force-pushed
the
kannan/activity-cancel
branch
from
b920471 to
84c0bac
Compare
3 participants
Added Nexus request and response protos for canceling workflow activities.
This is the first use case for sending commands to worker using Nexus queue. It allows us to cancel in-flight activities when a user cancels the workflow (even when activity heartbeat is not enabled).