-
Notifications
You must be signed in to change notification settings - Fork 94
Nexus sync operations that send messages to an entity workflow #252
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
Conversation
3eab7c0 to
9fe1869
Compare
9538e1d to
86c40ea
Compare
86c40ea to
78b2f52
Compare
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.
Nothing blocking
|
|
||
| from temporalio import workflow | ||
|
|
||
| from message_passing.introduction import Language |
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 had traditionally had all samples self-contained in all of our SDKs (i.e. they didn't reference each other), but maybe that's no longer what we want?
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, let's allow this one to reference. IMO it is educational / instructive: it's showing users that nexus allows a "service" to be transplanted without changes and given a nexus interface callable from another namespace.
| supported_languages = await nexus_client.execute_operation( | ||
| GreetingService.get_languages, GetLanguagesInput(include_unsupported=False) | ||
| ) | ||
| print(f"supported languages: {supported_languages}") |
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 discourage use of print in workflows
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.
Thanks, changed to return log: list[str]
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 was under the impression we are only doing this "description" markdown on the "hello" Nexus sample? Do we need to start doing this for all samples?
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 I think all nexus endpoints in samples should have a description. The feature exists and we want to encourage users to use it, and it is nicely rendered in the UI.
|
|
||
| # 👉 This is a handler for a nexus operation whose internal implementation involves executing an | ||
| # update against a long-running workflow that is private to the nexus service. | ||
| @nexusrpc.handler.sync_operation |
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 theory an update can be long running and we should demonstrate async operation with it, but I understand if we need some kind of server side support for update callbacks or something for that. May be worth noting though, up to you.
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.
Added a note explaining that updates must be fast when used in a sync operation. Yes, async nexus operations backed by updates do not exist yet and would require server-side nexus callback support in the server-side update state machine.
See temporalio/features#664
message_passing/introductionsample for the entity workflow backing the handler.approvesignal does not terminate the workflow, as it does currently.