Added VolumeManager.deleteBulk method to use FileSystem.createBulkDelete#6443
Open
dlmarion wants to merge 2 commits into
Open
Added VolumeManager.deleteBulk method to use FileSystem.createBulkDelete#6443dlmarion wants to merge 2 commits into
dlmarion wants to merge 2 commits into
Conversation
Added method to VolumeManager that accepts a collection of Paths to delete. If the underlying FileSystem supports bulk deletion, then that API method is called. Otherwise the Paths are deleted using an ExecutorService. Reviewed the existing code that used the other delete methods in the VolumeManager. Only two places seemed like candidates for using the new deleteBulk method. The other locations either only deleted one file aperiodically, or deleted many files but using some complex logic. The former didn't make sense to modify and the latter could potentially be modified later with some refactoring. Closes apache#5131
ctubbsii
reviewed
Jul 9, 2026
| // The S3A FileSystem implementation has a default of 250 and | ||
| // is configured by the property `fs.s3a.bulk.delete.page.size`. | ||
| if (bulk.pageSize() > 1) { | ||
| this.bulkDeleteFileSystems.put(fs, v); |
Member
There was a problem hiding this comment.
I'm not sure I understand why this is a map of FileSystem to Volume. Don't you just want a list of volumes with bulk delete capability? Multiple volumes could use the same FileSystem, so each FileSystem in this map could map to more than one Volume. If you're pulling the values out of this map, it could be incomplete.
Instead, each Volume object should have its own flag to indicate whether or not it supports bulk delete.
|
|
||
| for (Path p : paths) { | ||
| FileSystem fs = getFileSystemByPath(p); | ||
| Volume v = this.bulkDeleteFileSystems.get(fs); |
Member
There was a problem hiding this comment.
This is a bug. Just because there is an entry here by FileSystem, it doesn't mean it's retrieving the correct Volume. Multiple volumes can use the same FileSystem because a Volume is a FileSystem and base path.
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.
Added method to VolumeManager that accepts a collection of Paths to delete. If the underlying FileSystem supports bulk deletion, then that API method is called. Otherwise the Paths are deleted using an ExecutorService.
Reviewed the existing code that used the other delete methods in the VolumeManager. Only two places seemed like candidates for using the new deleteBulk method. The other locations either only deleted one file aperiodically, or deleted many files but using some complex logic. The former didn't make sense to modify and the latter could potentially be modified later with some refactoring.
Closes #5131