-
Notifications
You must be signed in to change notification settings - Fork 201
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
Add async activity example #342
Conversation
|
Include a very basic async activity example. This is the same example from the samples-java repository, ported to Go.
// Set this to 1 to make the activities run one after the other (note | ||
// how both are scheduled at the same time, but ActivityTaskStarted | ||
// differs). | ||
MaxConcurrentActivityExecutionSize: 2, | ||
// Set this to 0.5 to create some delay between when activities are | ||
// started. Note that in this case, the started time does not differ. | ||
// Only the completed time is different. | ||
WorkerActivitiesPerSecond: 2, |
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 think these settings are related to the sample and don't need to be set for this sample
} | ||
ctx = workflow.WithActivityOptions(ctx, ao) | ||
|
||
// Start activities asynchronously. |
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 not what we call an "async activity" in Temporal terms. All activities are async by default. In Temporal there is an "async activity completion" concept at https://docs.temporal.io/activities#asynchronous-activity-completion this would get confused with. This sample is just showing how to do parallel or concurrent activities.
You can add a sample for "parallel activities" if you'd like (assuming we don't already have such a sample, though it's very trivial), though I would recommend using a selector to wait for first complete since that's a better demonstration for most users.
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 indeed wasn't meant to be async completion (the expense example showcases that). I was going through the README and I saw:
Pending examples
Mostly examples we haven't yet ported from https://github.com/temporalio/samples-java/
Async activity calling: Example to be completed
which I thought meant essentially porting this code from the java example:
public String getGreeting(String name) {
/*
* This is our workflow method. We invoke the composeGreeting method two times using
* {@link io.temporal.workflow.Async#function(Func)}.
* The results of each async activity method invocation returns us a
* {@link io.temporal.workflow.Promise} which is similar to a Java {@link java.util.concurrent.Future}
*/
Promise<String> hello = Async.function(activities::composeGreeting, "Hello", name);
Promise<String> bye = Async.function(activities::composeGreeting, "Bye", name);
// After calling the two activity methods async, we block until we receive their results
return hello.get() + "\n" + bye.get();
}
I suppose since in Java there is also a different syntax which allows calling an activity and waiting for it to complete synchronously, it helps to have both samples. But given that in Go you always get a Future when calling ExecuteActivity
, not sure if having this sample as well adds as much value as in the Java repository (you could pretty much instead use the hello example and build upon that in this case). I made this PR pretty mainly because of that readme note.
though I would recommend using a selector to wait for first complete since that's a better demonstration for most users
That would end up making this example more or less like pickfirst
, so I guess in that case I'd be questioning the purpose of this sample.
Thus, I'd suggest dropping the sample, but maybe keep this PR for removing that section from the README, since there is the pickfirst 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.
Yeah, removing that whole "pending" section of the README may be best. @Quinn-With-Two-Ns - thoughts?
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 I would just remove the "pending" from the README
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.
Since that's quite different from what I started with, I made #343 as a result to remove the "pending" and will close this PR afterwards.
Closing since new changes have been moved to #343 |
Include a very basic async activity example. This is the same example from the samples-java repository, ported to Go.
What was changed
Adapted the HelloAsync activity from Java samples. I saw that this is to be ported -- not sure if that is still relevant or not, but it took very little to write it anyway.
Why?
There are similar examples, e.g. pickfirst, but this is demonstrating the concept at the most basic level, and I thought it's a good place to experiment with worker options (see worker option comments).
Checklist
How was this tested:
See tests.
Any docs updates needed?
Updated README accordingly (as I thought it would be best).