💥 feat(sdk): Support Standalone Activities - #1402
Conversation
49d89b7 to
dd726c2
Compare
dd726c2 to
bbcf171
Compare
Sushisource
left a comment
There was a problem hiding this comment.
Awesome to see this ready!
| /// To support deserialization of transmitted payloads, the object internally stores a reference | ||
| /// to the client used to make the request. For this reason, this type is parametrized with client | ||
| /// type. The client reference can be dropped by calling [`simple`](Self::untyped). |
There was a problem hiding this comment.
This comment looks like it's not accurate any more since just a dataconverter, not a whole client, is stored (which is good).
| /// True if activity input is present. | ||
| /// See [`ActivityDescribeOptions::include_input`](crate::ActivityDescribeOptions::include_input). | ||
| /// Use [`input`](Self::input) or [`raw_input`](Self::raw_input) to retrieve it. | ||
| pub fn has_input(&self) -> bool { | ||
| self.raw_input.is_some() | ||
| } |
There was a problem hiding this comment.
Nit: IMO we don't need this - easy enough to just call raw_input().is_some()
There was a problem hiding this comment.
I'd keep these methods for symmetry with heartbeat_details/last_failure, also I don't like making users call raw_* methods when they're not interested in raw payloads.
| /// True if activity outcome is present. | ||
| /// See [`ActivityDescribeOptions::include_outcome`](crate::ActivityDescribeOptions::include_outcome). | ||
| /// Use [`outcome`](Self::outcome) or [`raw_outcome`](Self::outcome) to retrieve it. | ||
| pub fn has_outcome(&self) -> bool { |
| pub enum ActivityResultError { | ||
| /// Activity execution did not complete successfully. | ||
| #[error("Activity failed: {0}")] | ||
| ActivityFailed(#[source] IncomingError), |
There was a problem hiding this comment.
With WorkflowGetResultError we distinguish Failed / timedout / cancelled etc at the top level, probably makes sense to do that here too.
| .await? | ||
| .into_inner(); | ||
|
|
||
| // If resp.outcome.value is None, poll again |
There was a problem hiding this comment.
The server uses long-poll for this request.
| /// Start a standalone activity and wait for its result. | ||
| /// | ||
| /// Equivalent to `start_activity(...).await?.result()`. | ||
| pub async fn execute_activity<A>( |
There was a problem hiding this comment.
We don't have execute_workflow, personally not a big fan of these simple sugars.
There was a problem hiding this comment.
Going to remove it for now since it can always be added later.
| /// | ||
| /// To get a handle that can be used to complete an activity asynchronously, | ||
| /// see [`get_async_activity_handle`](Self::get_async_activity_handle). | ||
| pub fn get_untyped_activity_handle( |
There was a problem hiding this comment.
Same here, I think just calling get_activity_handle<UntypedActivity>(...) is fine and we can say how to do that in the docstring.
There was a problem hiding this comment.
get_activity_handle requires the activity as argument. get_untyped_activity_handle does not. get_activity_handle::<UntypedActivity> would not work.
There was a problem hiding this comment.
Ah, yeah, I just wrote it wrong though - you can still just pass UntypedActivity as the arg.
...though I guess that does require you to pointlessly specify a name, so, that being the case I'm fine with this then
| /// Activity options. Specifying at least one of them is required, but specifying both is also | ||
| /// allowed. Note that this type does not cover all available timeout options for an Activity. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum ActivityCloseTimeoutOptions { |
There was a problem hiding this comment.
IMO no reason to add the "options" postfix here
| pub id: String, | ||
| /// Combined schedule-to-close and start-to-close timeout options. At least one of them is | ||
| /// required. | ||
| #[builder(setters(name = combined_close_timeouts, vis = ""))] |
There was a problem hiding this comment.
This is part of the start_fn when starting from a workflow, and it should be here too IMO, that simplifies a lot of other stuff.
The "combined" wording in the docstring / setter (which can go away if we make it part of constructor) diverges from the workflow version too, I think we can keep it the same.
There was a problem hiding this comment.
This setter is private. The idea is that the user does not use combined_close_timeouts or ActivityCloseTimeoutOptions directly - instead, they use either schedule_to_close_timeout or start_to_close_timeout to set one of them, or close_timeouts to set both.
There was a problem hiding this comment.
Ah. I didn't see it was private. Regardless... I still think it makes sense to just do exactly what the workflow version does. Passing the right variant to close_timeouts is easy
| @@ -0,0 +1,15 @@ | |||
| use std::error::Error; | |||
|
|
|||
| pub(crate) fn try_into_or_box_err<A, B, E, MapErr>( | |||
There was a problem hiding this comment.
Since it's only used from client lib, can probably just stick it in there as private.
💥 Breaking changes
ActivityInfofields related to the containing workflow are made optional.async_activity_handle::ActivityIdentifier::ByIdis split intoByIdWorkflowandByIdStandalone.What was changed
Why?
Feature request: temporalio/features#706
Checklist
Closes [Feature Request] Support standalone activities #1070
How was this tested:
Added tests in
crates/sdk-core/tests/integ_tests/standalone_activity_tests.rsSAA guide for Rust needs to be added here: https://docs.temporal.io/standalone-activity