[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_r449212013



##########
File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java
##########
@@ -289,6 +295,45 @@ public boolean rename(Path path, Path newPath) throws IOException {
     return source.rename(path, newPath);
   }
 
+  @Override
+  public void bulkRename(Map<Path,Path> oldToNewPathMap, int poolSize, String poolName,
+      String transactionId) throws IOException {
+    List<Future<Void>> results = new ArrayList<>();
+    SimpleThreadPool workerPool = new SimpleThreadPool(poolSize, poolName);
+    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()) {

Review comment:
       Seems like this should do the following.
   
   ```suggestion
         if (!success && (!exists(newPath) || exists(oldPath))) {
             throw new IOException("Rename operation "+transactionId+" returned false. orig: "+oldPath+" new: "+newPath);
         } else if (log.isTraceEnabled()) {
   ```

##########
File path: server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManager.java
##########
@@ -149,6 +150,11 @@ FSDataOutputStream createSyncable(Path logPath, int buffersize, short replicatio
   // volumes
   boolean rename(Path path, Path newPath) throws IOException;
 
+  // rename lots of files at once in a thread pool
+  // returns once all the threads have completed

Review comment:
       could add something to the docs about this operation being idempotent, safe to call again after previous partial run of bulkRename




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