[DO NOT MERGE] Standalone Nexus Operations#685
[DO NOT MERGE] Standalone Nexus Operations#685bergundy wants to merge 22 commits intotemporalio:masterfrom
Conversation
|
|
||
| // The number of attempts made to start/deliver the operation request. | ||
| // This number represents a minimum bound since the attempt is incremented after the request completes. | ||
| int32 attempt = 9; |
There was a problem hiding this comment.
This is attempt to deliver the start request. Will we support overall operation retry in the future? Will this name be confusing if we do? Maybe we should call it start_attempt so that people will not confuse it with activity attempt which has a different meaning.
There was a problem hiding this comment.
I want to keep this for consistency with PendingNexusOperationInfo.
| string request_id = 19; | ||
|
|
||
| // Operation token. Only set for asynchronous operations after a successful StartOperation call. | ||
| string operation_token = 20; |
There was a problem hiding this comment.
I thought we said we didn't want to expose this to callers? They should only have one way of referencing their operations: their caller-side operation ID.
There was a problem hiding this comment.
It's still worth exposing this information as we do for workflow callers.
There was a problem hiding this comment.
I have no horse in this race, but I'm curious, why is it useful to have?
There was a problem hiding this comment.
It's useful for debugging and can be used in the direct Nexus APIs to reattach to the same operation (future capability).
| // The run ID of the operation, useful when run_id was not specified in the request. | ||
| string run_id = 1; | ||
|
|
||
| // Stage to wait for. The operation may be in a more advanced stage when the poll is unblocked. |
There was a problem hiding this comment.
I'm confused, is this the stage the original request sent? Or does it represent the current stage of the operation?
There was a problem hiding this comment.
The current stage. Let me fix the docstring.
| // Updated on terminal status. | ||
| int64 state_transition_count = 10; | ||
| // Updated once on scheduled and once on terminal status. | ||
| int64 state_size_bytes = 11; |
There was a problem hiding this comment.
Is this intentionally a field only present in list? It was mentioned for standalone activities that everything in list was expected to be in describe.
Also, for standalone activities it was mentioned there would be a tool that would make sure everything in list was also in describe result. Can we prioritize that? It's a lot of effort for me to have to continually confirm our assertion on every PR and find these issues since we chose not to reuse types.
There was a problem hiding this comment.
Just want to call this out that we don't have this guarantee for schedules or batch which are much older archetypes: https://github.com/temporalio/api/blob/master/temporal/api/schedule/v1/message.proto https://github.com/temporalio/api/blob/master/temporal/api/workflowservice/v1/request_response.proto#L1715-L1751.
I don't think this guarantee needs to be high priority but we should keep track of it because I do think that it is nice to have. Ideally the SDKs would allow the types to have completely different fields, there's no need to reuse the models here.
There was a problem hiding this comment.
Right, but this guarantee/promise was made as part of not reusing models knowing the SDK will need this guarantee. Was not expecting a "nice to have" guarantee when the promise/guarantee was made.
There was a problem hiding this comment.
Let's take this offline.
There was a problem hiding this comment.
Discussed offline, we will write a tool soon.
| // Response to a successful UnpauseWorkflowExecution request. | ||
| message UnpauseWorkflowExecutionResponse { } | ||
|
|
||
| message StartNexusOperationRequest { |
There was a problem hiding this comment.
Is it intentional we don't have Priority support?
There was a problem hiding this comment.
Yes, priorities only apply to durable matching queues, those are not used for nexus tasks.
# Conflicts: # openapi/openapiv2.json # openapi/openapiv3.yaml # temporal/api/errordetails/v1/message.proto # temporal/api/nexus/v1/message.proto # temporal/api/workflowservice/v1/request_response.proto # temporal/api/workflowservice/v1/service.proto
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| string blocked_reason = 7; | ||
|
|
||
| // A reason that may be specified in the CancelNexusOpertionRequest. | ||
| string reason = 8; |
There was a problem hiding this comment.
This was set to 24 and I changed it; I don't see why it should be 24.
There was a problem hiding this comment.
(I wonder why we don't have a linter for this?)
There was a problem hiding this comment.
Copy pasta probably. Thanks for catching this.
092ad00 to
d99cea3
Compare
|
|
||
| // How long this operation has been running for, including all attempts and backoff between attempts. | ||
| // Elapsed time from schedule_time to now for running operations or to close_time for closed | ||
| // operations, including all attempts and backoff between attempts. |
There was a problem hiding this comment.
Clarifies it works for running operations (as opposed to NexusOperationListInfo); correct me if that's wrong.
temporal/api/enums/v1/nexus.proto
Outdated
|
|
||
| // Status of a standalone Nexus operation. | ||
| // The status is updated once, when the operation is originally scheduled, and again when the operation reaches a | ||
| // terminal status. |
There was a problem hiding this comment.
Moved this to NexusOperationListInfo where it seems to be applicable; and only there?
There was a problem hiding this comment.
It's applicable here too.
There was a problem hiding this comment.
Oh, I see so UNSPECIFIED -> RUNNING -> [terminal state].
bergundy
left a comment
There was a problem hiding this comment.
@stephanos added a few more comments but this is almost good to merge AFAIC.
| } | ||
|
|
||
| // A link to a standalone Nexus operation execution. | ||
| message NexusOperationExecution { |
There was a problem hiding this comment.
Looks like the naming is inconsistent with BatchJob and WorkflowEvent. Neither of those use the term Execution. I'm on the fence whether this is acceptable or not.
| NEXUS_OPERATION_EXECUTION_STATUS_CANCELED = 4; | ||
| // The operation was terminated. Termination happens immediately without notifying the handler. | ||
| NEXUS_OPERATION_EXECUTION_STATUS_TERMINATED = 5; | ||
| // The operation has timed out by reaching the specified schedule-to-close timeout. |
There was a problem hiding this comment.
This could be any timeout now that we've added start-to-close and schedule-to-close.
| // Requesting to cancel an operation does not automatically transition the operation to canceled status, depending | ||
| // on the current operation status and the cancelation type used. | ||
| NEXUS_OPERATION_EXECUTION_STATUS_CANCELED = 4; | ||
| // The operation was terminated. Termination happens immediately without notifying the handler. |
There was a problem hiding this comment.
Note to future us: This comment may not be true if we add a Nexus equivalent of a "parent close policy".
| string blocked_reason = 7; | ||
|
|
||
| // A reason that may be specified in the CancelNexusOpertionRequest. | ||
| string reason = 8; |
There was a problem hiding this comment.
Copy pasta probably. Thanks for catching this.
| // This is the only timeout settable for a Nexus operation. | ||
| // (-- api-linter: core::0140::prepositions=disabled | ||
| // aip.dev/not-precedent: "to" is used to indicate interval. --) | ||
| google.protobuf.Duration schedule_to_close_timeout = 8; |
There was a problem hiding this comment.
Might as well add start_to_close_timeout and schedule_to_start_timeout already. Those are already supported by the server for workflow ops.
|
|
||
| // How long this operation has been running for, including all attempts and backoff between attempts. | ||
| // Elapsed time from schedule_time to now for running operations or to close_time for closed | ||
| // operations, including all attempts and backoff between attempts. |
What changed?
Server PR