Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-9519

Some coding improvement in SecondaryNameNode#main

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Two nits:

      1. The checking whether secondary is null is not necessary in the following code in SecondaryNameNode.java.
      2. The comment in this code seems to imply that "when secondary is not null, SNN was stared as a daemon.", and this is not true. Suggest to improve the comment to make it clear,

      Assign to Xiao since he worked on HDFS-3059. Thanks Xiao.

            if (secondary != null) {
              // The web server is only needed when starting SNN as a daemon,
              // and not needed if called from shell command. Starting the web server
              // from shell may fail when getting credentials, if the environment
              // is not set up for it, which is most of the case.
              secondary.startInfoServer();
      
              secondary.startCheckpointThread();
              secondary.join();
            }
      
      1. HDFS-9519.01.patch
        2 kB
        Xiao Chen
      2. HDFS-9519.02.patch
        1 kB
        Xiao Chen

        Issue Links

          Activity

          Hide
          xiaochen Xiao Chen added a comment -

          Thanks for reporting this Yongjun, I'll work on this soon.

          Show
          xiaochen Xiao Chen added a comment - Thanks for reporting this Yongjun, I'll work on this soon.
          Hide
          liuml07 Mingliang Liu added a comment -

          Thanks for reporting this, Yongjun Zhang. The condition check is always true and the comment is confusing.

          Show
          liuml07 Mingliang Liu added a comment - Thanks for reporting this, Yongjun Zhang . The condition check is always true and the comment is confusing.
          Hide
          aw Allen Wittenauer added a comment -

          If the secondary namenode isn't actually configured but someone tries to start the 2nn, what happens? Also, does Checkpoint and Backup have different entry points or is this used for those too?

          Show
          aw Allen Wittenauer added a comment - If the secondary namenode isn't actually configured but someone tries to start the 2nn, what happens? Also, does Checkpoint and Backup have different entry points or is this used for those too?
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Mingliang Liu and Allen Wittenauer for the comments and sorry for my delayed response.

          The if (secondary != null) is confusing, and should be removed with the change of HDFS-6348. My bad I didn't catch this redundancy when fixing HDFS-3059.

          The reason of disabling the http server when starting with CommandLineOpts is that, after processing it the 2NN will terminate. E.g. when someone runs hdfs secondarynamenode -checkpoint the 2nn just start and checkpoint, then exit. Starting an http server during this is unnecessary - the 2nn webui is for the daemon anyways. Also, when security is enabled, admin may not have the credentials to the web server, but they should be allowed to do checkpointing.

          My comments in the code seems to be doing more confusion than explanation...... I revised it and attached in patch 1 for review.

          re. Allen Wittenauer's questions:

          If the secondary namenode isn't actually configured but someone tries to start the 2nn, what happens?

          In this case 2NN is started as a daemon. My comments were bad.

          Also, does Checkpoint and Backup have different entry points or is this used for those too?

          AFAICT, 2NN only supports -checkpoint, -format and -geteditsize, with the same entry point. Backup should be done from the NN.

          Show
          xiaochen Xiao Chen added a comment - Thanks Mingliang Liu and Allen Wittenauer for the comments and sorry for my delayed response. The if (secondary != null) is confusing, and should be removed with the change of HDFS-6348 . My bad I didn't catch this redundancy when fixing HDFS-3059 . The reason of disabling the http server when starting with CommandLineOpts is that, after processing it the 2NN will terminate. E.g. when someone runs hdfs secondarynamenode -checkpoint the 2nn just start and checkpoint, then exit. Starting an http server during this is unnecessary - the 2nn webui is for the daemon anyways. Also, when security is enabled, admin may not have the credentials to the web server, but they should be allowed to do checkpointing. My comments in the code seems to be doing more confusion than explanation...... I revised it and attached in patch 1 for review. re. Allen Wittenauer 's questions: If the secondary namenode isn't actually configured but someone tries to start the 2nn, what happens? In this case 2NN is started as a daemon. My comments were bad. Also, does Checkpoint and Backup have different entry points or is this used for those too? AFAICT, 2NN only supports -checkpoint , -format and -geteditsize , with the same entry point. Backup should be done from the NN.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Xiao Chen for the patch.

          Can we move the main comment to the beginning of the block, something like:

                // SecondaryNameNode can be started to run a command (i.e. checkpoint or
                // geteditsize etc) and terminate, or to run as a daemon when no command is 
                // specified. The web server is only needed when 2NN runs as a daemon.
                if (opts != null && opts.getCommand() != null) {
                  // 2NN is started to run a command and then terminate
                  int ret = secondary.processStartupCommand(opts);
                  terminate(ret);
                }
                
                // 2NN runs as a daemon, start the web server
                secondary.startInfoServer();
          

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Xiao Chen for the patch. Can we move the main comment to the beginning of the block, something like: // SecondaryNameNode can be started to run a command (i.e. checkpoint or // geteditsize etc) and terminate, or to run as a daemon when no command is // specified. The web server is only needed when 2NN runs as a daemon. if (opts != null && opts.getCommand() != null ) { // 2NN is started to run a command and then terminate int ret = secondary.processStartupCommand(opts); terminate(ret); } // 2NN runs as a daemon, start the web server secondary.startInfoServer(); Thanks.
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Yongjun. I agree with your general idea that we should focus on the 2 modes instead of the web server itself. Quick clarification:
          'Run as a daemon' is not limited to 'no command is specified' - if -format is given, after parseArgs, opts.getCommand() still returns null so that 2NN is still started as a daemon.
          Uploaded patch 2 which I think explains it clearer. Please let me know what you think.

          Show
          xiaochen Xiao Chen added a comment - Thanks Yongjun. I agree with your general idea that we should focus on the 2 modes instead of the web server itself. Quick clarification: 'Run as a daemon' is not limited to 'no command is specified' - if -format is given, after parseArgs , opts.getCommand() still returns null so that 2NN is still started as a daemon. Uploaded patch 2 which I think explains it clearer. Please let me know what you think.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks Xiao. +1 on rev2 pending jenkins test.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks Xiao. +1 on rev2 pending jenkins test.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 8m 30s trunk passed
          +1 compile 0m 43s trunk passed with JDK v1.8.0_66
          +1 compile 0m 45s trunk passed with JDK v1.7.0_91
          +1 checkstyle 0m 16s trunk passed
          +1 mvnsite 0m 56s trunk passed
          +1 mvneclipse 0m 15s trunk passed
          +1 findbugs 2m 1s trunk passed
          +1 javadoc 1m 11s trunk passed with JDK v1.8.0_66
          +1 javadoc 1m 53s trunk passed with JDK v1.7.0_91
          +1 mvninstall 0m 53s the patch passed
          +1 compile 0m 42s the patch passed with JDK v1.8.0_66
          +1 javac 0m 42s the patch passed
          +1 compile 0m 44s the patch passed with JDK v1.7.0_91
          +1 javac 0m 44s the patch passed
          +1 checkstyle 0m 17s the patch passed
          +1 mvnsite 0m 55s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 0s Patch has no whitespace issues.
          +1 findbugs 2m 7s the patch passed
          +1 javadoc 1m 8s the patch passed with JDK v1.8.0_66
          +1 javadoc 1m 52s the patch passed with JDK v1.7.0_91
          +1 unit 67m 19s hadoop-hdfs in the patch passed with JDK v1.8.0_66.
          -1 unit 80m 0s hadoop-hdfs in the patch failed with JDK v1.7.0_91.
          -1 asflicense 0m 22s Patch generated 58 ASF License warnings.
          175m 59s



          Reason Tests
          JDK v1.7.0_91 Failed junit tests hadoop.hdfs.server.namenode.TestAddBlockRetry
            hadoop.hdfs.server.namenode.ha.TestDFSUpgradeWithHA
            hadoop.hdfs.TestDFSStripedOutputStream
            hadoop.hdfs.server.namenode.snapshot.TestOpenFilesWithSnapshot
            hadoop.hdfs.server.namenode.ha.TestRequestHedgingProxyProvider
          JDK v1.7.0_91 Timed out junit tests org.apache.hadoop.hdfs.TestReplication



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:0ca8df7
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12776718/HDFS-9519.02.patch
          JIRA Issue HDFS-9519
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c49466f59b94 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 132478e
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/13822/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt
          unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/13822/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt
          JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/13822/testReport/
          asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/13822/artifact/patchprocess/patch-asflicense-problems.txt
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Max memory used 76MB
          Powered by Apache Yetus http://yetus.apache.org
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/13822/console

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 8m 30s trunk passed +1 compile 0m 43s trunk passed with JDK v1.8.0_66 +1 compile 0m 45s trunk passed with JDK v1.7.0_91 +1 checkstyle 0m 16s trunk passed +1 mvnsite 0m 56s trunk passed +1 mvneclipse 0m 15s trunk passed +1 findbugs 2m 1s trunk passed +1 javadoc 1m 11s trunk passed with JDK v1.8.0_66 +1 javadoc 1m 53s trunk passed with JDK v1.7.0_91 +1 mvninstall 0m 53s the patch passed +1 compile 0m 42s the patch passed with JDK v1.8.0_66 +1 javac 0m 42s the patch passed +1 compile 0m 44s the patch passed with JDK v1.7.0_91 +1 javac 0m 44s the patch passed +1 checkstyle 0m 17s the patch passed +1 mvnsite 0m 55s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s Patch has no whitespace issues. +1 findbugs 2m 7s the patch passed +1 javadoc 1m 8s the patch passed with JDK v1.8.0_66 +1 javadoc 1m 52s the patch passed with JDK v1.7.0_91 +1 unit 67m 19s hadoop-hdfs in the patch passed with JDK v1.8.0_66. -1 unit 80m 0s hadoop-hdfs in the patch failed with JDK v1.7.0_91. -1 asflicense 0m 22s Patch generated 58 ASF License warnings. 175m 59s Reason Tests JDK v1.7.0_91 Failed junit tests hadoop.hdfs.server.namenode.TestAddBlockRetry   hadoop.hdfs.server.namenode.ha.TestDFSUpgradeWithHA   hadoop.hdfs.TestDFSStripedOutputStream   hadoop.hdfs.server.namenode.snapshot.TestOpenFilesWithSnapshot   hadoop.hdfs.server.namenode.ha.TestRequestHedgingProxyProvider JDK v1.7.0_91 Timed out junit tests org.apache.hadoop.hdfs.TestReplication Subsystem Report/Notes Docker Image:yetus/hadoop:0ca8df7 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12776718/HDFS-9519.02.patch JIRA Issue HDFS-9519 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c49466f59b94 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 132478e findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/13822/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt unit test logs https://builds.apache.org/job/PreCommit-HDFS-Build/13822/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs-jdk1.7.0_91.txt JDK v1.7.0_91 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/13822/testReport/ asflicense https://builds.apache.org/job/PreCommit-HDFS-Build/13822/artifact/patchprocess/patch-asflicense-problems.txt modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Max memory used 76MB Powered by Apache Yetus http://yetus.apache.org Console output https://builds.apache.org/job/PreCommit-HDFS-Build/13822/console This message was automatically generated.
          Hide
          xiaochen Xiao Chen added a comment -

          The failed tests license warnings are not related.

          Show
          xiaochen Xiao Chen added a comment - The failed tests license warnings are not related.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #8960 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8960/)
          HDFS-9519. Some coding improvement in SecondaryNameNode#main. (Xiao Chen (yzhang: rev 2a4c7d4facabb8b99d6dcbf4ccfe2afedf4fd445)

          • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/SecondaryNameNode.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #8960 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8960/ ) HDFS-9519 . Some coding improvement in SecondaryNameNode#main. (Xiao Chen (yzhang: rev 2a4c7d4facabb8b99d6dcbf4ccfe2afedf4fd445) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/SecondaryNameNode.java
          Hide
          yzhangal Yongjun Zhang added a comment -

          Committed to trunk, branch-2 and branch-2.8. Thanks folks for review/comments, and Xiao for the contribution.

          Show
          yzhangal Yongjun Zhang added a comment - Committed to trunk, branch-2 and branch-2.8. Thanks folks for review/comments, and Xiao for the contribution.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #690 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/690/)
          HDFS-9519. Some coding improvement in SecondaryNameNode#main. (Xiao Chen (yzhang: rev 2a4c7d4facabb8b99d6dcbf4ccfe2afedf4fd445)

          • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/SecondaryNameNode.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #690 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/690/ ) HDFS-9519 . Some coding improvement in SecondaryNameNode#main. (Xiao Chen (yzhang: rev 2a4c7d4facabb8b99d6dcbf4ccfe2afedf4fd445) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/SecondaryNameNode.java
          Hide
          xiaochen Xiao Chen added a comment -

          Thanks Yongjun for the review + commit, and thanks Mingliang and Allen for the comments!

          Show
          xiaochen Xiao Chen added a comment - Thanks Yongjun for the review + commit, and thanks Mingliang and Allen for the comments!

            People

            • Assignee:
              xiaochen Xiao Chen
              Reporter:
              yzhangal Yongjun Zhang
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development