Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Conversation

Ozoniuss
Copy link

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

  1. How was this tested:
    See tests.

  2. Any docs updates needed?
    Updated README accordingly (as I thought it would be best).

@CLAassistant
Copy link

CLAassistant commented Apr 13, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Include a very basic async activity example. This is the same example
from the samples-java repository, ported to Go.
Comment on lines +24 to +31
// 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,
Copy link
Member

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.
Copy link
Member

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.

Copy link
Author

@Ozoniuss Ozoniuss Apr 16, 2024

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?

Copy link
Member

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?

Copy link
Contributor

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

Copy link
Author

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.

@Ozoniuss
Copy link
Author

Closing since new changes have been moved to #343

@Ozoniuss Ozoniuss closed this Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants