Details

    • Type: Test Test
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.23.0
    • Fix Version/s: 0.23.0
    • Component/s: test
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Though running multiple 2NNs isn't supported, accidentally doing so should not result in HDFS metadata corruptions. We should add a test case to exercise this possibility when name.dir.storage.restore is enabled, which is a particularly delicate code path.

      1. hdfs-2100.1.patch
        8 kB
        Aaron T. Myers
      2. hdfs-2100.0.patch
        8 kB
        Aaron T. Myers

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #703 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/703/)
        HDFS-2100. Improve TestStorageRestore. Contributed by Aaron T. Myers.

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1138262
        Files :

        • /hadoop/common/trunk/hdfs/CHANGES.txt
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStorageRestore.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #703 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/703/ ) HDFS-2100 . Improve TestStorageRestore. Contributed by Aaron T. Myers. atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1138262 Files : /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStorageRestore.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #753 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/753/)
        HDFS-2100. Improve TestStorageRestore. Contributed by Aaron T. Myers.

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1138262
        Files :

        • /hadoop/common/trunk/hdfs/CHANGES.txt
        • /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStorageRestore.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #753 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/753/ ) HDFS-2100 . Improve TestStorageRestore. Contributed by Aaron T. Myers. atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1138262 Files : /hadoop/common/trunk/hdfs/CHANGES.txt /hadoop/common/trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStorageRestore.java
        Hide
        Aaron T. Myers added a comment -

        Thanks a lot for the review, Todd. I've just committed this.

        Show
        Aaron T. Myers added a comment - Thanks a lot for the review, Todd. I've just committed this.
        Hide
        Hadoop QA added a comment -

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

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

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

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

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

        +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 passed core unit tests.

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

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/812//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/812//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/812//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/12483367/hdfs-2100.1.patch against trunk revision 1138098. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/812//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/812//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/812//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        +1 pending hudson result

        Show
        Todd Lipcon added a comment - +1 pending hudson result
        Hide
        Aaron T. Myers added a comment -

        Updated patch addressing Todd's comments.

        Seems like you got distracted halfway through writing this comment:
        // The created file should still exist in the

        Quite right. Changed to:

        // The created file should still exist after the restart.
        

        You might consider using ImmutableSet.of(path1) and ImmutableSet.of(path2, path3) from guava - it's a bit easier to read IMO but no biggy

        That's a great tip. I hated writing that code just to create a briefly-lived set.

        Can you add a javadoc to explain what bug this is testing for and what the simulation is? Seems funny that the test case is called testMultipleSecondasryCheckpoint but it only instantiates one 2NN.

        Added the following:

          /**
           * Test to simulate interleaved checkpointing by 2 2NNs after a storage
           * directory has been taken offline. The first will cause the directory to
           * come back online, but it won't have any valid contents. The second 2NN will
           * then try to perform a checkpoint. The NN should not serve up the image or
           * edits from the restored (empty) dir.
           */
        
        Show
        Aaron T. Myers added a comment - Updated patch addressing Todd's comments. Seems like you got distracted halfway through writing this comment: // The created file should still exist in the Quite right. Changed to: // The created file should still exist after the restart. You might consider using ImmutableSet.of(path1) and ImmutableSet.of(path2, path3) from guava - it's a bit easier to read IMO but no biggy That's a great tip. I hated writing that code just to create a briefly-lived set. Can you add a javadoc to explain what bug this is testing for and what the simulation is? Seems funny that the test case is called testMultipleSecondasryCheckpoint but it only instantiates one 2NN. Added the following: /** * Test to simulate interleaved checkpointing by 2 2NNs after a storage * directory has been taken offline. The first will cause the directory to * come back online, but it won't have any valid contents. The second 2NN will * then try to perform a checkpoint. The NN should not serve up the image or * edits from the restored (empty) dir. */
        Hide
        Todd Lipcon added a comment -

        Looks pretty good, nice test. A few comments:

        • Seems like you got distracted halfway through writing this comment:
          // The created file should still exist in the
          
        • You might consider using ImmutableSet.of(path1) and ImmutableSet.of(path2, path3) from guava - it's a bit easier to read IMO but no biggy
        • Can you add a javadoc to explain what bug this is testing for and what the simulation is? Seems funny that the test case is called testMultipleSecondasryCheckpoint but it only instantiates one 2NN.
        Show
        Todd Lipcon added a comment - Looks pretty good, nice test. A few comments: Seems like you got distracted halfway through writing this comment: // The created file should still exist in the You might consider using ImmutableSet.of(path1) and ImmutableSet.of(path2, path3) from guava - it's a bit easier to read IMO but no biggy Can you add a javadoc to explain what bug this is testing for and what the simulation is? Seems funny that the test case is called testMultipleSecondasryCheckpoint but it only instantiates one 2NN.
        Hide
        Aaron T. Myers added a comment -

        Patch which adds a new test method to cover this case.

        This patch also cleans up TestStorageRestore generally by making it not extend TestCase, getting rid of a few compiler warnings, and fixing a typo.

        Show
        Aaron T. Myers added a comment - Patch which adds a new test method to cover this case. This patch also cleans up TestStorageRestore generally by making it not extend TestCase , getting rid of a few compiler warnings, and fixing a typo.

          People

          • Assignee:
            Aaron T. Myers
            Reporter:
            Aaron T. Myers
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development