Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-2030

Fix the usability of namenode upgrade command

    Details

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

      Description

      Fixing the Namenode upgrade option along the same line as Namenode format option.

      If clusterid is not given then clusterid will be automatically generated for the upgrade but if clusterid is given then it will be honored.

      1. HDFS-2030-3.patch
        10 kB
        Bharath Mundlapudi
      2. HDFS-2030-2.patch
        10 kB
        Bharath Mundlapudi
      3. HDFS-2030-1.patch
        8 kB
        Bharath Mundlapudi

        Activity

        Bharath Mundlapudi created issue -
        Hide
        Bharath Mundlapudi added a comment -

        Attaching the patch

        Show
        Bharath Mundlapudi added a comment - Attaching the patch
        Bharath Mundlapudi made changes -
        Field Original Value New Value
        Attachment HDFS-2030-1.patch [ 12481724 ]
        Hide
        Suresh Srinivas added a comment -
        1. TestFSImageUpgrade
          • is missing banner.
          • Should we just put this in one of the existing tests?
          • Why do you need null and empty check in the tests if you are testing for startsWith("CID")
          • Do we need a test to check block pool ID is set?
          • Instead of assertTrue, use assertEquals where possible.
          • Please split the tests into testUpgradeFrom204, testUpgradeFrom22 and testUpgradeCurrentRelease. Also this test does not really do upgrade. Should we rename the test method.
          • There are few lines repeated every where. You could move it into a separate method.
        2. FSImage.java
          • Is it better to to have two methods setBlockPoolID() and setClusterID()?
          • "Try to set the clusterid and blockpoolid for the upgrade." Why Try to?
          • It is better to add the description in comments of what is happening in one place instead of spread all over. You may also want to move startOpt == StartupOption.UPGRADE into trySetClusterIDAndBlockPoolID().
        Show
        Suresh Srinivas added a comment - TestFSImageUpgrade is missing banner. Should we just put this in one of the existing tests? Why do you need null and empty check in the tests if you are testing for startsWith("CID") Do we need a test to check block pool ID is set? Instead of assertTrue, use assertEquals where possible. Please split the tests into testUpgradeFrom204, testUpgradeFrom22 and testUpgradeCurrentRelease. Also this test does not really do upgrade. Should we rename the test method. There are few lines repeated every where. You could move it into a separate method. FSImage.java Is it better to to have two methods setBlockPoolID() and setClusterID()? "Try to set the clusterid and blockpoolid for the upgrade." Why Try to? It is better to add the description in comments of what is happening in one place instead of spread all over. You may also want to move startOpt == StartupOption.UPGRADE into trySetClusterIDAndBlockPoolID().
        Hide
        Bharath Mundlapudi added a comment -

        Thanks for the review, Suresh.
        My comments inline.

        1.1 Missing banner - done.
        1.2 This method is package protected, this unit test just test this function instead of using time consuming MiniDFSCluster.
        1.3 Removed the null and empty checks.
        1.4 BoolpoolID is autogenerated. Now i have modified the tests to not mock this.
        1.5 Added assertEquals where necessary
        1.6 Made multiple tests

        2.1 Since the setBlockPoolID() and setClusterID() are in NNStorage, i moved this function to this class now solves this problem.
        2.2 renamed the function
        2.3 comments moved outside the function and moved the if condition inside the method.

        Attaching the patch with these changes.

        Show
        Bharath Mundlapudi added a comment - Thanks for the review, Suresh. My comments inline. 1.1 Missing banner - done. 1.2 This method is package protected, this unit test just test this function instead of using time consuming MiniDFSCluster. 1.3 Removed the null and empty checks. 1.4 BoolpoolID is autogenerated. Now i have modified the tests to not mock this. 1.5 Added assertEquals where necessary 1.6 Made multiple tests 2.1 Since the setBlockPoolID() and setClusterID() are in NNStorage, i moved this function to this class now solves this problem. 2.2 renamed the function 2.3 comments moved outside the function and moved the if condition inside the method. Attaching the patch with these changes.
        Hide
        Bharath Mundlapudi added a comment -

        Attached the patch.

        Show
        Bharath Mundlapudi added a comment - Attached the patch.
        Bharath Mundlapudi made changes -
        Attachment HDFS-2030-2.patch [ 12481979 ]
        Hide
        Bharath Mundlapudi added a comment -

        Done some more minor cleanup related to comments and adding more description to test class.

        Please find the attached patch.

        Show
        Bharath Mundlapudi added a comment - Done some more minor cleanup related to comments and adding more description to test class. Please find the attached patch.
        Bharath Mundlapudi made changes -
        Attachment HDFS-2030-3.patch [ 12481981 ]
        Hide
        Suresh Srinivas added a comment -

        +1 for the change

        Show
        Suresh Srinivas added a comment - +1 for the change
        Suresh Srinivas made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hadoop Flags [Reviewed]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12481981/HDFS-2030-3.patch
        against trunk revision 1134031.

        +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 appears to introduce 2 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 core unit tests:
        org.apache.hadoop.cli.TestHDFSCLI

        +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/756//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/756//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/756//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/12481981/HDFS-2030-3.patch against trunk revision 1134031. +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 appears to introduce 2 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 core unit tests: org.apache.hadoop.cli.TestHDFSCLI +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/756//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/756//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/756//console This message is automatically generated.
        Hide
        Suresh Srinivas added a comment -

        Findbugs warning and TestHDFSCLI is unrelated to this patch.

        Show
        Suresh Srinivas added a comment - Findbugs warning and TestHDFSCLI is unrelated to this patch.
        Hide
        Suresh Srinivas added a comment -

        I committed the patch. Thank you Bharath.

        Show
        Suresh Srinivas added a comment - I committed the patch. Thank you Bharath.
        Suresh Srinivas made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #746 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/746/)

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #746 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/746/ )
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #699 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/699/)

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #699 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/699/ )
        Arun C Murthy made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Bharath Mundlapudi
            Reporter:
            Bharath Mundlapudi
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development