XN137 commented on code in PR #2304:
URL: https://github.com/apache/polaris/pull/2304#discussion_r2274075336
##########
runtime/service/src/main/java/org/apache/polaris/service/task/BatchFileCleanupTaskHandler.java:
##########
@@ -53,7 +53,7 @@ public boolean handleTask(TaskEntity task, CallContext
callContext) {
BatchFileCleanupTask cleanupTask =
task.readData(BatchFileCleanupTask.class);
TableIdentifier tableId = cleanupTask.tableId();
List<String> batchFiles = cleanupTask.batchFiles();
- try (FileIO authorizedFileIO = fileIOSupplier.apply(task, callContext)) {
+ try (FileIO authorizedFileIO = fileIOSupplier.apply(task, tableId,
callContext)) {
Review Comment:
> we could just add the fully-qualified table identifier to the task as a
property and extract it (if it's present) when it's needed for logging
so you suggest to have another `TaskEntity` property and duplicate the value
that is already there in `TASK_DATA`.
would that mean we remove it from `TASK_DATA` or keep it there as well?
will the new property be optional or required?
if optional, should the `TaskFileIOSupplier` fail hard if its missing?
if required, are there any persisted task entities out there that need to be
"migrated" somehow?
> Hardly a time-sensitive bugfix, so let's do this the right way.
this is delaying some follow-up work i was doing i.e.
https://github.com/apache/polaris/pull/2302 but yeah as stated above, the right
way for me is passing the already available value.
> I think this is fine too, if the change to add the tableidentifier as a
property is a headache.
so changing the `FileIOFactory` interface would be fine but changing
`TaskFileIOSupplier` is a no-go? whats the criteria?
> I ran through the existing TableCleanupTaskHandlerTest with the debugger
and the logged identifiers (e.g. db1.schema1.table1) look correct
not sure what you did locally but for me the identifier is wrong in the
debugger:
<img width="1333" height="688" alt="Screenshot From 2025-08-13 10-27-33"
src="https://github.com/user-attachments/assets/c0e29ce0-4072-415f-b026-03677ff9c245";
/>
note how it has an empty `namespace` and the `name` is just the task name.
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
