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

Support executing local activities by name #397

Merged

Conversation

lminaudier
Copy link
Contributor

@lminaudier lminaudier commented Apr 1, 2021

What was changed:

The change replicates the behavior of ExecuteActivity for ExecuteLocalActivity to allow executing local activities by supplying funcs or func names.

In the process, ExecuteLocalActivity is using the activity registry to lookup the activity function by name when needed.

One question I have is that this could be potentially considered a breaking change because right now, it seems local activities executions don't seem to rely on the registry meaning that potentially, you don't have to register local activities in your worker? I still have to test if that is true and if so, if this is a documented behaviour. It seems that's indeed the case when I look at tests.

Why?

This PR attempts to fix #354 to allow executing local activities by using the activity name similarly to non local activities.

Checklist

  1. Closes issue: closes Allow string activity names in workflow.ExecuteLocalActivity #354
  2. How was this tested: for now manually by using my fork on my own internal project
  3. Any docs updates needed?: I don't think there is any doc around that specific to local activities.

Todo

  • Fix existing tests
  • Add tests for local activity execution by name

@CLAassistant
Copy link

CLAassistant commented Apr 1, 2021

CLA assistant check
All committers have signed the CLA.

Previous commit was enforcing local activities to be registered into the
worker to be ran. However, current behavior is that code like

ExecuteLocalActivity(ctx, func() error {
    return nil
})

is actually valid meaning local activity registration is optional when
you pass down the activity function directly.

This commits bring backs the old ExecuteLocalActivity behavior when the
activity passed is a func and tries to load the activity func from the
registry when we pass a string.
@lminaudier lminaudier force-pushed the execute-local-activities-by-name branch from c8850b0 to 00db9ac Compare April 1, 2021 15:35
@lminaudier lminaudier marked this pull request as ready for review April 1, 2021 15:40
Previous commits have removed validation of functions passed to
ExecuteLocalActivity.
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks for the PR and keeping things backwards compatible.

@Sushisource Sushisource merged commit fee5fb6 into temporalio:master Apr 1, 2021
@lminaudier lminaudier deleted the execute-local-activities-by-name branch April 2, 2021 08:36
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.

Allow string activity names in workflow.ExecuteLocalActivity
3 participants