HADOOP-3576. Fix NullPointerException when renaming a directory to its subdirectory. Contributed by Tsz Wo (Nicholas), SZE.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/core/trunk@669574 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -15,6 +15,9 @@ Trunk (unreleased changes)
     HADOOP-3563.  Refactor the distributed upgrade code so that it is 
     easier to identify datanode and namenode related code. (dhruba)
 
+    HADOOP-3576. Fix NullPointerException when renaming a directory
+    to its subdirectory. (Tse Wo (Nicholas), SZE via hairong) 
+
 Release 0.18.0 - Unreleased
 
   INCOMPATIBLE CHANGES

src/hdfs/org/apache/hadoop/dfs/FSDirectory.java

@@ -317,8 +317,7 @@ class FSDirectory implements FSConstants, Closeable {
   /**
    * @see #unprotectedRenameTo(String, String, long)
    */
-  public boolean renameTo(String src, String dst)
-  throws QuotaExceededException {
+  boolean renameTo(String src, String dst) throws QuotaExceededException {
     if (NameNode.stateChangeLog.isDebugEnabled()) {
       NameNode.stateChangeLog.debug("DIR* FSDirectory.renameTo: "
                                   +src+" to "+dst);
@@ -359,23 +358,6 @@ class FSDirectory implements FSConstants, Closeable {
         dst += Path.SEPARATOR + new Path(src).getName();
       }
       
-      byte[][] dstComponents = INode.getPathComponents(dst);
-      INode[] dstInodes = new INode[dstComponents.length];
-      rootDir.getExistingPathINodes(dstComponents, dstInodes);
-      
-      // check the validity of the destination
-      if (dstInodes[dstInodes.length-1] != null) { //check if destination exists
-        NameNode.stateChangeLog.warn("DIR* FSDirectory.unprotectedRenameTo: "
-                                     +"failed to rename "+src+" to "+dst+ 
-                                     " because destination exists");
-        return false;
-      } else if (dstInodes[dstInodes.length-2] == null) { // check if its parent exists
-        NameNode.stateChangeLog.warn("DIR* FSDirectory.unprotectedRenameTo: "
-            +"failed to rename "+src+" to "+dst+ 
-            " because destination's parent does not exists");
-        return false;
-      }
-      
       // remove source
       INode srcChild = null;
       try {
@@ -389,20 +371,37 @@ class FSDirectory implements FSConstants, Closeable {
         return false;
       }
 
-      // add to the destination
+      // check the validity of the destination
       INode dstChild = null;
       QuotaExceededException failureByQuota = null;
-      try {
-        // set the destination's name
+
+      byte[][] dstComponents = INode.getPathComponents(dst);
+      INode[] dstInodes = new INode[dstComponents.length];
+      rootDir.getExistingPathINodes(dstComponents, dstInodes);
+      if (dstInodes[dstInodes.length-1] != null) { //check if destination exists
+        NameNode.stateChangeLog.warn("DIR* FSDirectory.unprotectedRenameTo: "
+                                     +"failed to rename "+src+" to "+dst+ 
+                                     " because destination exists");
+      } else if (dstInodes[dstInodes.length-2] == null) { // check if its parent exists
+        NameNode.stateChangeLog.warn("DIR* FSDirectory.unprotectedRenameTo: "
+            +"failed to rename "+src+" to "+dst+ 
+            " because destination's parent does not exists");
+      }
+      else {
+        // add to the destination
         srcChild.setLocalName(dstComponents[dstInodes.length-1]);
-        // add it to the namespace
-        dstChild = addChild(dstInodes, dstInodes.length-1, srcChild, false);
-      } catch (QuotaExceededException qe) {
-        failureByQuota = qe;
+        try {
+          // add it to the namespace
+          dstChild = addChild(dstInodes, dstInodes.length-1, srcChild, false);
+        } catch (QuotaExceededException qe) {
+          failureByQuota = qe;
+        }
       }
       if (dstChild != null) {
-        NameNode.stateChangeLog.debug("DIR* FSDirectory.unprotectedRenameTo: "
+        if (NameNode.stateChangeLog.isDebugEnabled()) {
+          NameNode.stateChangeLog.debug("DIR* FSDirectory.unprotectedRenameTo: "
             +src+" is renamed to "+dst);
+        }
 
         // update modification time of dst and the parent of src
         srcInodes[srcInodes.length-2].setModificationTime(timestamp);

src/test/org/apache/hadoop/dfs/TestDFSRename.java

@@ -45,36 +45,50 @@ public class TestDFSRename extends junit.framework.TestCase {
       FileSystem fs = cluster.getFileSystem();
       assertTrue(fs.mkdirs(dir));
       
-      Path a = new Path(dir, "a");
-      Path aa = new Path(dir, "aa");
-      Path b = new Path(dir, "b");
-
-      DataOutputStream a_out = fs.create(a);
-      a_out.writeBytes("something");
-      a_out.close();
-      
-      //should not have any lease
-      assertEquals(0, countLease(cluster)); 
-
-      DataOutputStream aa_out = fs.create(aa);
-      aa_out.writeBytes("something");
+      { //test lease
+        Path a = new Path(dir, "a");
+        Path aa = new Path(dir, "aa");
+        Path b = new Path(dir, "b");
+  
+        DataOutputStream a_out = fs.create(a);
+        a_out.writeBytes("something");
+        a_out.close();
+        
+        //should not have any lease
+        assertEquals(0, countLease(cluster)); 
+  
+        DataOutputStream aa_out = fs.create(aa);
+        aa_out.writeBytes("something");
+  
+        //should have 1 lease
+        assertEquals(1, countLease(cluster)); 
+        list(fs, "rename0");
+        fs.rename(a, b);
+        list(fs, "rename1");
+        aa_out.writeBytes(" more");
+        aa_out.close();
+        list(fs, "rename2");
+  
+        //should not have any lease
+        assertEquals(0, countLease(cluster));
+      }
 
-      //should have 1 lease
-      assertEquals(1, countLease(cluster)); 
-      list(fs, "rename0");
-      fs.rename(a, b);
-      list(fs, "rename1");
-      aa_out.writeBytes(" more");
-      aa_out.close();
-      list(fs, "rename2");
+      { // test non-existent destination
+        Path dstPath = new Path("/c/d");
+        assertFalse(fs.exists(dstPath));
+        assertFalse(fs.rename(dir, dstPath));
+      }
 
-      //should not have any lease
-      assertEquals(0, countLease(cluster)); 
+      { // test rename /a/b to /a/b/c
+        Path src = new Path("/a/b");
+        Path dst = new Path("/a/b/c");
 
-      // test non-existent destination
-      Path dstPath = new Path("/c/d");
-      assertFalse(fs.exists(dstPath));
-      assertFalse(fs.rename(dir, dstPath));
+        DataOutputStream a_out = fs.create(new Path(src, "foo"));
+        a_out.writeBytes("something");
+        a_out.close();
+        
+        assertFalse(fs.rename(src, dst));
+      }
       
       fs.delete(dir, true);
     } finally {