-
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?
Changes from all commits
fba10c6
1cbb1d9
4ec0859
e374e87
e8e4115
88451e9
853562f
2f355b9
ffecc00
0459c62
f2da163
3791149
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ use crate::data_converters::{TemporalDeserializable, TemporalSerializable}; | |
| /// Implement on a marker struct to define an activity. | ||
| pub trait ActivityDefinition { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| /// Type of the input argument to the workflow | ||
| type Input: TemporalDeserializable + 'static; | ||
| type Input: TemporalDeserializable + TemporalSerializable + 'static; | ||
| /// Type of the output of the workflow | ||
| type Output: TemporalSerializable + 'static; | ||
|
|
||
|
|
||
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.
Arguably this should live alongside the rest of the activity code IMO (i.e. in
temporal_sdk::activities). Same for workflow definition (and workflow signal definition, workflow update definition, and workflow query definition).But the more I think about it, the more I think we have misnamed some things. I think it might clear it up if the "sdk" crate were actually named the "worker" crate or something. Arguably an SDK is made up of "runtime", "client", "worker", "converter", etc. If some of those are in common that can make sense, but to have an "SDK" crate not include most of what makes an SDK is a bit confusing 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.
Sure, I can see naming it worker. I don't think it maters all that much but it's a reasonable rename.
The definition makes sense in common because workflow definitions will have to live here so they can be used by the client. Having the kinds of definitions together makes 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.
Yup, the definitions being in here is what triggered my thoughts here. It makes sense to me that you need it in common for use by a client, all of which make up the SDK IMO and only the worker crate is needed by implementers.
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.
On second thought, I think I'd rather keep in named SDK and just re-export everything you might need, which I was intending to do anyway. But that can be done later on in the project when things are more settled.
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'll reserve comment until then