Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-5709

Improve NameNode upgrade with existing reserved paths and path components

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0, 2.2.0
    • Fix Version/s: 2.4.0
    • Component/s: namenode
    • Labels:
    • Hadoop Flags:
      Reviewed

      Description

      Right now in trunk, upgrade fails messily if the old fsimage or edits refer to a directory named ".snapshot". We should at least print a better error message (which I believe was the original intention in HDFS-4666), and Aaron T. Myers proposed automatically renaming these files and directories.

      1. hdfs-5709-1.patch
        31 kB
        Andrew Wang
      2. hdfs-5709-2.patch
        31 kB
        Andrew Wang
      3. hdfs-5709-3.patch
        34 kB
        Andrew Wang
      4. hdfs-5709-4.patch
        35 kB
        Andrew Wang
      5. hdfs-5709-5.patch
        32 kB
        Andrew Wang
      6. hdfs-5709-6.patch
        43 kB
        Andrew Wang
      7. hdfs-5709-7.patch
        48 kB
        Andrew Wang
      8. hdfs-5709-addendum.patch
        1 kB
        Andrew Wang

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1690 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1690/)
          Addendum patch for HDFS-5709, add missing license header. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564885)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1690 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1690/ ) Addendum patch for HDFS-5709 , add missing license header. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564885 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #1665 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1665/)
          Addendum patch for HDFS-5709, add missing license header. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564885)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #1665 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1665/ ) Addendum patch for HDFS-5709 , add missing license header. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564885 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Yarn-trunk #473 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/473/)
          Addendum patch for HDFS-5709, add missing license header. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564885)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #473 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/473/ ) Addendum patch for HDFS-5709 , add missing license header. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564885 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #5112 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5112/)
          Addendum patch for HDFS-5709, add missing license header. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564885)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #5112 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5112/ ) Addendum patch for HDFS-5709 , add missing license header. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564885 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java
          Hide
          Aaron T. Myers added a comment -

          Whoops, sorry I missed that. Thanks for taking care of it, Andrew.

          Show
          Aaron T. Myers added a comment - Whoops, sorry I missed that. Thanks for taking care of it, Andrew.
          Hide
          Andrew Wang added a comment -

          I realize that I didn't put a license header on the new test file. Here's an addendum to fix it up, I'll commit to the relevant branches.

          Show
          Andrew Wang added a comment - I realize that I didn't put a license header on the new test file. Here's an addendum to fix it up, I'll commit to the relevant branches.
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Hdfs-trunk #1664 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1664/)
          HDFS-5709. Improve NameNode upgrade with existing reserved paths and path components. Contributed by Andrew Wang. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564645)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsConstants.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImageFormat.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/apt/HdfsUserGuide.apt.vm
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/xdoc/HdfsSnapshots.xml
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUpgradeFromImage.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/hadoop-2-reserved.tgz
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk #1664 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1664/ ) HDFS-5709 . Improve NameNode upgrade with existing reserved paths and path components. Contributed by Andrew Wang. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564645 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsConstants.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImageFormat.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/apt/HdfsUserGuide.apt.vm /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/xdoc/HdfsSnapshots.xml /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUpgradeFromImage.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/hadoop-2-reserved.tgz
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #1689 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1689/)
          HDFS-5709. Improve NameNode upgrade with existing reserved paths and path components. Contributed by Andrew Wang. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564645)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsConstants.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImageFormat.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/apt/HdfsUserGuide.apt.vm
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/xdoc/HdfsSnapshots.xml
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUpgradeFromImage.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/hadoop-2-reserved.tgz
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1689 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1689/ ) HDFS-5709 . Improve NameNode upgrade with existing reserved paths and path components. Contributed by Andrew Wang. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564645 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsConstants.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImageFormat.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/apt/HdfsUserGuide.apt.vm /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/xdoc/HdfsSnapshots.xml /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUpgradeFromImage.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/hadoop-2-reserved.tgz
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Yarn-trunk #472 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/472/)
          HDFS-5709. Improve NameNode upgrade with existing reserved paths and path components. Contributed by Andrew Wang. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564645)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsConstants.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImageFormat.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/apt/HdfsUserGuide.apt.vm
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/xdoc/HdfsSnapshots.xml
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUpgradeFromImage.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/hadoop-2-reserved.tgz
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #472 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/472/ ) HDFS-5709 . Improve NameNode upgrade with existing reserved paths and path components. Contributed by Andrew Wang. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564645 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsConstants.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImageFormat.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/apt/HdfsUserGuide.apt.vm /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/xdoc/HdfsSnapshots.xml /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUpgradeFromImage.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/hadoop-2-reserved.tgz
          Hide
          Aaron T. Myers added a comment -

          I've just committed this to trunk and branch-2.

          Thanks a lot for the contribution, Andrew. Thanks also to Jing and Suresh for the reviews and discussion.

          Show
          Aaron T. Myers added a comment - I've just committed this to trunk and branch-2. Thanks a lot for the contribution, Andrew. Thanks also to Jing and Suresh for the reviews and discussion.
          Hide
          Aaron T. Myers added a comment -

          The javadoc issue is unrelated and is tracked by HADOOP-10325. The TestAuditLogs failure is spurious and is tracked by HDFS-5882. The TestDFSUpgradeFromImage failure is because we need to include the new binary file in order for that to pass.

          Given that, I'm going to commit this momentarily.

          Show
          Aaron T. Myers added a comment - The javadoc issue is unrelated and is tracked by HADOOP-10325 . The TestAuditLogs failure is spurious and is tracked by HDFS-5882 . The TestDFSUpgradeFromImage failure is because we need to include the new binary file in order for that to pass. Given that, I'm going to commit this momentarily.
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #5109 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5109/)
          HDFS-5709. Improve NameNode upgrade with existing reserved paths and path components. Contributed by Andrew Wang. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564645)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsConstants.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImageFormat.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/apt/HdfsUserGuide.apt.vm
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/xdoc/HdfsSnapshots.xml
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUpgradeFromImage.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/hadoop-2-reserved.tgz
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #5109 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5109/ ) HDFS-5709 . Improve NameNode upgrade with existing reserved paths and path components. Contributed by Andrew Wang. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1564645 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSUtil.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/HdfsConstants.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImageFormat.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/apt/HdfsUserGuide.apt.vm /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/site/xdoc/HdfsSnapshots.xml /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSUpgradeFromImage.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNameNodeOptionParsing.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/hadoop-2-reserved.tgz
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12627049/hdfs-5709-7.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 javadoc. The javadoc tool appears to have generated 2 warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          -1 release audit. The applied patch generated 1 release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.server.namenode.TestAuditLogs
          org.apache.hadoop.hdfs.TestDFSUpgradeFromImage

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6034//testReport/
          Release audit warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/6034//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6034//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12627049/hdfs-5709-7.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 2 warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. -1 release audit . The applied patch generated 1 release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.namenode.TestAuditLogs org.apache.hadoop.hdfs.TestDFSUpgradeFromImage +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6034//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/6034//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6034//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          The latest patch looks good to me. +1 pending Jenkins.

          Show
          Aaron T. Myers added a comment - The latest patch looks good to me. +1 pending Jenkins.
          Hide
          Andrew Wang added a comment -

          Thanks ATM and Jing for reviewing this! I updated the docs, added command line arg testing for upgrade, and also changed the LV name behavior.

          Jing, right now I'm using the presence of the k/vs in the map to indicate that the "-renameReserved" flag was passed at all, which is why I didn't statically initialize the map with default values. I could switch it to use a boolean instead, but (with ATM's suggestion) we now have the same default suffix for all reserved paths, so adding a new default is as easy as putting it into the new static array in HdfsConstants.

          Show
          Andrew Wang added a comment - Thanks ATM and Jing for reviewing this! I updated the docs, added command line arg testing for upgrade, and also changed the LV name behavior. Jing, right now I'm using the presence of the k/vs in the map to indicate that the "-renameReserved" flag was passed at all, which is why I didn't statically initialize the map with default values. I could switch it to use a boolean instead, but (with ATM's suggestion) we now have the same default suffix for all reserved paths, so adding a new default is as easy as putting it into the new static array in HdfsConstants.
          Hide
          Jing Zhao added a comment -

          The v6 patch looks good to me. Besides the comments from Aaron T. Myers, just one minor comment:
          in useDefaultRenameReservedPairs, instead of calling setRenameReservedMapInternal, can we just directly add the two default reserved pairs into the map? With this change we can avoid some unnecessary string processing and make it easier to add other default reserved pairs in the future.

          Show
          Jing Zhao added a comment - The v6 patch looks good to me. Besides the comments from Aaron T. Myers , just one minor comment: in useDefaultRenameReservedPairs, instead of calling setRenameReservedMapInternal, can we just directly add the two default reserved pairs into the map? With this change we can avoid some unnecessary string processing and make it easier to add other default reserved pairs in the future.
          Hide
          Aaron T. Myers added a comment -

          Latest patch looks pretty good to me. Three small comments:

          1. The additional docs are now wrong - they still mention the configuration variable.
          2. I'd recommend factoring out the NN args processing code into some static function that you could write some tests for.
          3. Perhaps I misunderstand Suresh's suggestion, but I was under the impression that the default renamed name would include the current layout version, not the layout version that added the feature making the name reserved. I don't feel super strongly about this, but it surprised me to see it this way.

          +1 once these are addressed.

          Show
          Aaron T. Myers added a comment - Latest patch looks pretty good to me. Three small comments: The additional docs are now wrong - they still mention the configuration variable. I'd recommend factoring out the NN args processing code into some static function that you could write some tests for. Perhaps I misunderstand Suresh's suggestion, but I was under the impression that the default renamed name would include the current layout version, not the layout version that added the feature making the name reserved. I don't feel super strongly about this, but it surprised me to see it this way. +1 once these are addressed.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12626425/hdfs-5709-6.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestDFSUpgradeFromImage

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6006//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6006//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12626425/hdfs-5709-6.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDFSUpgradeFromImage +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6006//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6006//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12626425/hdfs-5709-6.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestDFSUpgradeFromImage
          org.apache.hadoop.hdfs.server.namenode.ha.TestDFSUpgradeWithHA

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6005//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6005//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12626425/hdfs-5709-6.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDFSUpgradeFromImage org.apache.hadoop.hdfs.server.namenode.ha.TestDFSUpgradeWithHA +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6005//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6005//console This message is automatically generated.
          Hide
          Andrew Wang added a comment -

          Reattach with binary diff, so reviewers can use try out the test image too.

          Show
          Andrew Wang added a comment - Reattach with binary diff, so reviewers can use try out the test image too.
          Hide
          Andrew Wang added a comment -

          New patch attached.

          • New flag -renameReserved [kv-pairs] can be specified with -upgrade to do auto renaming. We support both the default of .reserved.LV.UPGRADE_RENAMED, and override with the optional kv pairs from the command line. The KV pairs undergo some basic validation (e.g. you can't set .reserved=.snapshot).
          • Rename "/.reserved" too. I think this reservation was added by Feature.ADD_INODE_ID, please correct me if that's wrong and I'll fix it up.
          • Upon encountering a reserved path, we throw a Precondition explaining the reserved paths and how to manually and automatically rename them.
          • I beefed up the test case, which is a quick way of seeing how this works.
          • Also played around manually with the test image to make sure that errors cases work as expected
          Show
          Andrew Wang added a comment - New patch attached. New flag -renameReserved [kv-pairs] can be specified with -upgrade to do auto renaming. We support both the default of .reserved.LV.UPGRADE_RENAMED , and override with the optional kv pairs from the command line. The KV pairs undergo some basic validation (e.g. you can't set .reserved=.snapshot ). Rename "/.reserved" too. I think this reservation was added by Feature.ADD_INODE_ID , please correct me if that's wrong and I'll fix it up. Upon encountering a reserved path, we throw a Precondition explaining the reserved paths and how to manually and automatically rename them. I beefed up the test case, which is a quick way of seeing how this works. Also played around manually with the test image to make sure that errors cases work as expected
          Hide
          Suresh Srinivas added a comment -

          Andrew Wang, that looks good.

          The likelihood of collision in case of .snapshot.LV.UPGRADE_RENAMED is probably very low. When namenode fails to ugprade due to reserved name collision, it should print out all the list of reserved names in the file system along with the error need to do -upgrade with -renameReserved flag. That way users know to pass all the reserved names and their corresponding preferred name, if they choose to use key/value pairs.

          Show
          Suresh Srinivas added a comment - Andrew Wang , that looks good. The likelihood of collision in case of .snapshot.LV.UPGRADE_RENAMED is probably very low. When namenode fails to ugprade due to reserved name collision, it should print out all the list of reserved names in the file system along with the error need to do -upgrade with -renameReserved flag. That way users know to pass all the reserved names and their corresponding preferred name, if they choose to use key/value pairs.
          Hide
          Andrew Wang added a comment -

          I had a quick call with Suresh to hash this out, and we arrived at the following which should be suitable for everyone:

          • Rather than a configuration option which can stick around forever, an additional command line flag (e.g. "-upgrade -renameReserved") is better. This way we worry about it once, and there are no lingering effects.
          • We default to renaming reserved paths to a convention like .snapshot.LV.UPGRADE_RENAMED, but also allow users to pass key/value pairs on the command line, e.g. "-upgrade -renameReserved .snapshot=.user-snapshot". In either case, we should do our best to detect collisions, but it's hard with the edit log.
          • It'd be good to do this for "/.reserved" too, which will help demonstrate that this is a generic solution.

          I think this is an accurate summary, so I'll start revving the patch as per above. Please comment if something is still off.

          Show
          Andrew Wang added a comment - I had a quick call with Suresh to hash this out, and we arrived at the following which should be suitable for everyone: Rather than a configuration option which can stick around forever, an additional command line flag (e.g. "-upgrade -renameReserved") is better. This way we worry about it once, and there are no lingering effects. We default to renaming reserved paths to a convention like .snapshot.LV.UPGRADE_RENAMED , but also allow users to pass key/value pairs on the command line, e.g. "-upgrade -renameReserved .snapshot=.user-snapshot". In either case, we should do our best to detect collisions, but it's hard with the edit log. It'd be good to do this for "/.reserved" too, which will help demonstrate that this is a generic solution. I think this is an accurate summary, so I'll start revving the patch as per above. Please comment if something is still off.
          Hide
          Aaron T. Myers added a comment -

          The configuration lingers on for ever, unless and informed user can get rid of it. That is the reason why configuration is perhaps not the best way to do this.

          I honestly don't understand what the aversion is to having this be configurable. Having a setting in an XML file longer than it strictly needs to be doesn't do any harm.

          Or, just rename it based on convention as I had proposed. User can either glean this from the log (preferred) or from searching fsimage the renamed paths and rename them as they wish. Given that the likelihood of users running into this conflict in the first place is low, this should be acceptable.

          I'm fine renaming it based on some convention by default, but I feel strongly that the rename behavior should be optionally configurable. That's what I proposed above as a compromise.

          Show
          Aaron T. Myers added a comment - The configuration lingers on for ever, unless and informed user can get rid of it. That is the reason why configuration is perhaps not the best way to do this. I honestly don't understand what the aversion is to having this be configurable. Having a setting in an XML file longer than it strictly needs to be doesn't do any harm. Or, just rename it based on convention as I had proposed. User can either glean this from the log (preferred) or from searching fsimage the renamed paths and rename them as they wish. Given that the likelihood of users running into this conflict in the first place is low, this should be acceptable. I'm fine renaming it based on some convention by default, but I feel strongly that the rename behavior should be optionally configurable. That's what I proposed above as a compromise.
          Hide
          Suresh Srinivas added a comment -

          This feels procedurally pretty similar to the alternative of a special startup option, and it's only required to be set if we hit a .snapshot path. Most users will never need to worry about this config.

          The problem is, now we have a configuration that needs to be added to NameNode for one time upgrade. The configuration lingers on for ever, unless and informed user can get rid of it. That is the reason why configuration is perhaps not the best way to do this.

          The mechanism I and Jing Zhao talked about addresses .reserved as well. Do you plan to address that in this jira?

          ...that implies scanning the fsimage and logs initially to enumerate all the .snapshot paths, have the user configure their rename rules...

          Or, just rename it based on convention as I had proposed. User can either glean this from the log (preferred) or from searching fsimage the renamed paths and rename them as they wish. Given that the likelihood of users running into this conflict in the first place is low, this should be acceptable.

          Show
          Suresh Srinivas added a comment - This feels procedurally pretty similar to the alternative of a special startup option, and it's only required to be set if we hit a .snapshot path. Most users will never need to worry about this config. The problem is, now we have a configuration that needs to be added to NameNode for one time upgrade. The configuration lingers on for ever, unless and informed user can get rid of it. That is the reason why configuration is perhaps not the best way to do this. The mechanism I and Jing Zhao talked about addresses .reserved as well. Do you plan to address that in this jira? ...that implies scanning the fsimage and logs initially to enumerate all the .snapshot paths, have the user configure their rename rules... Or, just rename it based on convention as I had proposed. User can either glean this from the log (preferred) or from searching fsimage the renamed paths and rename them as they wish. Given that the likelihood of users running into this conflict in the first place is low, this should be acceptable.
          Hide
          Andrew Wang added a comment -

          I'm not sure if this adds anything to the discussion, but the current workflow in the patch is:

          • By default, the config option is not set
          • On upgrade, if we hit a .snapshot path, we throw an exception and abort telling the user to set the config option
          • When the config option is set, there are log prints whenever something is renamed

          This feels procedurally pretty similar to the alternative of a special startup option, and it's only required to be set if we hit a .snapshot path. Most users will never need to worry about this config.

          I agree that doing renames with the context of the full parent path (e.g. /X/.snapshot renames to /X/.myx-snapshot, /Y/.snapshot renames to /Y/.myy.snapshot) would be more powerful, but IIUC that implies scanning the fsimage and logs initially to enumerate all the .snapshot paths, have the user configure their rename rules, then load the fsimage/logs again to do the rename.

          Suresh, does any of this influence your view? I'd really like to move forward on this issue.

          Show
          Andrew Wang added a comment - I'm not sure if this adds anything to the discussion, but the current workflow in the patch is: By default, the config option is not set On upgrade, if we hit a .snapshot path, we throw an exception and abort telling the user to set the config option When the config option is set, there are log prints whenever something is renamed This feels procedurally pretty similar to the alternative of a special startup option, and it's only required to be set if we hit a .snapshot path. Most users will never need to worry about this config. I agree that doing renames with the context of the full parent path (e.g. /X/.snapshot renames to /X/.myx-snapshot, /Y/.snapshot renames to /Y/.myy.snapshot) would be more powerful, but IIUC that implies scanning the fsimage and logs initially to enumerate all the .snapshot paths, have the user configure their rename rules, then load the fsimage/logs again to do the rename. Suresh, does any of this influence your view? I'd really like to move forward on this issue.
          Hide
          Aaron T. Myers added a comment -

          Where is this being proposed? Why is downgrade required in any of the proposed solutions?

          The alternative to automatically renaming files is to have to do this, but I see now that your point is just to more forcefully make users aware that a rename is taking place. I personally think this is overkill, but I don't feel super strongly about this.

          Doing the same thing in multiple ways seems unnecessary and confusing. I do not see any significant advantage to it. Perhaps I do not understand this solution.

          It won't be confusing at all for anyone who doesn't want it, since they'll just never set the config setting at all. For those users who want the rename to be a bit configurable, however, it will be a significant convenience. This isn't really any different than making something configurable but also setting a reasonable default that most users will not change.

          Show
          Aaron T. Myers added a comment - Where is this being proposed? Why is downgrade required in any of the proposed solutions? The alternative to automatically renaming files is to have to do this, but I see now that your point is just to more forcefully make users aware that a rename is taking place. I personally think this is overkill, but I don't feel super strongly about this. Doing the same thing in multiple ways seems unnecessary and confusing. I do not see any significant advantage to it. Perhaps I do not understand this solution. It won't be confusing at all for anyone who doesn't want it, since they'll just never set the config setting at all. For those users who want the rename to be a bit configurable, however, it will be a significant convenience. This isn't really any different than making something configurable but also setting a reasonable default that most users will not change.
          Hide
          Suresh Srinivas added a comment -

          By "silently" I meant without logging anything, not that we shouldn't necessarily do the rename by default.

          By silently I mean without the active participation of the user. Logging into namenode logs, I can see many people not realizing the rename has occurred until some applications that depend on the path fail.

          Why would they prefer to have to manually downgrade the software, start up the cluster, manually find all reserved names, rename them, then re-attempt the upgrade?

          Where is this being proposed? Why is downgrade required in any of the proposed solutions?

          Suresh - what about the other part of my proposal, i.e. by default perform the rename to the name you suggested ("<original file name><layout_version>.reserved_renamed_after_ugprade"), but optionally rename to something else?

          Doing the same thing in multiple ways seems unnecessary and confusing. I do not see any significant advantage to it. Perhaps I do not understand this solution.

          Show
          Suresh Srinivas added a comment - By "silently" I meant without logging anything, not that we shouldn't necessarily do the rename by default. By silently I mean without the active participation of the user. Logging into namenode logs, I can see many people not realizing the rename has occurred until some applications that depend on the path fail. Why would they prefer to have to manually downgrade the software, start up the cluster, manually find all reserved names, rename them, then re-attempt the upgrade? Where is this being proposed? Why is downgrade required in any of the proposed solutions? Suresh - what about the other part of my proposal, i.e. by default perform the rename to the name you suggested ("<original file name><layout_version>.reserved_renamed_after_ugprade"), but optionally rename to something else? Doing the same thing in multiple ways seems unnecessary and confusing. I do not see any significant advantage to it. Perhaps I do not understand this solution.
          Hide
          Aaron T. Myers added a comment -

          One of the requirements we considered is, the files should not be renamed silently. Hence the start up option that a user must pass. This is the same reason why we invented configuration option as well.

          By "silently" I meant without logging anything, not that we shouldn't necessarily do the rename by default. The question I still have is - why would any user not want to automatically rename the files? Why would they prefer to have to manually downgrade the software, start up the cluster, manually find all reserved names, rename them, then re-attempt the upgrade? This seems strictly inferior to the alternative of automatically renaming and then allowing the user to manually do the rename afterward.

          Suresh - what about the other part of my proposal, i.e. by default perform the rename to the name you suggested ("<original file name>.<layout_version>.reserved_renamed_after_ugprade"), but optionally rename to something else? Are you OK with that?

          Show
          Aaron T. Myers added a comment - One of the requirements we considered is, the files should not be renamed silently. Hence the start up option that a user must pass. This is the same reason why we invented configuration option as well. By "silently" I meant without logging anything, not that we shouldn't necessarily do the rename by default. The question I still have is - why would any user not want to automatically rename the files? Why would they prefer to have to manually downgrade the software, start up the cluster, manually find all reserved names, rename them, then re-attempt the upgrade? This seems strictly inferior to the alternative of automatically renaming and then allowing the user to manually do the rename afterward. Suresh - what about the other part of my proposal, i.e. by default perform the rename to the name you suggested ("<original file name>.<layout_version>.reserved_renamed_after_ugprade"), but optionally rename to something else? Are you OK with that?
          Hide
          Suresh Srinivas added a comment -

          I don't think we should be introducing another startup option for this, mostly because I can't imagine a user not wanting to automatically rename files that use names that are now reserved.

          One of the requirements we considered is, the files should not be renamed silently. Hence the start up option that a user must pass. This is the same reason why we invented configuration option as well.

          Most of the time this option is not necessary (true with configuration as well). But we could fail the namenode upgrade by indicating "upgrade must be performed with rename flag because the reserved file names are used...". Only a very small set of users will hit this issue and will have to start the upgrade again with -rename_reserved_names flag.

          Show
          Suresh Srinivas added a comment - I don't think we should be introducing another startup option for this, mostly because I can't imagine a user not wanting to automatically rename files that use names that are now reserved. One of the requirements we considered is, the files should not be renamed silently. Hence the start up option that a user must pass. This is the same reason why we invented configuration option as well. Most of the time this option is not necessary (true with configuration as well). But we could fail the namenode upgrade by indicating "upgrade must be performed with rename flag because the reserved file names are used...". Only a very small set of users will hit this issue and will have to start the upgrade again with -rename_reserved_names flag.
          Hide
          Aaron T. Myers added a comment -

          The user must run -upgrade with a new option that allows renaming of reserved file.

          I don't think we should be introducing another startup option for this, mostly because I can't imagine a user not wanting to automatically rename files that use names that are now reserved. The current process of having to downgrade the software, start the cluster, rename all the files (not all of which may have been found during the attempted upgrade!) is so onerous that we really should just do it for users.

          I do not understand what the pain is. It is just renaming the files.

          The pain is in the difficulty of automating this. A tool would have to either do the equivalent of `find / -name ...' on the file system, which in a very large HDFS instance might be untenable, or parse the NN startup logs which is an error-prone thing to try to do.

          How about the following proposal, which hopefully appeases everyone:

          • We do renames of reserved names by default, for the reasons stated above.
          • By default we do as Suresh suggested and rename all reserved file names to "<original file name>.<layout_version>.reserved_renamed_after_ugprade"
          • Optionally, one can set a single configuration parameter as Andrew proposed ('e.g. ".snapshot=.user-snapshot,.some-new-reserved=.renamed-new-reserved"'). If on renaming a reserved file name it's in this configured list, we use that for the substitution. If it's not present in this configured list, then we do the default as Suresh suggested.
          Show
          Aaron T. Myers added a comment - The user must run -upgrade with a new option that allows renaming of reserved file. I don't think we should be introducing another startup option for this, mostly because I can't imagine a user not wanting to automatically rename files that use names that are now reserved. The current process of having to downgrade the software, start the cluster, rename all the files (not all of which may have been found during the attempted upgrade!) is so onerous that we really should just do it for users. I do not understand what the pain is. It is just renaming the files. The pain is in the difficulty of automating this. A tool would have to either do the equivalent of `find / -name ...' on the file system, which in a very large HDFS instance might be untenable, or parse the NN startup logs which is an error-prone thing to try to do. How about the following proposal, which hopefully appeases everyone: We do renames of reserved names by default, for the reasons stated above. By default we do as Suresh suggested and rename all reserved file names to "<original file name>.<layout_version>.reserved_renamed_after_ugprade" Optionally, one can set a single configuration parameter as Andrew proposed ('e.g. ".snapshot=.user-snapshot,.some-new-reserved=.renamed-new-reserved"'). If on renaming a reserved file name it's in this configured list, we use that for the substitution. If it's not present in this configured list, then we do the default as Suresh suggested.
          Hide
          Suresh Srinivas added a comment -

          could you lay out your alternative proposal for a conf option? I could rename the conf and make it so it takes a delimited set of kv pairs, e.g. ".snapshot=.user-snapshot,.some-new-reserved=.renamed-new-reserved", but I felt that was kind of ugly. I wanted full sub rather than prefix here for flexibility.

          This is not what I have in mind. /<path>/<reserved_file> could be renamed to /<path>/<reserved_file>+<configured_rename_suffix>. The user finds all the renamed files (from the log) and renames them once the system comes up, if necessary. In fact coming to think of it, I think the suffix should probably be not configurable either. The system can choose some convention where rename suffix could be - ".<layout_version>.reserved_renamed_after_ugprade". The user must run -upgrade with a new option that allows renaming of reserved file.

          The advantages of this are:

          1. Post upgrade at any time (until user renames the file), all the renamed files can be found
          2. The probability of conflict of file names during upgrade is lower
          3. Unnecessary configuration changes are avoided

          With just a prefix mutation, I could easily imagine having to run some operation after the NN had started up to then find all of the renamed paths and again rename them to some other name with the prefix removed. That's a pain that we shouldn't put our users through.

          I do not understand what the pain is. It is just renaming the files. Also what if a user wants to rename .snapshot in one directory to x and .snapshot in another directory to y, based on the context of how the file is being used.

          Empirically we rarely add reserved names (only two in the lifetime of HDFS so far) and I don't anticipate adding many more

          If we looked at this same question last year, we would have said we will never have reserved names in HDFS at all. I think there are some that I have plans of adding. I want to add some directories in the future for storing file system specific information. This is the way I envision moving fsimage and possibly finalized editlog segments into HDFS itself.

          ... but rather to do as Andrew suggests and add a conf option per reserved name, and add the code necessary for each one as it comes up....

          I think adding one configuration for each reserved name does not seem right. In fact, if possible we should avoid configuration changes at all for renames that are required one time during upgrade to a version that adds reserved file names.

          Show
          Suresh Srinivas added a comment - could you lay out your alternative proposal for a conf option? I could rename the conf and make it so it takes a delimited set of kv pairs, e.g. ".snapshot=.user-snapshot,.some-new-reserved=.renamed-new-reserved", but I felt that was kind of ugly. I wanted full sub rather than prefix here for flexibility. This is not what I have in mind. /<path>/<reserved_file> could be renamed to /<path>/<reserved_file>+<configured_rename_suffix>. The user finds all the renamed files (from the log) and renames them once the system comes up, if necessary. In fact coming to think of it, I think the suffix should probably be not configurable either. The system can choose some convention where rename suffix could be - ".<layout_version>.reserved_renamed_after_ugprade". The user must run -upgrade with a new option that allows renaming of reserved file. The advantages of this are: Post upgrade at any time (until user renames the file), all the renamed files can be found The probability of conflict of file names during upgrade is lower Unnecessary configuration changes are avoided With just a prefix mutation, I could easily imagine having to run some operation after the NN had started up to then find all of the renamed paths and again rename them to some other name with the prefix removed. That's a pain that we shouldn't put our users through. I do not understand what the pain is. It is just renaming the files. Also what if a user wants to rename .snapshot in one directory to x and .snapshot in another directory to y, based on the context of how the file is being used. Empirically we rarely add reserved names (only two in the lifetime of HDFS so far) and I don't anticipate adding many more If we looked at this same question last year, we would have said we will never have reserved names in HDFS at all. I think there are some that I have plans of adding. I want to add some directories in the future for storing file system specific information. This is the way I envision moving fsimage and possibly finalized editlog segments into HDFS itself. ... but rather to do as Andrew suggests and add a conf option per reserved name, and add the code necessary for each one as it comes up.... I think adding one configuration for each reserved name does not seem right. In fact, if possible we should avoid configuration changes at all for renames that are required one time during upgrade to a version that adds reserved file names.
          Hide
          Aaron T. Myers added a comment -

          Hey guys,

          Jing - I'm not in favor of a generic prefix for all reserved names because I don't think it's flexible enough. A user should be able to rename a reserved component name to a specific other reserved component name. With just a prefix mutation, I could easily imagine having to run some operation after the NN had started up to then find all of the renamed paths and again rename them to some other name with the prefix removed. That's a pain that we shouldn't put our users through.

          Suresh - with regard to the suggestion that we have a completely generic system of renaming reserved component names, how strongly do you see that as a requirement? I personally view that as overkill for two reasons: 1) Empirically we rarely add reserved names (only two in the lifetime of HDFS so far) and I don't anticipate adding many more, and 2) we can't necessarily anticipate the precise semantics of all reserved names. For example, ".reserved" should only be considered invalid and renamed if it's in the root directory. I think the solution for that is not to develop a completely generic system for renaming path components, but rather to do as Andrew suggests and add a conf option per reserved name, and add the code necessary for each one as it comes up.

          I'm in favor of checking in this patch as-is. If we later decide to add a more generic system, we can do that and quite easily maintain backward compatibility of this conf option with that system. I wouldn't personally object to implementing a K/V config setting system as Andrew suggested, but again, seems like overkill to me.

          Show
          Aaron T. Myers added a comment - Hey guys, Jing - I'm not in favor of a generic prefix for all reserved names because I don't think it's flexible enough. A user should be able to rename a reserved component name to a specific other reserved component name. With just a prefix mutation, I could easily imagine having to run some operation after the NN had started up to then find all of the renamed paths and again rename them to some other name with the prefix removed. That's a pain that we shouldn't put our users through. Suresh - with regard to the suggestion that we have a completely generic system of renaming reserved component names, how strongly do you see that as a requirement? I personally view that as overkill for two reasons: 1) Empirically we rarely add reserved names (only two in the lifetime of HDFS so far) and I don't anticipate adding many more, and 2) we can't necessarily anticipate the precise semantics of all reserved names. For example, ".reserved" should only be considered invalid and renamed if it's in the root directory. I think the solution for that is not to develop a completely generic system for renaming path components, but rather to do as Andrew suggests and add a conf option per reserved name, and add the code necessary for each one as it comes up. I'm in favor of checking in this patch as-is. If we later decide to add a more generic system, we can do that and quite easily maintain backward compatibility of this conf option with that system. I wouldn't personally object to implementing a K/V config setting system as Andrew suggested, but again, seems like overkill to me.
          Hide
          Jing Zhao added a comment -

          I could rename the conf and make it so it takes a delimited set of kv pairs

          We can define a generic prefix for all the reserved names, and simply generate a new name by adding the prefix to the reserved names.

          Show
          Jing Zhao added a comment - I could rename the conf and make it so it takes a delimited set of kv pairs We can define a generic prefix for all the reserved names, and simply generate a new name by adding the prefix to the reserved names.
          Hide
          Andrew Wang added a comment -

          Hi Suresh, could you lay out your alternative proposal for a conf option? I could rename the conf and make it so it takes a delimited set of kv pairs, e.g. ".snapshot=.user-snapshot,.some-new-reserved=.renamed-new-reserved", but I felt that was kind of ugly. I wanted full sub rather than prefix here for flexibility.

          I also honestly don't think there will be that many more, if any, reserved path components in the future. I'm trying to dig up some listing of reserved paths in other FSs as a guide, but I'm coming up dry besides http://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words which has the definitely quirky behavior of FAT and NTFS, and obviously WAFL's .snapshot.

          Show
          Andrew Wang added a comment - Hi Suresh, could you lay out your alternative proposal for a conf option? I could rename the conf and make it so it takes a delimited set of kv pairs, e.g. ".snapshot=.user-snapshot,.some-new-reserved=.renamed-new-reserved", but I felt that was kind of ugly. I wanted full sub rather than prefix here for flexibility. I also honestly don't think there will be that many more, if any, reserved path components in the future. I'm trying to dig up some listing of reserved paths in other FSs as a guide, but I'm coming up dry besides http://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words which has the definitely quirky behavior of FAT and NTFS, and obviously WAFL's .snapshot.
          Hide
          Suresh Srinivas added a comment -

          Since I don't foresee us adding that many more reserved paths, a config option per seems like a tolerable overhead.

          We already have another path .reserved under root directory that is a reserved name. I agree with the suggestion that Jing has made earlier. We should have a generic configuration that can be used to replace any reserved names we may want to add in the future, instead of for .snapshot alone in this jira. This way we do not need to support the .snapshot only configuration for backward compatibility reasons.

          Show
          Suresh Srinivas added a comment - Since I don't foresee us adding that many more reserved paths, a config option per seems like a tolerable overhead. We already have another path .reserved under root directory that is a reserved name. I agree with the suggestion that Jing has made earlier. We should have a generic configuration that can be used to replace any reserved names we may want to add in the future, instead of for .snapshot alone in this jira. This way we do not need to support the .snapshot only configuration for backward compatibility reasons.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12624021/hdfs-5709-5.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestDFSUpgradeFromImage

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5922//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5922//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12624021/hdfs-5709-5.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDFSUpgradeFromImage +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5922//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5922//console This message is automatically generated.
          Hide
          Andrew Wang added a comment -

          Hey Suresh,

          Sure, I can summarize. What this patch does essentially is pass all path components during FSImage loading and paths during edit log loading through new renameReserved* methods. Right now these methods just have an if check for if we're upgrading from a version that doesn't support snapshots, but it's easy to add more if statements for other LVs with reserved components. Jing had some suggestions about how we can make the renaming process a little more generic, but I think he and I agree that it should be easy to do this later.

          The only difference from your prior proposed solution is that I wanted a more expressive config option than a prefix, so went with a full substitution instead. This lets users rename .snapshot to .user-snapshot, not just MYPREFIX.snapshot. Since I don't foresee us adding that many more reserved paths, a config option per seems like a tolerable overhead.

          Show
          Andrew Wang added a comment - Hey Suresh, Sure, I can summarize. What this patch does essentially is pass all path components during FSImage loading and paths during edit log loading through new renameReserved* methods. Right now these methods just have an if check for if we're upgrading from a version that doesn't support snapshots, but it's easy to add more if statements for other LVs with reserved components. Jing had some suggestions about how we can make the renaming process a little more generic, but I think he and I agree that it should be easy to do this later. The only difference from your prior proposed solution is that I wanted a more expressive config option than a prefix, so went with a full substitution instead. This lets users rename .snapshot to .user-snapshot , not just MYPREFIX.snapshot . Since I don't foresee us adding that many more reserved paths, a config option per seems like a tolerable overhead.
          Hide
          Suresh Srinivas added a comment -

          Andrew, can you please summarize the solution. I want to make sure that the renaming is not being done only for .snapshot, but in a generic way.

          Show
          Suresh Srinivas added a comment - Andrew, can you please summarize the solution. I want to make sure that the renaming is not being done only for .snapshot, but in a generic way.
          Hide
          Jing Zhao added a comment -

          Thanks Andrew! +1 pending Jenkins.

          Show
          Jing Zhao added a comment - Thanks Andrew! +1 pending Jenkins.
          Hide
          Andrew Wang added a comment -

          Thanks Jing, that clarifies it for me. Newest rev does what you suggest.

          Show
          Andrew Wang added a comment - Thanks Jing, that clarifies it for me. Newest rev does what you suggest.
          Hide
          Jing Zhao added a comment -

          because it catches an additional class of errors (e.g. ".." is valid in some paths, but not others)

          In the current DFSUtil#isValideName, ".." is allowed only when the path starts with "/.reserved/.inodes", and this is used by NFS to use an inode id to indicate a file/directory. For a real inode's local name, ".." should still be disallowed. (HDFS-5104 made this change)

          Thus I think we should also add checking for ".." in isValidComponent, and then we actually can cover all the cases and we can remove the isValidName check.

          Show
          Jing Zhao added a comment - because it catches an additional class of errors (e.g. ".." is valid in some paths, but not others) In the current DFSUtil#isValideName, ".." is allowed only when the path starts with "/.reserved/.inodes", and this is used by NFS to use an inode id to indicate a file/directory. For a real inode's local name, ".." should still be disallowed. ( HDFS-5104 made this change) Thus I think we should also add checking for ".." in isValidComponent, and then we actually can cover all the cases and we can remove the isValidName check.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12623987/hdfs-5709-4.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestDFSUpgradeFromImage

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5921//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5921//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12623987/hdfs-5709-4.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDFSUpgradeFromImage +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5921//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5921//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          +1 pending Jenkins.

          Show
          Aaron T. Myers added a comment - +1 pending Jenkins.
          Hide
          Andrew Wang added a comment -

          Thanks again for the reviews everyone. v4 takes ATM's suggestions of javadoc and method name. Jing, I think I'd like to keep the DFSUtil#isValidName check for additional paranoia, and because it catches an additional class of errors (e.g. ".." is valid in some paths, but not others).

          Show
          Andrew Wang added a comment - Thanks again for the reviews everyone. v4 takes ATM's suggestions of javadoc and method name. Jing, I think I'd like to keep the DFSUtil#isValidName check for additional paranoia, and because it catches an additional class of errors (e.g. ".." is valid in some paths, but not others).
          Hide
          Jing Zhao added a comment -

          Thanks Andrew! The new patch looks good to me, +1. The only suggestion is that maybe we do not need the following preconditions check since we already check every component in renameReservedComponentOnUpgrade

          +      Preconditions.checkArgument(DFSUtil.isValidName(newPath),
          +          "Renamed path " + newPath + " is not a valid"
          +          + " name. Please verify the value of "
          +          + DFSConfigKeys.DFS_NAMENODE_UPGRADE_RENAME_RESERVED_SNAPSHOT);
          
          Show
          Jing Zhao added a comment - Thanks Andrew! The new patch looks good to me, +1. The only suggestion is that maybe we do not need the following preconditions check since we already check every component in renameReservedComponentOnUpgrade + Preconditions.checkArgument(DFSUtil.isValidName(newPath), + "Renamed path " + newPath + " is not a valid" + + " name. Please verify the value of " + + DFSConfigKeys.DFS_NAMENODE_UPGRADE_RENAME_RESERVED_SNAPSHOT);
          Hide
          Aaron T. Myers added a comment -

          Latest patch looks very good to me. My only suggestion is that DFSUtil#isValidComponent could include a little javadoc explaining when it should be used, e.g. not for verifying paths on read since it will reject components that are valid but reserved names. You might even consider renaming the function to something like "isValidNameForComponent" or something. That might be overkill, though.

          +1 from me once this is addressed. Jing, this look good to you?

          Show
          Aaron T. Myers added a comment - Latest patch looks very good to me. My only suggestion is that DFSUtil#isValidComponent could include a little javadoc explaining when it should be used, e.g. not for verifying paths on read since it will reject components that are valid but reserved names. You might even consider renaming the function to something like "isValidNameForComponent" or something. That might be overkill, though. +1 from me once this is addressed. Jing, this look good to you?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12623844/hdfs-5709-3.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestDFSUpgradeFromImage

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5916//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5916//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12623844/hdfs-5709-3.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDFSUpgradeFromImage +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5916//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5916//console This message is automatically generated.
          Hide
          Andrew Wang added a comment -

          Thanks for the review Jing, new patch addresses most of your comments except as follows:

          • I made the methods pass a String rather than a FSN. I think adding generic methods and image and edit wrappers is a little overkill right now, so I'd like to do that when we actually want to do this renaming for another reserved path. Should be straightforward.
          • I put a check with DFSUtil#isValidName when renaming a path. For image loading, it's mostly dealing with inodes and single path components, so I added another DFSUtil that should cover that.
          Show
          Andrew Wang added a comment - Thanks for the review Jing, new patch addresses most of your comments except as follows: I made the methods pass a String rather than a FSN. I think adding generic methods and image and edit wrappers is a little overkill right now, so I'd like to do that when we actually want to do this renaming for another reserved path. Should be straightforward. I put a check with DFSUtil#isValidName when renaming a path. For image loading, it's mostly dealing with inodes and single path components, so I added another DFSUtil that should cover that.
          Hide
          Jing Zhao added a comment -

          The patch looks good to me. Some comments:

          1. In FSEditLogLoader, we also need to handle "OP_ADD_BLOCK" after HDFS-5704 got committed recently.
          2. It will be better to let renameReservedPathsOnUpgrade and renameReservedComponentOnUpgrade take a string as the third parameter instead of an instance of FSNamesystem. We may also want to pass in the to-be-replaced string as a parameter, and in that case, we can make these two methods act as more generic utility methods so that it can be used for other reserved names (e.g., "/.reserved/.inodes", which we may want to handle in a separate jira).
          3. Then we can have another two methods in FSImageFormat/FSEditLogLoader to call the util methods, where we can check layoutversion and new names.
          4. When checking the new name, I think we should follow the rules in DFSUtil#isValidName?
          5. In renameReservedComponentOnUpgrade, maybe we do not need to convert the byte[] to string for the comparison? We have DOT_SNAPSHOT_DIR_BYTES defined in HdfsConstants already.
          6. There is an unused import in FSImageFormat.
          Show
          Jing Zhao added a comment - The patch looks good to me. Some comments: In FSEditLogLoader, we also need to handle "OP_ADD_BLOCK" after HDFS-5704 got committed recently. It will be better to let renameReservedPathsOnUpgrade and renameReservedComponentOnUpgrade take a string as the third parameter instead of an instance of FSNamesystem. We may also want to pass in the to-be-replaced string as a parameter, and in that case, we can make these two methods act as more generic utility methods so that it can be used for other reserved names (e.g., "/.reserved/.inodes", which we may want to handle in a separate jira). Then we can have another two methods in FSImageFormat/FSEditLogLoader to call the util methods, where we can check layoutversion and new names. When checking the new name, I think we should follow the rules in DFSUtil#isValidName? In renameReservedComponentOnUpgrade, maybe we do not need to convert the byte[] to string for the comparison? We have DOT_SNAPSHOT_DIR_BYTES defined in HdfsConstants already. There is an unused import in FSImageFormat.
          Hide
          Jing Zhao added a comment -

          Thanks for the work Andrew! I'm now reviewing the patch and will post my comments soon.

          Show
          Jing Zhao added a comment - Thanks for the work Andrew! I'm now reviewing the patch and will post my comments soon.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12623493/hdfs-5709-2.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestDFSUpgradeFromImage

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5904//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5904//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12623493/hdfs-5709-2.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDFSUpgradeFromImage +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5904//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5904//console This message is automatically generated.
          Hide
          Andrew Wang added a comment -

          Woops, I should have mentioned that one. The config only needs to be validated if it needs to be used (i.e. we encounter a .snapshot path), so I felt it was better to keep all the validation inside that method. Thanks again ATM.

          Show
          Andrew Wang added a comment - Woops, I should have mentioned that one. The config only needs to be validated if it needs to be used (i.e. we encounter a .snapshot path), so I felt it was better to keep all the validation inside that method. Thanks again ATM.
          Hide
          Aaron T. Myers added a comment -

          The latest patch looks pretty good to me. Docs look great, thanks for adding those.

          Looks like you also didn't take my suggestion to move the path separator check to NN startup, but I'm guessing you had your reasons. +1 once you explain that, and pending a clean Jenkins run.

          Show
          Aaron T. Myers added a comment - The latest patch looks pretty good to me. Docs look great, thanks for adding those. Looks like you also didn't take my suggestion to move the path separator check to NN startup, but I'm guessing you had your reasons. +1 once you explain that, and pending a clean Jenkins run.
          Hide
          Andrew Wang added a comment -

          Thanks for the review ATM, here's a new patch. I took all your comments as advised except for the static FSN method; I feel like it's better to pass around FSN than to make a new Configuration in a static context.

          Show
          Andrew Wang added a comment - Thanks for the review ATM, here's a new patch. I took all your comments as advised except for the static FSN method; I feel like it's better to pass around FSN than to make a new Configuration in a static context.
          Hide
          Aaron T. Myers added a comment -

          Patch looks pretty good to me. A few very small comments:

          1. I think this indentation is off:
            +     // Rename .snapshot paths if we're doing an upgrade
            +      parentPath = renameReservedPathsOnUpgrade(parentPath, getLayoutVersion(),
            +          namesystem);
            
          2. I think we should consider scrapping the renameSnapshotPathChecked variable. Seems like it's just for efficiency, but in practice this code should only be triggered a relatively small number of times (since it only happens for path components containing the reserved string) and I think we can reasonably expect the JIT to take care of it, since it's really only checking conditions that will never change for the lifetime of the process. You might also consider moving the check that the replacement string doesn't contain the path separator to NN startup time, since you can reasonably check the validity of that config setting without there being any reserved names in the namespace.
          3. You might consider making FSNamesystem#renameSnapshotPath static so that you don't have to pass a reference to the FSNamesystem into the FSImageFormat methods.
          4. You should add some documentation about this setting to the snapshots docs.
          Show
          Aaron T. Myers added a comment - Patch looks pretty good to me. A few very small comments: I think this indentation is off: + // Rename .snapshot paths if we're doing an upgrade + parentPath = renameReservedPathsOnUpgrade(parentPath, getLayoutVersion(), + namesystem); I think we should consider scrapping the renameSnapshotPathChecked variable. Seems like it's just for efficiency, but in practice this code should only be triggered a relatively small number of times (since it only happens for path components containing the reserved string) and I think we can reasonably expect the JIT to take care of it, since it's really only checking conditions that will never change for the lifetime of the process. You might also consider moving the check that the replacement string doesn't contain the path separator to NN startup time, since you can reasonably check the validity of that config setting without there being any reserved names in the namespace. You might consider making FSNamesystem#renameSnapshotPath static so that you don't have to pass a reference to the FSNamesystem into the FSImageFormat methods. You should add some documentation about this setting to the snapshots docs.
          Hide
          Andrew Wang added a comment -

          Test failure is the new test I added, use the binary diff to run it locally.

          Show
          Andrew Wang added a comment - Test failure is the new test I added, use the binary diff to run it locally.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12623011/hdfs-5709-1.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestDFSUpgradeFromImage

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5875//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5875//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12623011/hdfs-5709-1.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDFSUpgradeFromImage +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5875//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5875//console This message is automatically generated.
          Hide
          Andrew Wang added a comment -

          Binary patch attached. High-level notes:

          • I went with a full-substitution for reserved path components with the new configuration option, with the understanding that ".user-snapshot" is a prettier format. By default, this is null and will make the user set it if we encounter a .snapshot path during upgrade.
          • Should be easy to extend if we ever have add more reserved paths. I went through and munged all the edit log ops except the cache directive ones; I think these are okay to skip since they won't prevent the NN from starting up, but I can do them too if desired.
          • It's hard to check that the destination of the rename is safe, so I don't. The issue is if we rename something to .user-snapshot and the edit log later touchz's or mkdirs .user-snapshot. We could check for .user-snapshot in all paths flying by, but then we'd be precluding that from being used anywhere in the namespace, not just where it might collide in later edit log ops.
          • New test and provided tarball have an image and edits that refer to ".snapshot" paths. I did a variety of touch and mkdir operations.
          Show
          Andrew Wang added a comment - Binary patch attached. High-level notes: I went with a full-substitution for reserved path components with the new configuration option, with the understanding that ".user-snapshot" is a prettier format. By default, this is null and will make the user set it if we encounter a .snapshot path during upgrade. Should be easy to extend if we ever have add more reserved paths. I went through and munged all the edit log ops except the cache directive ones; I think these are okay to skip since they won't prevent the NN from starting up, but I can do them too if desired. It's hard to check that the destination of the rename is safe, so I don't. The issue is if we rename something to .user-snapshot and the edit log later touchz's or mkdirs .user-snapshot . We could check for .user-snapshot in all paths flying by, but then we'd be precluding that from being used anywhere in the namespace, not just where it might collide in later edit log ops. New test and provided tarball have an image and edits that refer to ".snapshot" paths. I did a variety of touch and mkdir operations.
          Hide
          Jing Zhao added a comment -

          a prefix over a full substitution

          I guess one advantage of using prefix is that we only need to set one prefix for all the reserved file names (like .snapshot and .inode, e.g.)? In case that we set full sub, we may need to have multiple configuration properties to support all the reserved names.

          Show
          Jing Zhao added a comment - a prefix over a full substitution I guess one advantage of using prefix is that we only need to set one prefix for all the reserved file names (like .snapshot and .inode, e.g.)? In case that we set full sub, we may need to have multiple configuration properties to support all the reserved names.
          Hide
          Andrew Wang added a comment -

          Cool, thanks Suresh and Jing. Glad we've converged on something palatable. Suresh, is there any reason you prefer a prefix over a full substitution? Full sub would be more flexible. I somewhat prefer the look of .myprefix-snapshot over myprefix-.snapshot too.

          There's also the question of what to do if the destination path already exists. I think the simplest thing to do is just to abort. My first thought was to try incrementing some value until there's no collision, e.g. .user-snapshot, .user-snapshot-1, .user-snapshot-2, but you could still collide with a future edit in the edit log. We could insert a UUID, but then edit log replay becomes non-deterministic across NNs. So, I think just abort.

          I think an interactive upgrade process can be a possible solution

          This is an interesting suggestion, but I'd prefer to make this as automatic as possible first if we can do it.

          Show
          Andrew Wang added a comment - Cool, thanks Suresh and Jing. Glad we've converged on something palatable. Suresh, is there any reason you prefer a prefix over a full substitution? Full sub would be more flexible. I somewhat prefer the look of .myprefix-snapshot over myprefix-.snapshot too. There's also the question of what to do if the destination path already exists. I think the simplest thing to do is just to abort. My first thought was to try incrementing some value until there's no collision, e.g. .user-snapshot, .user-snapshot-1, .user-snapshot-2, but you could still collide with a future edit in the edit log. We could insert a UUID, but then edit log replay becomes non-deterministic across NNs. So, I think just abort. I think an interactive upgrade process can be a possible solution This is an interesting suggestion, but I'd prefer to make this as automatic as possible first if we can do it.
          Hide
          Jing Zhao added a comment -

          One possible solution is to turn of snapshot feature post upgade and allow .snapshot directory or file. When a user tries to turn on .snapshot the namenode fails to come up, forcing them to turn off the feature, rename directories and turn it back on again. This may be a lot of work.

          To support this we need to add a switch into SnapshotManager and we need to check this switch every time resolving a .snapshot path. This may be a quick fix but will make the current code not clean.

          make it an active decision during upgrade

          I think an interactive upgrade process can be a possible solution, and this can be the default behavior if a file/directory with a reserved name is detected. In case that users want to disable the interactive process, a flag (like -force) can be provided and the name read from the configuration (like .user-snapshot) will be used for the rename. And all the rename should be recorded/logged.

          Show
          Jing Zhao added a comment - One possible solution is to turn of snapshot feature post upgade and allow .snapshot directory or file. When a user tries to turn on .snapshot the namenode fails to come up, forcing them to turn off the feature, rename directories and turn it back on again. This may be a lot of work. To support this we need to add a switch into SnapshotManager and we need to check this switch every time resolving a .snapshot path. This may be a quick fix but will make the current code not clean. make it an active decision during upgrade I think an interactive upgrade process can be a possible solution, and this can be the default behavior if a file/directory with a reserved name is detected. In case that users want to disable the interactive process, a flag (like -force) can be provided and the name read from the configuration (like .user-snapshot) will be used for the rename. And all the rename should be recorded/logged.
          Hide
          Suresh Srinivas added a comment -

          Maybe we could have a config option (by default unset) that will rename a reserved path on upgrade to a user-provided one. Something like dfs.namenode.upgrade.rename.reserved.snapshot=.user-snapshot. If we need more complex behavior, we could even do something with path globbing.

          I like this with some changes.

          The configuration could be dfs.namenode.upgrade.rename.reserved.prefix=<user-string>. We can fail the upgrade if this configuration is not present. During upgrade we can rename the reserved file names to user-string + reserved file name. The namenode can print this information in the log or user can search for those.

          Show
          Suresh Srinivas added a comment - Maybe we could have a config option (by default unset) that will rename a reserved path on upgrade to a user-provided one. Something like dfs.namenode.upgrade.rename.reserved.snapshot=.user-snapshot. If we need more complex behavior, we could even do something with path globbing. I like this with some changes. The configuration could be dfs.namenode.upgrade.rename.reserved.prefix=<user-string>. We can fail the upgrade if this configuration is not present. During upgrade we can rename the reserved file names to user-string + reserved file name. The namenode can print this information in the log or user can search for those.
          Hide
          Andrew Wang added a comment -

          These are good points. I agree that we shouldn't do a silent rename, and a mandatory pre-upgrade tool isn't a great option either. However, making the user ls -R and rename everything is still a pretty big hassle. After the rename, I think you'd also need to saveNamespace on both NNs to flush any references to .snapshot in the edit log.

          Maybe we could have a config option (by default unset) that will rename a reserved path on upgrade to a user-provided one. Something like dfs.namenode.upgrade.rename.reserved.snapshot=.user-snapshot. If we need more complex behavior, we could even do something with path globbing.

          This also might not be the last time we want to add a new reserved path, so it seems worth having a better upgrade story for the future.

          Show
          Andrew Wang added a comment - These are good points. I agree that we shouldn't do a silent rename, and a mandatory pre-upgrade tool isn't a great option either. However, making the user ls -R and rename everything is still a pretty big hassle. After the rename, I think you'd also need to saveNamespace on both NNs to flush any references to .snapshot in the edit log. Maybe we could have a config option (by default unset) that will rename a reserved path on upgrade to a user-provided one. Something like dfs.namenode.upgrade.rename.reserved.snapshot=.user-snapshot . If we need more complex behavior, we could even do something with path globbing. This also might not be the last time we want to add a new reserved path, so it seems worth having a better upgrade story for the future.
          Hide
          Suresh Srinivas added a comment -

          Hey Suresh, could you elaborate a bit on your -1?

          Sorry I should have added more details in my previous comment. Silently renaming it will cause issues to any applications that depend on the name .snapshot. I would rather make it an active decision during upgrade so that the admin can take care of renaming to appropriate name he chooses and also ensure applications that depend on such naming are updated. Otherwise upgrade will silently succeed followed by failures.

          I am assuming that .snapshot is used in very few clusters. In such a case cost of rollback and rename probably should not affect large number of clusters.

          As regards to tool to run for pre-upgrade, there are several issues:

          1. This tool can be available only in later releases (as we cannot change older releases already installed on the cluster).
          2. A user may not run the tool, even if it is made available on newer release and will have to rollback (as you pointed out)
          3. The pre-upgrade tool must consume fsimage and editlog (even if there is no .snapshot file/directory). This will extend the time for upgrade for every upgrade.

          One possible solution is to turn of snapshot feature post upgade and allow .snapshot directory or file. When a user tries to turn on .snapshot the namenode fails to come up, forcing them to turn off the feature, rename directories and turn it back on again. This may be a lot of work.

          Show
          Suresh Srinivas added a comment - Hey Suresh, could you elaborate a bit on your -1? Sorry I should have added more details in my previous comment. Silently renaming it will cause issues to any applications that depend on the name .snapshot. I would rather make it an active decision during upgrade so that the admin can take care of renaming to appropriate name he chooses and also ensure applications that depend on such naming are updated. Otherwise upgrade will silently succeed followed by failures. I am assuming that .snapshot is used in very few clusters. In such a case cost of rollback and rename probably should not affect large number of clusters. As regards to tool to run for pre-upgrade, there are several issues: This tool can be available only in later releases (as we cannot change older releases already installed on the cluster). A user may not run the tool, even if it is made available on newer release and will have to rollback (as you pointed out) The pre-upgrade tool must consume fsimage and editlog (even if there is no .snapshot file/directory). This will extend the time for upgrade for every upgrade. One possible solution is to turn of snapshot feature post upgade and allow .snapshot directory or file. When a user tries to turn on .snapshot the namenode fails to come up, forcing them to turn off the feature, rename directories and turn it back on again. This may be a lot of work.
          Hide
          Jing Zhao added a comment -

          For automatic renaming, at least we could not silently do the renaming, thus a separate tool for pre-upgrade makes sense to me.

          Show
          Jing Zhao added a comment - For automatic renaming, at least we could not silently do the renaming, thus a separate tool for pre-upgrade makes sense to me.
          Hide
          Andrew Wang added a comment -

          Hey Suresh, could you elaborate a bit on your -1? From my read of HDFS-4666, others seemed basically okay with the idea of automatic renaming. I'm considering it since telling the user at this late stage isn't very friendly. They'll have to rollback, swap all their jars, fix the .snapshot paths, then upgrade again. Also, unless we enumerate all the erroneous .snapshot paths, fixing all of them is somewhat error-prone.

          Another possibility is making this a separate tool that users can run pre-upgrade, but that still means possibly doing the rollback dance if someone doesn't read the release notes.

          Show
          Andrew Wang added a comment - Hey Suresh, could you elaborate a bit on your -1? From my read of HDFS-4666 , others seemed basically okay with the idea of automatic renaming. I'm considering it since telling the user at this late stage isn't very friendly. They'll have to rollback, swap all their jars, fix the .snapshot paths, then upgrade again. Also, unless we enumerate all the erroneous .snapshot paths, fixing all of them is somewhat error-prone. Another possibility is making this a separate tool that users can run pre-upgrade, but that still means possibly doing the rollback dance if someone doesn't read the release notes.
          Hide
          Suresh Srinivas added a comment -

          I am -1 on automatically renaming the directory.

          +1 for printing a more meaningful error message that says ".snapshot directory is reserved file name in the release you are upgrading to. Please rollback to the old release and rename the directories and make changes to the applications that rely on existence of such directories, followed by upgrade to this release."

          Show
          Suresh Srinivas added a comment - I am -1 on automatically renaming the directory. +1 for printing a more meaningful error message that says ".snapshot directory is reserved file name in the release you are upgrading to. Please rollback to the old release and rename the directories and make changes to the applications that rely on existence of such directories, followed by upgrade to this release."
          Hide
          Andrew Wang added a comment -

          Right now, I get these stack traces when trying to upgrade to trunk, with a "mkdir /.snapshot" edit, and then in the fsimage:

          pergroup:rwxr-xr-x, opCode=OP_MKDIR, txid=2]
          java.lang.ArrayIndexOutOfBoundsException: -1
              at org.apache.hadoop.hdfs.server.namenode.FSDirectory.addChild(FSDirectory.java:2193)
                  at org.apache.hadoop.hdfs.server.namenode.FSDirectory.unprotectedMkdir(FSDirectory.java:1982)
                  at org.apache.hadoop.hdfs.server.namenode.FSDirectory.unprotectedMkdir(FSDirectory.java:1967)
                  at org.apache.hadoop.hdfs.server.namenode.FSEditLogLoader.applyEditLogOp(FSEditLogLoader.java:458)
                  at org.apache.hadoop.hdfs.server.namenode.FSEditLogLoader.loadEditRecords(FSEditLogLoader.java:206)
                  at org.apache.hadoop.hdfs.server.namenode.FSEditLogLoader.loadFSEdits(FSEditLogLoader.java:119)
                  at org.apache.hadoop.hdfs.server.namenode.FSImage.loadEdits(FSImage.java:730)
                  at org.apache.hadoop.hdfs.server.namenode.FSImage.loadFSImage(FSImage.java:644)
                  at org.apache.hadoop.hdfs.server.namenode.FSImage.doUpgrade(FSImage.java:329)
                  at org.apache.hadoop.hdfs.server.namenode.FSImage.recoverTransitionRead(FSImage.java:248)
                  at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.loadFSImage(FSNamesystem.java:845)
                  at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.loadFromDisk(FSNamesystem.java:614)
                  at org.apache.hadoop.hdfs.server.namenode.NameNode.loadNamesystem(NameNode.java:444)
                  at org.apache.hadoop.hdfs.server.namenode.NameNode.initialize(NameNode.java:492)
                  at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:648)
                  at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:633)
                  at org.apache.hadoop.hdfs.server.namenode.NameNode.createNameNode(NameNode.java:1239)
                  at org.apache.hadoop.hdfs.server.namenode.NameNode.main(NameNode.java:1305)
          
          13/12/31 15:12:32 ERROR namenode.FSImage: Failed to load image from FSImageFile(file=/tmp/hadoop-andrew/dfs/name/current/fsimage_0000000000000000008, cpktTxId=0000000000000000008)
              java.io.FileNotFoundException: Directory does not exist: /.snapshot
                  at org.apache.hadoop.hdfs.server.namenode.INodeDirectory.valueOf(INodeDirectory.java:55)
                  at org.apache.hadoop.hdfs.server.namenode.FSImageFormat$Loader.loadDirectory(FSImageFormat.java:520)
                  at org.apache.hadoop.hdfs.server.namenode.FSImageFormat$Loader.loadLocalNameINodes(FSImageFormat.java:424)
                  at org.apache.hadoop.hdfs.server.namenode.FSImageFormat$Loader.load(FSImageFormat.java:340)
                  at org.apache.hadoop.hdfs.server.namenode.FSImage.loadFSImage(FSImage.java:824)
                  at org.apache.hadoop.hdfs.server.namenode.FSImage.loadFSImage(FSImage.java:813)
                  at org.apache.hadoop.hdfs.server.namenode.FSImage.loadFSImageFile(FSImage.java:661)
                  at org.apache.hadoop.hdfs.server.namenode.FSImage.loadFSImage(FSImage.java:630)
                  at org.apache.hadoop.hdfs.server.namenode.FSImage.doUpgrade(FSImage.java:329)
                  at org.apache.hadoop.hdfs.server.namenode.FSImage.recoverTransitionRead(FSImage.java:248)
                  at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.loadFSImage(FSNamesystem.java:845)
                  at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.loadFromDisk(FSNamesystem.java:614)
                  at org.apache.hadoop.hdfs.server.namenode.NameNode.loadNamesystem(NameNode.java:444)
                  at org.apache.hadoop.hdfs.server.namenode.NameNode.initialize(NameNode.java:492)
                  at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:648)
                  at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:633)
                  at org.apache.hadoop.hdfs.server.namenode.NameNode.createNameNode(NameNode.java:1239)
                  at org.apache.hadoop.hdfs.server.namenode.NameNode.main(NameNode.java:1305)
          
          Show
          Andrew Wang added a comment - Right now, I get these stack traces when trying to upgrade to trunk, with a "mkdir /.snapshot" edit, and then in the fsimage: pergroup:rwxr-xr-x, opCode=OP_MKDIR, txid=2] java.lang.ArrayIndexOutOfBoundsException: -1 at org.apache.hadoop.hdfs.server.namenode.FSDirectory.addChild(FSDirectory.java:2193) at org.apache.hadoop.hdfs.server.namenode.FSDirectory.unprotectedMkdir(FSDirectory.java:1982) at org.apache.hadoop.hdfs.server.namenode.FSDirectory.unprotectedMkdir(FSDirectory.java:1967) at org.apache.hadoop.hdfs.server.namenode.FSEditLogLoader.applyEditLogOp(FSEditLogLoader.java:458) at org.apache.hadoop.hdfs.server.namenode.FSEditLogLoader.loadEditRecords(FSEditLogLoader.java:206) at org.apache.hadoop.hdfs.server.namenode.FSEditLogLoader.loadFSEdits(FSEditLogLoader.java:119) at org.apache.hadoop.hdfs.server.namenode.FSImage.loadEdits(FSImage.java:730) at org.apache.hadoop.hdfs.server.namenode.FSImage.loadFSImage(FSImage.java:644) at org.apache.hadoop.hdfs.server.namenode.FSImage.doUpgrade(FSImage.java:329) at org.apache.hadoop.hdfs.server.namenode.FSImage.recoverTransitionRead(FSImage.java:248) at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.loadFSImage(FSNamesystem.java:845) at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.loadFromDisk(FSNamesystem.java:614) at org.apache.hadoop.hdfs.server.namenode.NameNode.loadNamesystem(NameNode.java:444) at org.apache.hadoop.hdfs.server.namenode.NameNode.initialize(NameNode.java:492) at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:648) at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:633) at org.apache.hadoop.hdfs.server.namenode.NameNode.createNameNode(NameNode.java:1239) at org.apache.hadoop.hdfs.server.namenode.NameNode.main(NameNode.java:1305) 13/12/31 15:12:32 ERROR namenode.FSImage: Failed to load image from FSImageFile(file=/tmp/hadoop-andrew/dfs/name/current/fsimage_0000000000000000008, cpktTxId=0000000000000000008) java.io.FileNotFoundException: Directory does not exist: /.snapshot at org.apache.hadoop.hdfs.server.namenode.INodeDirectory.valueOf(INodeDirectory.java:55) at org.apache.hadoop.hdfs.server.namenode.FSImageFormat$Loader.loadDirectory(FSImageFormat.java:520) at org.apache.hadoop.hdfs.server.namenode.FSImageFormat$Loader.loadLocalNameINodes(FSImageFormat.java:424) at org.apache.hadoop.hdfs.server.namenode.FSImageFormat$Loader.load(FSImageFormat.java:340) at org.apache.hadoop.hdfs.server.namenode.FSImage.loadFSImage(FSImage.java:824) at org.apache.hadoop.hdfs.server.namenode.FSImage.loadFSImage(FSImage.java:813) at org.apache.hadoop.hdfs.server.namenode.FSImage.loadFSImageFile(FSImage.java:661) at org.apache.hadoop.hdfs.server.namenode.FSImage.loadFSImage(FSImage.java:630) at org.apache.hadoop.hdfs.server.namenode.FSImage.doUpgrade(FSImage.java:329) at org.apache.hadoop.hdfs.server.namenode.FSImage.recoverTransitionRead(FSImage.java:248) at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.loadFSImage(FSNamesystem.java:845) at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.loadFromDisk(FSNamesystem.java:614) at org.apache.hadoop.hdfs.server.namenode.NameNode.loadNamesystem(NameNode.java:444) at org.apache.hadoop.hdfs.server.namenode.NameNode.initialize(NameNode.java:492) at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:648) at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:633) at org.apache.hadoop.hdfs.server.namenode.NameNode.createNameNode(NameNode.java:1239) at org.apache.hadoop.hdfs.server.namenode.NameNode.main(NameNode.java:1305)

            People

            • Assignee:
              Andrew Wang
              Reporter:
              Andrew Wang
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development