-
Notifications
You must be signed in to change notification settings - Fork 5
Simple activity, context propagation, and message passing samples #20
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
|
|
||
| module ActivitySimple | ||
| class MyWorkflow < Temporalio::Workflow::Definition | ||
| def execute |
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.
Please forgive reviewing API in the samples PR.
I would have reached for run instead of execute -- that has consistency with other SDKs, as well as threading APIs such as those of Java and Python etc. Is there a reason to do with the Ruby ecosystem for using execute?
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.
Please forgive reviewing API in the samples PR.
No prob, this is a good place. And I am happy to see others digging in to Ruby.
Based on some ChatGPT ecosystem querying, it did seem execute was more preferred for background-job-like things. There is also perform. It is also what is used in the past Ruby SDK implementation and the Coinbase Ruby SDK implementation with success/familiarity. We use "run" as the attribute/annotation/decorator name, but in those other SDKs you can choose any method name though our samples often choose "run". But for Ruby, this is an actual abstract method and if made sense to match what was already there in ecosystem and existing Temporal Ruby.
| # Use an instance for the stateful DB activity, other activity we will pass | ||
| # in as class meaning it is instantiated each attempt | ||
| db_client = ActivitySimple::MyActivities::MyDatabaseClient.new | ||
| select_from_db_activity = ActivitySimple::MyActivities::SelectFromDatabase.new(db_client) |
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.
Isn't .new just shorthand for .new()? But then both would be instances which contradicts what you say above -- my ruby understanding must be failing me.
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.
Oh ignore I see, only one of those is actually an activity.
|
|
||
| # Create worker with the activities and workflow | ||
| worker = Temporalio::Worker.new( | ||
| client:, |
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 syntax shorthand for client: client, a bit like TS/Rust?
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
| yield | ||
| ensure | ||
| # Put them all back, even if they were nil | ||
| orig_values.each { |key, val| Thread.current[key] = val } |
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 would be setting a better example to create a single container within the thread-local namespace to hold the application-logic keys, rather than having them all at the the top-level.
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.
The user of the interceptor can do this (single key with a hash of all values needed), this interceptor is just a general purpose key copier. Would actually probably encourage users of this interceptor to not use multiple keys.
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 didn't understand your response here.
| Temporalio::Workflow.logger.info("Workflow called by user: #{Thread.current[:my_user]}") | ||
|
|
||
| # Wait for signal then run activity | ||
| Temporalio::Workflow.wait_condition { @complete } |
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.
Neat. But, won't the logger.info statement be called every time the condition is evaluated? That would be particularly undesirable since it is saying that a user query is being handled.
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.
Oh, no, maybe I don't understand what @complete is. I thought it was a reference to the method which is used as a query hander. What is it? Does @foo just always evaluate to nil for any value of "foo" unless it's been set?
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 don't understand what @complete is
@complete is a Ruby attribute. That is class state, akin to self.complete in Python. However, unlike Python, accessing an uninitialized Ruby attribute returns nil instead of errors. Also unlike Python, Ruby attributes are only visible within the class (i.e. they are "private") and you have to use other things to expose them.
Does
@foojust always evaluate tonilfor any value of "foo" unless it's been set?
Yup, which makes Ruby workflows so nice and easy to write and really Ruby is a very pretty language altogether, so much so that in my biased opinion, workflows written in Ruby will be the most dev friendly to read/write of all.
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 agree they're very nice to read. I don't think that having a poor typing story is the most dev-friendly for writing.
| puts 'Executing workflow with user "Alice"' | ||
| handle = client.start_workflow( | ||
| ContextPropagation::SayHelloWorkflow, | ||
| 'Bob', |
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.
Are we restricting to 0- or 1-arity in Ruby as we wish we had done for others?
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.
No, and we didn't wish to do this for the others, we just discourage multi-param. Every SDK needs to support multiple parameters, even if we encourage a single one.
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.
Not sure about that
| module MyActivities | ||
| # Fake database client | ||
| class MyDatabaseClient | ||
| def select_value(table) |
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.
probably worth adding a short sleep here to emphasize that we're faking I/O (as you do below in the message-passing sample)
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.
Meh, not really needed that much (the async concepts of whether a sleep is present or not is different in Ruby and doesn't really mean anything) and wasn't done at https://github.com/temporalio/samples-dotnet/blob/main/src/ActivitySimple/MyActivities.cs either where it doesn't matter there either. But I may add this.
| # workflow_query | ||
| # def language | ||
| # @language | ||
| # end |
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.
👍
| def approve(input) | ||
| # A signal handler mutates the workflow state but cannot return a value. | ||
| @approved_for_release = true | ||
| @approver_name = input['name'] |
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.
Out of interest is assignment an expression in ruby?
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, so technically this method does return a value. The last statement of every method is the return value of the method. But whether that's meaningful or not depends on docs and context (so you know a workflow signal handler's return value is discarded).
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.
OK good to know. So I guess there's nothing stopping a user from writing a signal handler that returns a value and using it as a normal method, in addition to as a signal handler.
| # will fail the update. The WorkflowExecutionUpdateAccepted event will still be added to history. (Update | ||
| # validators can be used to reject updates before any event is written to history, but they cannot be async, | ||
| # and so we cannot use an update validator for this purpose.) | ||
| raise Temporalio::Error::ApplicationError, "Greeting service does not support #{new_language}" unless greeting |
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't say I immediately love the style of
raise ErrorThatProbablyDidntHappen unless something_which_is_probably_trueIs it unquestioned Ruby idiom or is it acceptable to use an if statement in this situation?
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 is an unquestioned Ruby idiom. So much so that if you tried to use if ! instead of unless here or you tried to use a block instead this suffixed inline conditional, the style detector (Rubocop) would ding you. And in most cases the plugins in the IDE including the one I use, would auto-correct it to this if it'll fit on the line.
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.
😢
dandavison
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.
Nice! Did a pass -- my first real pass at Ruby SDK -- and asked some questions.
What was changed
activity_simplesample based on https://github.com/temporalio/samples-dotnet/tree/main/src/ActivitySimplecontext_propagationsample based on https://github.com/temporalio/samples-dotnet/tree/main/src/ContextPropagationmessage_passing_simplesample based on https://github.com/temporalio/samples-python/tree/main/message_passing/introductionNote, this will not pass CI until the next Ruby SDK release
Checklist