fix(io): attempt every S3 delete and report how many failed - #966
Open
plusplusjiajia wants to merge 5 commits into
Open
plusplusjiajia wants to merge 5 commits into
plusplusjiajia wants to merge 5 commits into
Conversation
plusplusjiajia
force-pushed
the
fix-s3-delete-files-attempt-all
branch
from
September 26, 2026 06:30
fee511b to
1f5fb83
Compare
zhjwpku
approved these changes
Sep 26, 2026
| size_t failed = 0; | ||
| for (const auto& file_location : file_locations) { | ||
| locations_by_io[&FileIOForPath(file_location)].push_back(file_location); | ||
| if (auto status = FileIOForPath(file_location).DeleteFile(file_location); |
Collaborator
There was a problem hiding this comment.
Should we use a thread pool to delete files concurrently? Sequential S3 deletes could be slow for large batches. Java's S3FileIO does that, so maybe we should pursue that too, but not a requirement for this PR.
Member
Author
There was a problem hiding this comment.
@zhjwpku Thanks! Done: deletes now run on up to s3.delete.num-threads threads (default: hardware threads), as in Java. Java also batches keys into DeleteObjects; Arrow has no public batch delete for S3, so each thread deletes one file at a time.
zhjwpku
reviewed
Sep 27, 2026
| auto logger = GetCurrentLogger(); | ||
| std::vector<std::future<void>> helpers; | ||
| for (size_t i = 1; i < std::min(delete_threads_, file_locations.size()); ++i) { | ||
| helpers.push_back(std::async(std::launch::async, [&] { |
Collaborator
There was a problem hiding this comment.
We have TaskGroup and Executor abstractions, not sure if they fit it here.
Collaborator
There was a problem hiding this comment.
Maybe not since there is no need to retry here. cc @HuaHuaY
Member
Author
There was a problem hiding this comment.
@zhjwpku Thanks! TaskGroup needs a caller-supplied Executor, which DeleteFiles can't receive, and ExpireSnapshots already retries it. So plain threads here.
ArrowS3FileIO::DeleteFiles grouped locations by credential prefix and returned at the first group that failed, so files in later groups were never attempted. Java's S3FileIO attempts every batch, logs each failed path and reports the failure count. Delete each file through its delegate, log each failure, and return "Failed to delete N of M files" at the end. This sends the same requests as before, since Arrow's S3 DeleteFiles also deletes one file at a time. ResolvingFileIO stays fail-fast across delegates, as it is in Java.
Java's S3FileIO runs its deletes on an executor sized by s3.delete.num-threads, which defaults to the number of processors. DeleteFiles now does the same, with the calling thread taking part. Java also packs keys into DeleteObjects batches. Arrow has no public batch delete for S3, so each thread still deletes one file at a time, as Arrow's own DeleteFiles does.
std::jthread needs -fexperimental-library with libc++ 18 and 19, which the Clang 18+ requirement covers, so use std::async futures instead; they also wait for their thread if an exception unwinds. Helper threads now bind the caller's logger, as logger.h prescribes for thread pools. Without it their warnings went to the global logger while the returned error only carries the failure count.
wait() leaves an exception stored in a helper's future, so a delete that threw was neither counted nor reported, and DeleteFiles could return success. get() rethrows it on the calling thread, as the sequential loop did.
plusplusjiajia
force-pushed
the
fix-s3-delete-files-attempt-all
branch
from
September 29, 2026 04:11
41ff77e to
492c6cb
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #898 (comment).
ArrowS3FileIO::DeleteFilesreturned at the first credential prefix whose delete failed, so files under the remaining prefixes were never attempted. Java'sS3FileIO.deleteFilesattempts every batch on a thread pool, logs each failed path, and throwsBulkDeletionFailureExceptionwith the count.This matches that:
Failed to delete N of M files.s3.delete.num-threadsthreads (default: the number of hardware threads), as in Java.Java also packs keys into
DeleteObjectsbatches. Arrow has no public batch delete for S3, so each thread deletes one file at a time, as Arrow's ownDeleteFilesdoes.ResolvingFileIOstays fail-fast across delegates, as Java's does.DeleteFilesAttemptsEveryFileputs an allowed file between two denied ones; it fails onmainand passes here. This touches the same function as #898, so whichever lands second needs a small rebase.