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

Check workflow task after reapply events #2840

Merged
merged 8 commits into from
May 12, 2022
Merged

Conversation

yux0
Copy link
Contributor

@yux0 yux0 commented May 12, 2022

What changed?
Check workflow task after reapply events

Why?
When reapply event to current branch, we need to check if it should schedule a new workflow task

How did you test it?

Potential risks

Is hotfix candidate?

@yux0 yux0 requested a review from a team as a code owner May 12, 2022 17:56

// After reapply event, checking if we should schedule a workflow task
// Do not create workflow task when the workflow is cron and the cron has not been started yet
if msBuilder.GetExecutionInfo().CronSchedule != "" && !msBuilder.HasProcessedOrPendingWorkflowTask() {
Copy link
Member

Choose a reason for hiding this comment

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

instead of checking for cronSchedule, you should check firstWorkflowTaskBackoff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If cron has the first wf task, then we should continue schedule a task. Is this correct?

Copy link
Member

Choose a reason for hiding this comment

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

We also need to take care of workflow retry backoff case I think.

Comment on lines 2848 to 2850
// Do not create workflow task when the workflow has first workflow task backoff and execution is not started yet
workflowTaskBackoff := timestamp.TimeValue(executionInfo.GetExecutionTime()).After(timestamp.TimeValue(executionInfo.GetStartTime()))
if workflowTaskBackoff && !mutableState.HasProcessedOrPendingWorkflowTask() {
Copy link
Member

Choose a reason for hiding this comment

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

Shall we create a util function for this check?

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 also needed here

workflowTaskBackoff := timestamp.TimeValue(executionInfo.GetExecutionTime()).After(timestamp.TimeValue(executionInfo.GetStartTime()))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

@yux0 yux0 merged commit 7361f8b into temporalio:master May 12, 2022
@yux0 yux0 deleted the reapply-wf-task branch May 12, 2022 23:23
meiliang86 pushed a commit that referenced this pull request May 12, 2022
* Check workflow task after reapply events
meiliang86 pushed a commit that referenced this pull request May 27, 2022
* Check workflow task after reapply events
meiliang86 pushed a commit that referenced this pull request Jun 1, 2022
* Check workflow task after reapply events
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

4 participants