vigneshio commented on code in PR #4928:
URL: https://github.com/apache/polaris/pull/4928#discussion_r3497760626
##########
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:
Thanks for the review, @flyrain !!
For the main production `FileIO` implementations (`S3FileIO`, `GCSFileIO`,
`ADLSFileIO`, and S3-compatible stores like MinIO,RustFS), `deleteFile()` on a
missing object is a silent no-op, **matching S3/GCS/Azure semantics.**
The `NotFoundException` catch is intentionally narrow and mainly exists for
`InMemoryFileIO` tests and rare race conditions.
I avoided catching broader exceptions to prevent masking real failures like
permission or network issues.
If a provider throws a different exception for a missing file, the task
would currently retry and eventually fail. Wud you prefer we broaden the
handling for **_"not found"_** cases.. Or keep the current behavior and
document the assumption?
WDYT??
--
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]
