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

fix(core): don't retry a task if the execution is killed #2057

Merged
merged 1 commit into from
Sep 18, 2023

Conversation

loicmathieu
Copy link
Member

Note that this will make the task not killing but ends with the "previous" state (failed), the execution will then be marked as failed and not killed at the moment, but if #1747 is merged the execution will correctly be marked as KILLED. Anyway, currently, without this, the execution is also marked as FAILED, but the attempts continue so this PR improves the current process by short-circuiting the attempt for killed execution.

Note that this will make the task not killing but ends with the "previous" state (failed), the execution will then be marked as failed and not killed at the moment, but if #1747 is merged the execution will correctly be marked as KILLED.
Anyway, currently, without this , the exuction is also marked as FAILED but the attempts continue so this PR improve the current process by shortcirciting the attempt for killed execution.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

85.7% 85.7% Coverage
0.0% 0.0% Duplication

@tchiotludo tchiotludo added the enhancement New feature or request label Sep 7, 2023
@@ -308,7 +308,8 @@ private WorkerTaskResult run(WorkerTask workerTask, Boolean cleanUp) throws Queu
WorkerTask finalWorkerTask = Failsafe
.with(AbstractRetry.<WorkerTask>retryPolicy(workerTask.getTask().getRetry())
.handleResultIf(result -> result.getTaskRun().lastAttempt() != null &&
Objects.requireNonNull(result.getTaskRun().lastAttempt()).getState().getCurrent() == State.Type.FAILED
result.getTaskRun().lastAttempt().getState().getCurrent() == State.Type.FAILED &&
Copy link
Member

Choose a reason for hiding this comment

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

It lacks a test to cover it I think

Copy link
Member Author

Choose a reason for hiding this comment

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

I can try to make one but as it's by nature racy the test might be flaky.

@tchiotludo tchiotludo merged commit 8733cd6 into develop Sep 18, 2023
@tchiotludo tchiotludo deleted the fix/kill-attempt branch September 18, 2023 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants