Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Ensure that JOIN tasks wait for all retries before terminating #3640

Closed
wants to merge 2 commits into from
Closed

Ensure that JOIN tasks wait for all retries before terminating #3640

wants to merge 2 commits into from

Conversation

bjpirt
Copy link
Contributor

@bjpirt bjpirt commented Jun 7, 2023

Credit to @james-deee who authored the patch that this PR implements.

This fixes the bug where a JOIN wasn't waiting for all of the retries for a task to have been performed

Pull Request type

  • Bugfix

Changes in this PR

This fixes the bug where a JOIN wasn't waiting for all of the retries for a task to have been performed

@manan164
Copy link
Contributor

manan164 commented Jun 7, 2023

Hi @bjpirt , Changes look fine. Can you please write few tests around this change?

@@ -56,15 +56,19 @@ public boolean execute(
break;
}
TaskModel.Status taskStatus = forkedTask.getStatus();
hasFailures = !taskStatus.isSuccessful() && !forkedTask.getWorkflowTask().isOptional();
hasFailures =
Copy link
Contributor

Choose a reason for hiding this comment

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

the code at line 52 should already fetch the join task that is retried, so it will be in progress and will wait for it to complete.

@JsonIgnore
public boolean isStillInNeedOfProcessing() {
if (Status.FAILED.equals(getStatus())) {
final int taskDefRetryCount = getTaskDefinition().map(TaskDef::getRetryCount).orElse(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can create NPE when task definition is not present.

@manan164
Copy link
Contributor

Hi @bjpirt , I have raised a fix for this change. #3655.
Ideally, changing the task model is not a good idea. Since this behavior was introduced in #3146 we should fix that.

@bjpirt
Copy link
Contributor Author

bjpirt commented Jun 16, 2023

Closing in favour of #3655

@bjpirt bjpirt closed this Jun 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants