Standalone Activity -- unify Describe and GetResult methods #651
Conversation
dandavison
force-pushed
the
standalone-activity-unify-describe-and-get-result
branch
from
33418ae to
9a0db0c
Compare
| // This replaces both DescribeActivityExecution and GetActivityExecutionResult with a single | ||
| // flexible endpoint that can handle both synchronous queries and long polling for both | ||
| // info updates and completion. | ||
| rpc GetActivityExecutionStatus (GetActivityExecutionStatusRequest) returns (GetActivityExecutionStatusResponse) { |
There was a problem hiding this comment.
No comment on the rest of the PR, but doesn't this method provide more than just the status? Status is but one part of the info returned here (and it seems by default we don't even return the info/status)
There was a problem hiding this comment.
Agreed. Discussed with @bergundy, @fretz12, @maciejdudko. Changed it to DescribeActivityExecution
There was a problem hiding this comment.
It is a bit strange that describe by default (e.g. GET https://my-cluster/namespaces/my-namespace/activities/my-activity) doesn't actually return anything but run_id by default unlike our other forms of describe (though of course the SDK will hopefully maintain consistency/expectations even if our API doesn't).
There was a problem hiding this comment.
Valid concern. I propose we change the flag to exclude_info to make it opt-out.
There was a problem hiding this comment.
Agreed. It is now:
// Exclude the info field from the response.
bool exclude_info = 4;
// Include the input field in the response.
bool include_input = 5;
// Include the outcome field in the response.
bool include_outcome = 6;
dandavison
force-pushed
the
standalone-activity-unify-describe-and-get-result
branch
2 times, most recently
from
906dd19 to
9f0f131
Compare
dandavison
force-pushed
the
standalone-activity-unify-describe-and-get-result
branch
2 times, most recently
from
19b9a57 to
b27dd73
Compare
dandavison
force-pushed
the
standalone-activity-unify-describe-and-get-result
branch
from
b27dd73 to
14d98bf
Compare
| message DescribeActivityExecutionRequest { | ||
| string namespace = 1; | ||
| string activity_id = 2; | ||
| // Activity run ID, targets the latest run if run_id is empty. |
There was a problem hiding this comment.
Why did you remove this comment?
There was a problem hiding this comment.
This was unintentional. Reinstated.
| bool include_info = 4; | ||
| bool include_input = 5; | ||
| bool include_outcome = 6; |
| bytes long_poll_token = 5; | ||
| message NoWaitOptions {} | ||
| message StateChangeWaitOptions { | ||
| bytes long_poll_token = 1; |
There was a problem hiding this comment.
We should require the run ID to be specified if the long poll token is present.
| // - Get activity outcome synchronously if completed (when include_outcome is true) | ||
| // - Long poll for activity completion (when wait_outcome is true) | ||
| // | ||
| // This replaces both DescribeActivityExecution and GetActivityExecutionResult with a single |
There was a problem hiding this comment.
This comment isn't relevant since we removed GetActivityExecutionResult.
There was a problem hiding this comment.
Thanks, that text was unintentionally included. Removed.
dandavison
deleted the
standalone-activity-unify-describe-and-get-result
branch
…io#651) Introduce a unified method for polling / long-polling an activity execution for status and/or outcome.
Introduce a unified method for polling / long-polling an activity execution for status and/or outcome.
This is a proposal to address the design discussion at https://github.com/temporalio/api/pull/640/files#r2414002340. It introduces a unified method for polling / long-polling an activity execution for status and/or outcome.