Support user metadata in child workflow, timer, and activity #846
Conversation
There was a problem hiding this comment.
This looks great! May want to wait for @Sushisource to approve before merging.
| let mut s = Self::new(attribs); | ||
| OnEventWrapper::on_event_mut(&mut s, TimerMachineEvents::Schedule) | ||
| .expect("Scheduling timers doesn't fail"); | ||
| let cmd = Command { | ||
| command_type: CommandType::StartTimer as i32, | ||
| attributes: Some(s.shared_state().attrs.into()), | ||
| user_metadata: Default::default(), | ||
| attributes: Some(s.shared_state().attrs.clone().into()), |
There was a problem hiding this comment.
This clone shouldn't be needed, and we don't want to clone the entire attributes for commands, generally speaking, as it can have a lot of stuff in it (though, for timer, that's likely not the case)
There was a problem hiding this comment.
Without the clone, I get the error
cannot move out of a shared reference
move occurs because value has type `temporal_sdk_core_protos::coresdk::workflow_commands::StartTimer`, which does not implement the `Copy` traitrustc[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic message [0]?0#file:///Users/andrewyuan/temporal/core/core/src/worker/workflow/machines/timer_state_machine.rs)
Looks like we clone the attrs of Activity https://github.com/yuandrew/sdk-core/blob/dd37e2946a385090ded0fc3312180027002d4b16/core/src/worker/workflow/machines/activity_state_machine.rs#L134
There was a problem hiding this comment.
hmm, curious why we're even getting this error in the first place.. the line I added for user_metadata doesn't involve s directly, but maybe because I'm cloning attribs.summary, it messes with the borrow checker, when s is created?let mut s = Self::new(attribs);
Is there a different workaround here, or would we have to clone (or implement Copy) here?
There was a problem hiding this comment.
Ah, yeah I didn't see it getting moved later. This clone is fine to keep then, but we could change the line above to also take the summary.
And yeah, in general I want to reduce how many places lots of various strings get cloned, like the place you linked, but that's a bigger project. We may not ever do it since ultimately it's not that big of a deal but, I want to avoid adding more spots where we unnecessarily clone.
| pub fn timer<T: Into<TimerOptions>>(&self, opts: T) -> impl CancellableFuture<TimerResult> { | ||
| let opts: TimerOptions = opts.into(); |
There was a problem hiding this comment.
Ah nice, you went with this option, cool. Probably made it much easier to avoid updating every test under the sun lol
sdk/src/workflow_context/options.rs
Outdated
| @@ -68,6 +68,9 @@ pub struct ActivityOptions { | |||
| pub cancellation_type: ActivityCancellationType, | |||
| /// Activity retry policy | |||
| pub retry_policy: Option<RetryPolicy>, | |||
| /// Summary of the activity | |||
| /// NOTE: Experimental | |||
There was a problem hiding this comment.
We don't really need to mark it experimental. Everything in Rust SDK is. (same with other spots)
| summary: attribs.static_summary.clone(), | ||
| details: attribs.static_details.clone(), |
There was a problem hiding this comment.
This is a little nitpicky, but, instead of cloning here, since these values aren't used by start_child_workflow_cmd_to_api, let's instead https://doc.rust-lang.org/std/mem/fn.take.html the string out and we avoid a clone
What was changed
added
static_summaryandstatic_detailsto child workflowadded
summaryto timer and activityand plumbed all new fields to user_metadata in command sent to server.
Why?
This allows individual SDK languages to pass this information to the server, allowing it to show up in the UI.
Checklist
Closes [Feature Request] Support user metadata in child workflow, timer, and activity #830
How was this tested:
Wrote new tests to validate metadata is being passed to server