[GitHub] [accumulo] keith-turner commented on a change in pull request #1635: Create bulkRename method in VolumeManager

[GitBox <gi...@apache.org>]

keith-turner commented on a change in pull request #1635:
URL: https://github.com/apache/accumulo/pull/1635#discussion_r443064254



##########
File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java
##########
@@ -289,6 +294,38 @@ public boolean rename(Path path, Path newPath) throws IOException {
     return source.rename(path, newPath);
   }
 
+  @Override
+  public List<Future<Boolean>> bulkRename(Map<Path,Path> oldToNewPathMap,
+      SimpleThreadPool workerPool, String transactionId) throws InterruptedException {
+    List<Future<Boolean>> results = new ArrayList<>();
+    oldToNewPathMap.forEach((oldPath, newPath) -> results.add(workerPool.submit(() -> {
+      boolean success;
+      try {
+        success = rename(oldPath, newPath);
+      } catch (IOException e) {
+        // The rename could have failed because this is the second time its running (failures
+        // could cause this to run multiple times).
+        if (!exists(newPath) || exists(oldPath)) {
+          throw e;
+        }
+        log.debug(
+            "Ignoring rename exception for {} because destination already exists. orig: {} new: {}",
+            transactionId, oldPath, newPath, e);
+        success = true;
+      }
+      if (!success) {
+        log.error("Rename operation {} returned false. orig: {} new: {}", transactionId, oldPath,

Review comment:
       should this throw an exception?

##########
File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java
##########
@@ -289,6 +294,38 @@ public boolean rename(Path path, Path newPath) throws IOException {
     return source.rename(path, newPath);
   }
 
+  @Override
+  public List<Future<Boolean>> bulkRename(Map<Path,Path> oldToNewPathMap,
+      SimpleThreadPool workerPool, String transactionId) throws InterruptedException {
+    List<Future<Boolean>> results = new ArrayList<>();
+    oldToNewPathMap.forEach((oldPath, newPath) -> results.add(workerPool.submit(() -> {

Review comment:
       A lambda within a lambda. I was looking for a way to add a third inner lambda, but did not see an opportunity.

##########
File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java
##########
@@ -289,6 +294,38 @@ public boolean rename(Path path, Path newPath) throws IOException {
     return source.rename(path, newPath);
   }
 
+  @Override
+  public List<Future<Boolean>> bulkRename(Map<Path,Path> oldToNewPathMap,
+      SimpleThreadPool workerPool, String transactionId) throws InterruptedException {
+    List<Future<Boolean>> results = new ArrayList<>();
+    oldToNewPathMap.forEach((oldPath, newPath) -> results.add(workerPool.submit(() -> {
+      boolean success;
+      try {
+        success = rename(oldPath, newPath);
+      } catch (IOException e) {
+        // The rename could have failed because this is the second time its running (failures
+        // could cause this to run multiple times).
+        if (!exists(newPath) || exists(oldPath)) {
+          throw e;
+        }
+        log.debug(
+            "Ignoring rename exception for {} because destination already exists. orig: {} new: {}",
+            transactionId, oldPath, newPath, e);
+        success = true;
+      }
+      if (!success) {
+        log.error("Rename operation {} returned false. orig: {} new: {}", transactionId, oldPath,
+            newPath);
+      } else if (log.isTraceEnabled()) {
+        log.trace("{} moved {} to {}", transactionId, oldPath, newPath);
+      }
+      return success;
+    })));
+    workerPool.shutdown();

Review comment:
       If this method is going to shutdown the pool, I think it would be better if it created the pool also.  This method could accept a number of threads and operation name instead of a thread pool and use those to create the pool.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org