Address comments

Author: aarondav

network/common/src/main/java/org/apache/spark/network/util/JavaUtils.java

@@ -122,11 +122,10 @@ public static void deleteRecursively(File file) throws IOException {
       }
     }
 
-    if (!file.delete()) {
-      // Delete can also fail if the file simply did not exist
-      if (file.exists()) {
-        throw new IOException("Failed to delete: " + file.getAbsolutePath());
-      }
+    boolean deleted = file.delete();
+    // Delete can also fail if the file simply did not exist.
+    if (!deleted && file.exists()) {
+      throw new IOException("Failed to delete: " + file.getAbsolutePath());
     }
   }
 

network/shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockManager.java

@@ -25,7 +25,6 @@
 import java.util.Map;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.Executor;
-import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -53,8 +52,8 @@ public class ExternalShuffleBlockManager {
   // Map containing all registered executors' metadata.
   private final ConcurrentMap<AppExecId, ExecutorShuffleInfo> executors;
 
-  // Single-threaded executor used to perform expensive recursive directory deletion.
-  private final Executor directoryCleanupExecutor;
+  // Single-threaded Java executor used to perform expensive recursive directory deletion.
+  private final Executor directoryCleaner;
 
   public ExternalShuffleBlockManager() {
     // TODO: Give this thread a name.
@@ -63,9 +62,9 @@ public ExternalShuffleBlockManager() {
 
   // Allows tests to have more control over when directories are cleaned up.
   @VisibleForTesting
-  ExternalShuffleBlockManager(Executor directoryCleanupExecutor) {
+  ExternalShuffleBlockManager(Executor directoryCleaner) {
     this.executors = Maps.newConcurrentMap();
-    this.directoryCleanupExecutor = directoryCleanupExecutor;
+    this.directoryCleaner = directoryCleaner;
   }
 
   /** Registers a new Executor with all the configuration we need to find its shuffle files. */
@@ -134,8 +133,9 @@ public void applicationRemoved(String appId, boolean cleanupLocalDirs) {
           logger.info("Cleaning up executor {}'s {} local dirs", fullId, executor.localDirs.length);
 
           // Execute the actual deletion in a different thread, as it may take some time.
-          directoryCleanupExecutor.execute(new Runnable() {
-            @Override public void run() {
+          directoryCleaner.execute(new Runnable() {
+            @Override
+            public void run() {
               deleteExecutorDirs(executor.localDirs);
             }
           });

network/shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleCleanupSuite.java

@@ -21,6 +21,7 @@
 import java.io.IOException;
 import java.util.Random;
 import java.util.concurrent.Executor;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import com.google.common.util.concurrent.MoreExecutors;
 import org.junit.Test;
@@ -53,16 +54,19 @@ public void noCleanupAndCleanup() throws IOException {
   public void cleanupUsesExecutor() throws IOException {
     TestShuffleDataContext dataContext = createSomeData();
 
+    final AtomicBoolean cleanupCalled = new AtomicBoolean(false);
+
     // Executor which does nothing to ensure we're actually using it.
     Executor noThreadExecutor = new Executor() {
-      @Override public void execute(Runnable runnable) { /* do nothing */ }
+      @Override public void execute(Runnable runnable) { cleanupCalled.set(true); }
     };
 
     ExternalShuffleBlockManager manager = new ExternalShuffleBlockManager(noThreadExecutor);
 
     manager.registerExecutor("app", "exec0", dataContext.createExecutorInfo("shuffleMgr"));
     manager.applicationRemoved("app", true);
 
+    assertTrue(cleanupCalled.get());
     assertStillThere(dataContext);
 
     dataContext.cleanup();