ctubbsii commented on code in PR #6443:
URL: https://github.com/apache/accumulo/pull/6443#discussion_r3554832934
##########
server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java:
##########
@@ -95,6 +102,29 @@ protected VolumeManagerImpl(Map<String,Volume> volumes,
AccumuloConfiguration co
this.volumesByName = volumes;
// We may have multiple directories used in a single FileSystem (e.g.
testing)
this.volumesByFileSystemUri = invertVolumesByFileSystem(volumesByName);
+
+ this.bulkDeleteFileSystems = new HashMap<>();
+ for (Volume v : volumes.values()) {
+ FileSystem fs = v.getFileSystem();
+ String base = v.getBasePath().isBlank() ? "/" : v.getBasePath();
+ Path basePath = fs.makeQualified(new Path(base));
+ try {
+ if (fs.hasPathCapability(basePath, "fs.capability.bulk.delete")) {
+ try (BulkDelete bulk = fs.createBulkDelete(basePath)) {
+ // Don't use the BulkDelete API if the page size is only 1.
+ // The DefaultBulkDeleteOperation implementation has a page size
of 1.
+ // 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);
Review 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.
##########
server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java:
##########
@@ -213,6 +243,110 @@ public boolean deleteRecursively(Path path) throws
IOException {
return getFileSystemByPath(path).delete(path, true);
}
+ @Override
+ public Map<Path,DeleteStatus> deleteBulk(Collection<Path> paths) throws
IOException {
+
+ requireNonNull(paths);
+
+ if (paths.isEmpty()) {
+ return Map.of();
+ }
+
+ if (paths.size() == 1) {
+ Path p = paths.iterator().next();
+ try {
+ if (delete(p)) {
+ return Map.of(p, DeleteStatus.TRUE);
+ } else {
+ return Map.of(p, DeleteStatus.FALSE);
+ }
+ } catch (IOException e) {
+ return Map.of(p, DeleteStatus.ERROR);
+ }
+ }
+
+ final Map<Volume,ArrayList<Path>> pathsSupportingBulkDelete = new
HashMap<>();
+ final List<Path> pathsNotSupportingBulkDelete = new ArrayList<>();
+ final Map<Path,DeleteStatus> results = new ConcurrentHashMap<>();
+ final List<Future<Void>> futures = new LinkedList<>();
+
+ for (Path p : paths) {
+ FileSystem fs = getFileSystemByPath(p);
+ Volume v = this.bulkDeleteFileSystems.get(fs);
Review 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 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]
