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/29 19:35:45 UTC
[PR] perf: remove redundant exists check before deleteFile (saves cloud API calls) [polaris]
vigneshio opened a new pull request, #4928:
URL: https://github.com/apache/polaris/pull/4928
perf: remove redundant exists check before deleteFile (saves cloud API calls)
When deleting individual data files asynchronously, the worker first makes a HEAD request (TaskUtils.exists) and then a DELETE request (deleteFile). Cloud storage DELETE operations (like S3 DeleteObject) are generally idempotent and do not fail if the file is absent. Performing a HEAD before every DELETE doubles the number of API calls and latency, severely impacting cloud billing and cleanup performance.
Fix: Remove the TaskUtils.exists check and simply call fileIO.deleteFile(file). Added a narrow catch for NotFoundException only to preserve test behavior for InMemoryFileIO (real cloud FileIOs succeed silently on missing objects).
## Checklist
- [x] 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] perf: remove redundant exists check before deleteFile (saves cloud API calls) [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: remove redundant exists check before deleteFile (saves cloud API calls) [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf: remove redundant exists check before deleteFile (saves cloud API calls) [polaris]
Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on code in PR #4928:
URL: https://github.com/apache/polaris/pull/4928#discussion_r3501845048
##########
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 @flyrain , added a debug log in the` NotFoundException` catch as suggested...
--
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] perf: remove redundant exists check before deleteFile (saves cloud API calls) [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b merged PR #4928:
URL: https://github.com/apache/polaris/pull/4928
--
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