You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "vigneshio (via GitHub)" <gi...@apache.org> on 2026/06/26 21:58:35 UTC
[PR] Fix async task retry when handlers return false [polaris]
vigneshio opened a new pull request, #4914:
URL: https://github.com/apache/polaris/pull/4914
Fix async task retry when handlers return false
Task handlers (ManifestFileCleanupTaskHandler, BatchFileCleanupTaskHandler, TableCleanupTaskHandler) legitimately return false on transient failures (IOExceptions, ExecutionExceptions from deletes and so on..). The comment in ManifestFileCleanupTaskHandler even says "return false (so the task gets retried)".
However, TaskExecutorImpl only treated success==true as a terminal state. On false it only logged a warning and returned normally. Because retry lives exclusively in .exceptionallyComposeAsync, returning false never triggered retries. The task entity was never dropped and cleanup could be lost.
This change makes a false return (and "no handler" case) throw a RuntimeException so the existing retry logic (3 total executions, TASK_RETRY_DELAY * attempt backoff) is used. The finally block still emits AFTER_ATTEMPT_TASK events with the correct TASK_SUCCESS value. After final exhaustion the task entity is left behind (existing behavior for exhausted tasks).
Added unit tests to cover ths..
## Checklist
- [ ] Don't disclose security issues! (contact security@apache.org)
- [x] Clearly explained why the changes are needed, or linked related issues
- [x] Added/updated tests with good coverage, or manually tested (and explained how)
- [x] Added comments for complex logic
- [x] Updated \`CHANGELOG.md\` (if needed)
- [ ] Updated documentation in \`site/content/in-dev/unreleased\` (if needed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix async task retry when handlers return false [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on code in PR #4914:
URL: https://github.com/apache/polaris/pull/4914#discussion_r3510962252
##########
runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -247,6 +251,12 @@ protected void handleTask(
.addKeyValue("taskEntityId", taskEntityId)
.addKeyValue("taskEntityName", taskEntity.getName())
.log("Unable to execute async task");
+ throw new RuntimeException(
+ "Task handler returned false for task entity id "
Review Comment:
Thanks @adutra and @dimas-b!
I agree - changing `handleTask` to return `void` would make the contract much cleaner way..
For this PR I focused on the immediate bug (making handlers throw the original exception instead of returning false so it actually propagates into the existing retry path). I've done that for the handlers that were catching exceptions.
I will do the `void` return type change + cleanup in a follow-up PR ASAP as you suggested @adutra , to keep this one scoped to the bugfix.
Let me know if that works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix async task retry when handlers return false [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4914:
URL: https://github.com/apache/polaris/pull/4914#discussion_r3507021725
##########
runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -247,6 +251,12 @@ protected void handleTask(
.addKeyValue("taskEntityId", taskEntityId)
.addKeyValue("taskEntityName", taskEntity.getName())
.log("Unable to execute async task");
+ throw new RuntimeException(
+ "Task handler returned false for task entity id "
Review Comment:
+1 to `void` return type.
That said, I'd be fine with doing it in a follow-up PR.... just to avoid "fixing a bug" and making an interface change in the same PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix async task retry when handlers return false [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #4914:
URL: https://github.com/apache/polaris/pull/4914#discussion_r3505056156
##########
runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -247,6 +251,12 @@ protected void handleTask(
.addKeyValue("taskEntityId", taskEntityId)
.addKeyValue("taskEntityName", taskEntity.getName())
.log("Unable to execute async task");
+ throw new RuntimeException(
+ "Task handler returned false for task entity id "
Review Comment:
But what's the point of returning false from the handler, then immediately throwing here? Shouldn't we go one step further and change `handleTask` to return `void`? That would make the contract clearer for task handlers: if the handler returns normally the task succeeded; if it throws, the task failed and should be retried.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix async task retry when handlers return false [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on code in PR #4914:
URL: https://github.com/apache/polaris/pull/4914#discussion_r3501781710
##########
runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -247,6 +251,12 @@ protected void handleTask(
.addKeyValue("taskEntityId", taskEntityId)
.addKeyValue("taskEntityName", taskEntity.getName())
.log("Unable to execute async task");
+ throw new RuntimeException(
+ "Task handler returned false for task entity id "
Review Comment:
Thanks @dimas-b good point... 💯 we were throwing a fresh unrelated exception..
Changed them to throw the original exception (wrapped where needed for checked IOException) so it propagates into the retry path. Kept the `return false → throw` logic in the executor as a backstop _since the tests rely on it._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix async task retry when handlers return false [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4914:
URL: https://github.com/apache/polaris/pull/4914#discussion_r3507033460
##########
runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -247,6 +251,12 @@ protected void handleTask(
.addKeyValue("taskEntityId", taskEntityId)
.addKeyValue("taskEntityName", taskEntity.getName())
.log("Unable to execute async task");
+ throw new RuntimeException(
+ "Task handler returned false for task entity id "
Review Comment:
... or we could re-purpose and re-title this PR for the bigger scope, which is changing to `void` return type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org