dimas-b commented on code in PR #4962:
URL: https://github.com/apache/polaris/pull/4962#discussion_r3539826138
##########
runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -229,35 +230,32 @@ protected void handleTask(
.addKeyValue("taskEntityId", taskEntityId)
.addKeyValue("taskType", task.getTaskType())
.log("Unable to find handler for task type");
- throw new RuntimeException(
+ throw new TaskHandlerNotFoundException(
"Unable to find handler for task type "
+ task.getTaskType()
+ " for task entity id "
+ taskEntityId);
}
TaskHandler handler = handlerOpt.get();
- success = handler.handleTask(task, ctx);
- if (success) {
- LOGGER
- .atInfo()
- .addKeyValue("taskEntityId", taskEntityId)
- .addKeyValue("handlerClass", handler.getClass())
- .log("Task successfully handled");
- metaStoreManager.dropEntityIfExists(
- ctx.getPolarisCallContext(), null, taskEntity, Map.of(), false);
- } else {
+ handler.handleTask(task, ctx);
+ success = true;
+ LOGGER
+ .atInfo()
+ .addKeyValue("taskEntityId", taskEntityId)
+ .addKeyValue("handlerClass", handler.getClass())
+ .log("Task successfully handled");
+ metaStoreManager.dropEntityIfExists(
+ ctx.getPolarisCallContext(), null, taskEntity, Map.of(), false);
+ } catch (Exception e) {
+ if (!(e instanceof TaskHandlerNotFoundException)) {
Review Comment:
nit: Would it be more readable to simply catch and re-throw
`TaskHandlerNotFoundException` above `Exception`?
##########
runtime/service/src/test/java/org/apache/polaris/service/task/TaskExecutorImplTest.java:
##########
@@ -224,8 +223,7 @@ public boolean handleTask(TaskEntity task, CallContext
callContext) {
PolarisEventMetadata.builder().realmId(realm).build(),
1))
.isInstanceOf(RuntimeException.class)
- .hasMessageContaining("Task handler returned false")
- .hasMessageContaining(String.valueOf(taskEntity.getId()));
Review Comment:
Did we lose the task ID in the exception message? I guess it would be nice
to keep it for maximum backward compatibility. I guess people may rely on see
it in logs 🤔
##########
runtime/service/src/test/java/org/apache/polaris/service/task/BatchFileCleanupTaskHandlerTest.java:
##########
@@ -226,7 +229,7 @@ public void testMetadataFileCleanupIfFileNotExist() throws
IOException {
task = addTaskLocation(task);
assertThatPredicate(handler::canHandleTask).accepts(task);
- assertThat(handler.handleTask(task, polarisCallContext)).isTrue();
+ handler.handleTask(task, polarisCallContext);
Review Comment:
`doesNotThrowAnyException()`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
