flyrain commented on code in PR #4928:
URL: https://github.com/apache/polaris/pull/4928#discussion_r3500395438
##########
runtime/service/src/main/java/org/apache/polaris/service/task/FileCleanupTaskHandler.java:
##########
@@ -90,20 +91,15 @@ public CompletableFuture<Void> tryDelete(
}
return CompletableFuture.runAsync(
() -> {
- // totally normal for a file to already be missing, e.g. a data
file
- // may be in multiple manifests. There's a possibility we check
the
- // file's existence, but then it is deleted before we have a
chance to
- // send the delete request. In such a case, we <i>should</i>
retry
- // and find
- if (TaskUtils.exists(file, fileIO)) {
+ // deleteFile is idempotent on cloud object stores (S3
DeleteObject, etc.).
+ // It is totally normal for a data file to already be missing
(e.g. present
+ // in multiple manifests across snapshots). We call delete
directly.
+ // Some FileIO impls (e.g. InMemory) throw NotFound on missing;
we treat that
+ // as success (already deleted).
+ try {
fileIO.deleteFile(file);
- } else {
- LOGGER
- .atInfo()
- .addKeyValue("file", file)
- .addKeyValue("baseFile", baseFile != null ? baseFile : "")
- .addKeyValue("tableId", tableId)
- .log("table file cleanup task scheduled, but data file
doesn't exist");
+ } catch (NotFoundException nfe) {
+ // already gone (e.g. InMemoryFileIO or race)
Review Comment:
> the task would currently retry and eventually fail.
I think that's acceptable at the file level. Thanks for the explanation.
One minor comment: we may add a info or debug log here.
--
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]
