svn commit: r1427287 - in /hbase/trunk: hbase-common/src/test/java/org/apache/hadoop/hbase/ hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/ hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/

[te...@apache.org]

Author: tedyu
Date: Tue Jan  1 03:07:31 2013
New Revision: 1427287

URL: http://svn.apache.org/viewvc?rev=1427287&view=rev
Log:
HBASE-7467 CleanerChore checkAndDeleteDirectory not deleting empty directories (Jean-Marc and Jesse Yates)


Modified:
    hbase/trunk/hbase-common/src/test/java/org/apache/hadoop/hbase/HBaseCommonTestingUtility.java
    hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/CleanerChore.java
    hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/TestCleanerChore.java

Modified: hbase/trunk/hbase-common/src/test/java/org/apache/hadoop/hbase/HBaseCommonTestingUtility.java
URL: http://svn.apache.org/viewvc/hbase/trunk/hbase-common/src/test/java/org/apache/hadoop/hbase/HBaseCommonTestingUtility.java?rev=1427287&r1=1427286&r2=1427287&view=diff
==============================================================================
--- hbase/trunk/hbase-common/src/test/java/org/apache/hadoop/hbase/HBaseCommonTestingUtility.java (original)
+++ hbase/trunk/hbase-common/src/test/java/org/apache/hadoop/hbase/HBaseCommonTestingUtility.java Tue Jan  1 03:07:31 2013
@@ -101,7 +101,7 @@ public class HBaseCommonTestingUtility {
    * @return True if we removed the test dirs
    * @throws IOException
    */
-  boolean cleanupTestDir() throws IOException {
+  public boolean cleanupTestDir() throws IOException {
     if (deleteDir(this.dataTestDir)) {
       this.dataTestDir = null;
       return true;
@@ -153,4 +153,4 @@ public class HBaseCommonTestingUtility {
       return false;
     }
   }
-};
\ No newline at end of file
+};

Modified: hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/CleanerChore.java
URL: http://svn.apache.org/viewvc/hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/CleanerChore.java?rev=1427287&r1=1427286&r2=1427287&view=diff
==============================================================================
--- hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/CleanerChore.java (original)
+++ hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/CleanerChore.java Tue Jan  1 03:07:31 2013
@@ -145,13 +145,23 @@ public abstract class CleanerChore<T ext
    * @return <tt>true</tt> if the directory was deleted, <tt>false</tt> otherwise.
    * @throws IOException if there is an unexpected filesystem error
    */
-  private boolean checkAndDeleteDirectory(Path toCheck) throws IOException {
+  public boolean checkAndDeleteDirectory(Path toCheck) throws IOException {
     if (LOG.isTraceEnabled()) {
       LOG.trace("Checking directory: " + toCheck);
     }
     FileStatus[] children = FSUtils.listStatus(fs, toCheck);
     // if the directory doesn't exist, then we are done
-    if (children == null) return true;
+    if (children == null) {
+      try {
+        return fs.delete(toCheck, false);
+      } catch (IOException e) {
+        if (LOG.isTraceEnabled()) {
+          LOG.trace("Couldn't delete directory: " + toCheck, e);
+        }
+      }
+      // couldn't delete w/o exception, so we can't return success.
+      return false;
+    }
 
     boolean canDeleteThis = true;
     for (FileStatus child : children) {
@@ -168,9 +178,22 @@ public abstract class CleanerChore<T ext
       }
     }
 
-    // if all the children have been deleted, then we should try to delete this directory. However,
-    // don't do so recursively so we don't delete files that have been added since we checked.
-    return canDeleteThis ? fs.delete(toCheck, false) : false;
+    // if the directory has children, we can't delete it, so we are done
+    if (!canDeleteThis) return false;
+
+    // otherwise, all the children (that we know about) have been deleted, so we should try to
+    // delete this directory. However, don't do so recursively so we don't delete files that have
+    // been added since we last checked.
+    try {
+      return fs.delete(toCheck, false);
+    } catch (IOException e) {
+      if (LOG.isTraceEnabled()) {
+        LOG.trace("Couldn't delete directory: " + toCheck, e);
+      }
+    }
+
+    // couldn't delete w/o exception, so we can't return success.
+    return false;
   }
 
   /**

Modified: hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/TestCleanerChore.java
URL: http://svn.apache.org/viewvc/hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/TestCleanerChore.java?rev=1427287&r1=1427286&r2=1427287&view=diff
==============================================================================
--- hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/TestCleanerChore.java (original)
+++ hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/master/cleaner/TestCleanerChore.java Tue Jan  1 03:07:31 2013
@@ -48,11 +48,9 @@ public class TestCleanerChore {
   @After
   public void cleanup() throws Exception {
     // delete and recreate the test directory, ensuring a clean test dir between tests
-    Path testDir = UTIL.getDataTestDir();
-    FileSystem fs = UTIL.getTestFileSystem();
-    fs.delete(testDir, true);
-    fs.mkdirs(testDir);
-  }
+    UTIL.cleanupTestDir();
+}
+
 
   @Test
   public void testSavesFilesOnRequest() throws Exception {
@@ -95,8 +93,10 @@ public class TestCleanerChore {
     // create the directory layout in the directory to clean
     Path parent = new Path(testDir, "parent");
     Path child = new Path(parent, "child");
+    Path emptyChild = new Path(parent, "emptyChild");
     Path file = new Path(child, "someFile");
     fs.mkdirs(child);
+    fs.mkdirs(emptyChild);
     // touch a new file
     fs.create(file).close();
     // also create a file in the top level directory
@@ -225,6 +225,66 @@ public class TestCleanerChore {
     Mockito.reset(spy);
   }
 
+  /**
+   * The cleaner runs in a loop, where it first checks to see all the files under a directory can be
+   * deleted. If they all can, then we try to delete the directory. However, a file may be added
+   * that directory to after the original check. This ensures that we don't accidentally delete that
+   * directory on and don't get spurious IOExceptions.
+   * <p>
+   * This was from HBASE-7465.
+   * @throws Exception on failure
+   */
+  @Test
+  public void testNoExceptionFromDirectoryWithRacyChildren() throws Exception {
+    Stoppable stop = new StoppableImplementation();
+    // need to use a localutil to not break the rest of the test that runs on the local FS, which
+    // gets hosed when we start to use a minicluster.
+    HBaseTestingUtility localUtil = new HBaseTestingUtility();
+    Configuration conf = localUtil.getConfiguration();
+    final Path testDir = UTIL.getDataTestDir();
+    final FileSystem fs = UTIL.getTestFileSystem();
+    LOG.debug("Writing test data to: " + testDir);
+    String confKey = "hbase.test.cleaner.delegates";
+    conf.set(confKey, AlwaysDelete.class.getName());
+
+    AllValidPaths chore = new AllValidPaths("test-file-cleaner", stop, conf, fs, testDir, confKey);
+    // spy on the delegate to ensure that we don't check for directories
+    AlwaysDelete delegate = (AlwaysDelete) chore.cleanersChain.get(0);
+    AlwaysDelete spy = Mockito.spy(delegate);
+    chore.cleanersChain.set(0, spy);
+
+    // create the directory layout in the directory to clean
+    final Path parent = new Path(testDir, "parent");
+    Path file = new Path(parent, "someFile");
+    fs.mkdirs(parent);
+    // touch a new file
+    fs.create(file).close();
+    assertTrue("Test file didn't get created.", fs.exists(file));
+    final Path racyFile = new Path(parent, "addedFile");
+
+    // when we attempt to delete the original file, add another file in the same directory
+    Mockito.doAnswer(new Answer<Boolean>() {
+      @Override
+      public Boolean answer(InvocationOnMock invocation) throws Throwable {
+        fs.create(racyFile).close();
+        FSUtils.logFileSystemState(fs, testDir, LOG);
+        return (Boolean) invocation.callRealMethod();
+      }
+    }).when(spy).isFileDeletable(Mockito.any(Path.class));
+
+    // attempt to delete the directory, which
+    if (chore.checkAndDeleteDirectory(parent)) {
+      throw new Exception(
+          "Reported success deleting directory, should have failed when adding file mid-iteration");
+    }
+
+    // make sure all the directories + added file exist, but the original file is deleted
+    assertTrue("Added file unexpectedly deleted", fs.exists(racyFile));
+    assertTrue("Parent directory deleted unexpectedly", fs.exists(parent));
+    assertFalse("Original file unexpectedly retained", fs.exists(file));
+    Mockito.verify(spy, Mockito.times(1)).isFileDeletable(Mockito.any(Path.class));
+  }
+
   private static class AllValidPaths extends CleanerChore<BaseHFileCleanerDelegate> {
 
     public AllValidPaths(String name, Stoppable s, Configuration conf, FileSystem fs,
@@ -252,4 +312,4 @@ public class TestCleanerChore {
       return false;
     }
   }
-}
\ No newline at end of file
+}