.NET SDK - Phase 2 - Workflows - Initial Comment Period#76
.NET SDK - Phase 2 - Workflows - Initial Comment Period#76cretz merged 2 commits intotemporalio:masterfrom
Conversation
| present. | ||
| * Workflow queries are public methods with the non-inheritable `[WorkflowQuery]` attribute | ||
| * Can be in base class, but if overridden, must also have the attribute | ||
| * Method cannot return a `Task`, but can accept arguments |
There was a problem hiding this comment.
How hard would it be to add support for Task returning queries? I'm asking since we're talking about allowing starting local activities from update validators and queries.
There was a problem hiding this comment.
It wouldn't be that hard, but it adds confusion for users. Some languages actually can do better things.
In .NET for example where there is likely no sandbox and users can run async code quite easily from sync contexts on the global thread pool, we would be much better off just implicitly calling these as local activities already (or doing workflow task heartbeating) and leaving them as non-Task returning. Users can then choose to invoke async code if they really want, but we don't have the concept of local activities in the way.
There was a problem hiding this comment.
I agree with Chad. I don't think we should even call such things "local activities" if we allow it. We should just say "you can disable the sandbox and do stuff from queries if you really want, just don't mutate state, pinky swear". I don't think making it a "real" local activity really buys people anything, but does wildly complicate internals.
There was a problem hiding this comment.
I think the reason why we do want to leverage activities here is for dependency injection.
I don't know if we'll want to call the API local activity, I agree that is confusing.
I do see a reason to eventually support returning tasks and also using a mechanism supported by the worker so we can avoid triggering the deadlock detector or in generally blocking the workflow thread.
We don't need to design this now though.
I was just pointing out that this might need to change and wanted to understand how hard it would be to change when needed.
| public static Task ExecuteActivityAsync( | ||
| string activity, object[] args, ActivityStartOptions options) { /*...*/ } | ||
|
|
||
| public static Task<TResult> ExecuteActivityAsync<TResult>( | ||
| string activity, object[] args, ActivityStartOptions options) { /*...*/ } |
There was a problem hiding this comment.
I like how you avoided the ambiguity and only do multiple args with stringly typed activities.
There was a problem hiding this comment.
👍 This was also done client side for workflows, signals, and queries. This was also done in Python (kinda, in a slightly different way).
| * In client I call options `WorkflowStartOptions`, not only for `StartWorkflowAsync` but also for | ||
| `ExecuteWorkflowAsync`. I have a similar pattern here with activities and children. Should I continue this or should I | ||
| remove the `Start` word? |
There was a problem hiding this comment.
Both Java and TS call this WorkflowOptions, I don't see why we need to mention Start, there's nothing to disambiguate (ATM).
There was a problem hiding this comment.
Ok, I may make this change everywhere then.
| * `ReplayWorkflowAsync` is thread safe and we'll document this instead of making an overload of this that accepts an | ||
| iterator | ||
| * Unlike Python we choose not to provide the history on the `WorkflowReplayResult` because we don't aggregate for the | ||
| user (they have LINQ ways to run many in parallel and they can correlate as desired) No newline at end of file |
There was a problem hiding this comment.
As long as the experience without accepting an iterator is nice, I'm all of less APIs.
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
LGTM, didn't have any major comments.
| * Every workflow must have the non-inheritable `[Workflow]` attribute | ||
| * Optional `Name` positional property can be given. Default is the unqualified type name with the "I" prefix removed | ||
| if it's an interface and the next letter is also capital | ||
| * So what does `IDWorkflow` become? Yes, `DWorkflow` :-( Weighed as an acceptably rare cost for obvious benefit |
There was a problem hiding this comment.
I still maintain just having to type the name is better than the inevitable set of users that will run into this and be surprised.
There was a problem hiding this comment.
It's pretty normal to use the derived name by default (in .NET and in our SDKs). I don't think this is ever gonna matter in practice. This one gotcha is only for those that are intentionally violating .NET standards (something I've found is very rare for iface names).
| present. | ||
| * Workflow queries are public methods with the non-inheritable `[WorkflowQuery]` attribute | ||
| * Can be in base class, but if overridden, must also have the attribute | ||
| * Method cannot return a `Task`, but can accept arguments |
There was a problem hiding this comment.
I agree with Chad. I don't think we should even call such things "local activities" if we allow it. We should just say "you can disable the sandbox and do stuff from queries if you really want, just don't mutate state, pinky swear". I don't think making it a "real" local activity really buys people anything, but does wildly complicate internals.
|
|
||
| public void SetSignalHandler<T>(string name, Func<T, Task> handler) { /*...*/ } | ||
|
|
||
| public void RemoveSignalHandler(string name) { /*...*/ } |
There was a problem hiding this comment.
What happens with this and tryget (as well as for queries) if the handler was defined by [WorkflowSignal] ?
There was a problem hiding this comment.
The handler map is built from the attributes but technically is mutable through these methods. So yes, you can technically remove a handler added via an attribute. And of course getting a handler by name works no matter how that handler was added. In that sense attribute-based handlers (our preference) are just sugar on this. It doesn't make much sense for a user to remove an attribute-added handler IMO, but at least it's a simple to understand concept. This is the same way it works in Python atm too. I hope nobody ever calls dynamic handler things FWIW.
|
Added updates based on design session. Specifically:
Will merge early next week if no more feedback comes |
3 participants
.NET SDK - Phase 2 - Workflows - Initial Comment Period
This is the proposal for the second phase of the .NET SDK.