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

Cancel activity when eager execution and request cancellation are in the same WFT #3029

Merged

Conversation

yycptt
Copy link
Member

@yycptt yycptt commented Jun 27, 2022

What changed?

  • Cancel activity instead of returning an activity task to worker when schedule activity with eager execution command and request cancellation command are in the same workflow task.
  • Rename the feature flag from local dispatch to eager execution

Why?

  • Match the behavior when eager execution is disabled.
  • Basically change the behavior of eager execution + cancel from performing operation schedule + start + cancel to schedule + cancel + start (no-op), so that activity will be cancelled instead of started and returned to worker.

How did you test it?

  • Added unit test

Potential risks

  • N/A, sdk is still development the feature?

Is hotfix candidate?

  • yes.

@yycptt yycptt requested a review from yiminc June 27, 2022 22:21
// EnableActivityLocalDispatch indicates if acitivty local dispatch is enabled per namespace
EnableActivityLocalDispatch = "system.enableActivityLocalDispatch"
// EnableActivityEagerExecution indicates if acitivty eager execution is enabled per namespace
EnableActivityEagerExecution = "system.enableActivityEagerExecution"
Copy link
Member Author

Choose a reason for hiding this comment

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

Should be ok as this is an experimental feature and sdk side is still developing?

@yycptt yycptt marked this pull request as ready for review June 28, 2022 00:47
@yycptt yycptt requested a review from a team as a code owner June 28, 2022 00:47
@yiminc
Copy link
Member

yiminc commented Jun 28, 2022

The behavior without eager execution is schedule + cancel, there is no started event.
We should keep the same behavior when eager execution is enabled, there should be no started event.

@yycptt
Copy link
Member Author

yycptt commented Jun 28, 2022

The behavior without eager execution is schedule + cancel, there is no started event. We should keep the same behavior when eager execution is enabled, there should be no started event.

This is what this pr is doing: cancel instead of start the activity and return it to worker.
Schedule + cancel without eager execution: activity scheduled + cancel requested + cancelled
Existing schedule + cancel with eager execution: activity scheduled + activity started + cancel requested and returns activity task to worker
With this change schedule + cancel with eager execution will behave the same as without eager execution.

@yycptt yycptt merged commit b262b52 into temporalio:master Jun 29, 2022
@yycptt yycptt deleted the activity-eager-execution-with-cancel branch June 29, 2022 01:00
alexshtin pushed a commit that referenced this pull request Jul 8, 2022
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.

None yet

2 participants