-
Notifications
You must be signed in to change notification settings - Fork 103
SDK Worker Executes activities #1086
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rust-sdk-prerelease
Are you sure you want to change the base?
Conversation
1d870f1 to
6f5266e
Compare
6f5266e to
e374e87
Compare
f8c693c to
f2da163
Compare
| impl Worker { | ||
| // /// Create a new worker from an existing connection, and options. | ||
| // pub fn new(connection: Connection, options: WorkerOptions) -> Self {} | ||
| // TODO [rust-sdk-branch]: Not 100% sure I like passing runtime here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably my main open question on this PR. This is really only used for telemetry options, and worker heartbeat interval.
We could have worker heartbeat on connection (inside client) as discussed way back, and then only pass in telemetry explicitly... but not sure how worth it that is or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other SDKs, we have expected the connection to carry the runtime, the client to carry a connection, and the worker to therefore access the runtime that way upon instantiation. I admit I haven't dug into how one creates a client/connection in this SDK with a runtime, but that's where it should come from IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the connection only needs the metric meter. The whole runtime would have to move into common to have things work that way - but, that's not necessarily an issue. More the problem is that the idea of the runtime in general maybe feels a bit odd in Rust... but maybe that's not a huge issue. It holding onto a tokio runtime/handle is the main odd part. The other thing it holds onto is a tracing subscriber, which actually does make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the tracing subscriber not applicable to the client too (and other things on TelemetryInstance)? I think we can find the common above-client/worker things we need and put them in common and have worker instantiation just do what the assume tokio stuff does and not even expose core runtime at all.
But I suspect we may want a concept of an overarching instantiated thing (even if it has a global static default) and that may be a common runtime that carries telemetry stuff (so far, maybe more later). Granted that is different than the "core runtime" which is that + tokio runtime.
Either way we handle it, I would not expect an end user to ever have to import temporalio_sdk_core crate or even know it exists.
cretz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes, didn't do full review, just looking at API design
| pub fn activity<AD: ActivityDefinition>( | ||
| &self, | ||
| _activity: AD, | ||
| input: AD::Input, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if I have no input? I have seen some tricks done here in Rust for varying arity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just (), or (a, b, c) for multi args.
I could generate a bunch of versions of this function for different arities but I kinda hate that and it dirties up the docstrings. They all need different names too since there are no overloads. The tuples feels easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That you can accept tuples is great, but I am worried about the ambiguity between a single tuple arg and multi args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They can just serialize to the same thing. No real issue there. The fact that your "single tuple arg" happens to get serialized as multiple args shouldn't matter I don't think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can matter. There is a big difference between multiple scalar args and a single array of multiple values. I don't think you can even resolve the array ambiguity in some deserialization cases. I think you have to clearly define the difference between single param array and multiple params, and with the latter being rare (only for interop needs IMO), it can probably just be a specialized type.
| Ok(().into()) | ||
| pub(crate) struct SleepyActivities {} | ||
|
|
||
| #[activities] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show an example or put it on your TODO to demonstrate how to represent activities implemented in another language? Specifically, can someone have just a trait of activities or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use the macros on a function whose body is just unimplemented!(), or implement activity definition by hand on your own struct
I can add an example
| /// However, this trait may be implemented manually if desired. | ||
| /// | ||
| /// Implement on a marker struct to define an activity. | ||
| pub trait ActivityDefinition { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should consider making activity definition have an invoke method that can be invoked and allow name to accept self. In (some) other languages, activity definitions can be instantiated by a user, and the decorators/attributes/etc are just sugar for creating them (and so they can be read by a user too). Arguably for those performing untyped activity invocations, they can instantiate a struct that implements this and just provide the name (and the input/output types if they'd like).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's already the case here. You can just make a struct and implement activity definition. Or, you can use the macros on a function whose body is just unimplemented!() which is cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would definitely be interested in a sample/snippet showing how to impl and register activity definition. May be worth a helper struct/fn, though hopefully people don't use that approach to create activities much. I will say one place where we see this approach used a lot in .NET is for DI where they want to control lifetimes and inject deps and such to the invocation.
crates/sdk/src/activities.rs
Outdated
|
|
||
| impl ActivityDefinitions { | ||
| /// Registers all activities on an activity implementer that don't take a receiver. | ||
| pub fn register_activities_static<AI>(&mut self) -> &mut Self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really that useful? I think it isn't too much to ask all users to put #[activities] be on a struct (even if empty struct) and always require initializing that struct. If some methods (or all) don't happen to take self, that's ok. But it's still the same registration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah fair point, initializing it would typically be a no-op in that case
crates/sdk/src/activities.rs
Outdated
| self | ||
| } | ||
| /// Registers a specific activitiy that does not take a receiver. | ||
| pub fn register_activity<AD: ActivityDefinition + ExecutableActivity>(&mut self) -> &mut Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you must keep this approach, I would call this register_activity_static and change register_activity_with_instance to register_activity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One might expect worker stuff to be in workers, same as activity stuff is in activities
| } | ||
| } | ||
|
|
||
| async fn one_activity_wf(ctx: WfContext) -> WorkflowResult<Payload> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to come back and fix names like WfContext I think at some point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we've talked about that a bunch, I haven't forgotten.
crates/sdk/src/workflow_context.rs
Outdated
| pub fn activity_untyped( | ||
| &self, | ||
| activity_type: String, | ||
| input: Payload, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As unfortunate as it is, I think all SDKs have to have some way to both implement and invoke multi-param activities, even if it's not the suggested or typed approach. This is due to needs for dynamic and cross-language compatibility. One way may be a CompositeInput structure that accepts a collection of payloads (and change this to TryInto<Payload> or whatever, because arguably anything that is serializable should be able to be passed here). Arguably you don't even need this separate untyped overload and can allow users to invoke activity with a created-right-then-name-only definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, true, I can potentially just get rid of untyped
crates/sdk/src/workflow_context.rs
Outdated
| pub fn activity_untyped( | ||
| &self, | ||
| activity_type: String, | ||
| input: Payload, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder from a Temporal user POV if we should not be exposing literal prost-generated Payload but instead offer a specific Payload type of our own with any helpers we might want and such. This will also come in handy for people that may want to accept literal Payload which can be preferred in cases of dynamic activities/workflows or just lazy/deferred deserialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense but I'm not really going to deal with that until I flesh out the data conversion stuff
crates/sdk/src/workflow_context.rs
Outdated
| ) -> Result<impl CancellableFuture<ActivityResolution>, PayloadConversionError> { | ||
| // TODO [rust-sdk-branch]: Get payload converter properly | ||
| let pc = PayloadConverter::serde_json(); | ||
| let payload = pc.to_payload(&input, &SerializationContext::Workflow)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this, IMO we should consistently put context parameters first
crates/sdk/src/workflow_context.rs
Outdated
|
|
||
| /// Request to run an activity | ||
| pub fn activity( | ||
| pub fn activity<AD: ActivityDefinition>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer the verb form of this myself (i.e. start_activity), same for clients starting activities/workflows.
749e0c6 to
3791149
Compare
ctx.activity(MyActivities::my_activity, input, ActivityOptions::default());ClientnotConnection