Hadoop Common
  1. Hadoop Common
  2. HADOOP-4618

Move http server from FSNamesystem into NameNode.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.1.0
    • Fix Version/s: 0.20.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      Moved HTTP server from FSNameSystem to NameNode. Removed FSNamesystem.getNameNodeInfoPort(). Replaced FSNamesystem.getDFSNameNodeMachine() and FSNamesystem.getDFSNameNodePort() with new method FSNamesystem.getDFSNameNodeAddress(). Removed constructor NameNode(bindAddress, conf).
      Show
      Moved HTTP server from FSNameSystem to NameNode. Removed FSNamesystem.getNameNodeInfoPort(). Replaced FSNamesystem.getDFSNameNodeMachine() and FSNamesystem.getDFSNameNodePort() with new method FSNamesystem.getDFSNameNodeAddress(). Removed constructor NameNode(bindAddress, conf).

      Description

      NameNode is responsible now for starting its RPC server. The (web UI) http server is started inside FSNamesystem class.
      I think it should be the NameNode's responsibility to deal with all issues intended for exposing information it holds to the outside world. This should include the RCP server as well as the http server.
      And FSNamesystem class should be a logical container of internal namespace data-structures and in general should not be accessed directly from the outside.

      1. MoveHttpServer.patch
        24 kB
        Konstantin Shvachko
      2. MoveHttpServer.patch
        24 kB
        Konstantin Shvachko
      3. MoveHttpServer.patch
        24 kB
        Konstantin Shvachko

        Activity

        Hide
        Robert Chansler added a comment -

        Edit release note for publication.

        Show
        Robert Chansler added a comment - Edit release note for publication.
        Hide
        steve_l added a comment -

        OK, My code is up in sync with this; I even have a test for MiniDFSCluster that gets the port number and asserts that it is open

        Show
        steve_l added a comment - OK, My code is up in sync with this; I even have a test for MiniDFSCluster that gets the port number and asserts that it is open
        Hide
        Konstantin Shvachko added a comment -

        I just committed this.

        Show
        Konstantin Shvachko added a comment - I 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/12393663/MoveHttpServer.patch
        against trunk revision 712729.

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +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 warnings.

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3571/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3571/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3571/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3571/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/12393663/MoveHttpServer.patch against trunk revision 712729. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +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 warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3571/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3571/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3571/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3571/console This message is automatically generated.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        +1 new patch looks good.

        Show
        Tsz Wo Nicholas Sze added a comment - +1 new patch looks good.
        Hide
        Konstantin Shvachko added a comment -

        Thanks for the comments.

        • Good point Steve, I will mark this as an incompatible change. The right way would have been to deprecate getNameNodeInfoPort(), but there is no way to implement the method in FSNamesystem as the http server is not there anymore.
        • Nicholas, I agree I should have left the "final" section in place.
        • JavaDoc warning is related to HADOOP-4621.

        In this version I restored the final section and deprecated FSNamesystem.getDFSNameNodeAddress() in order to be able to remove it later.

        Show
        Konstantin Shvachko added a comment - Thanks for the comments. Good point Steve, I will mark this as an incompatible change. The right way would have been to deprecate getNameNodeInfoPort(), but there is no way to implement the method in FSNamesystem as the http server is not there anymore. Nicholas, I agree I should have left the "final" section in place. JavaDoc warning is related to HADOOP-4621 . In this version I restored the final section and deprecated FSNamesystem.getDFSNameNodeAddress() in order to be able to remove it later.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        In FSNamesystem.close(), it is better not to take dir.close() out from finally{}. If pendingReplications.stop() throws a RuntimeException, e.g. NPE, then dir won't be closed and, as a result, the image won't be closed.

        Show
        Tsz Wo Nicholas Sze added a comment - In FSNamesystem.close(), it is better not to take dir.close() out from finally{}. If pendingReplications.stop() throws a RuntimeException, e.g. NPE, then dir won't be closed and, as a result, the image won't be closed.
        Hide
        steve_l added a comment -

        Even though this will be tricky for me to merge in with my lifecycle stuff, I think it is good, hence the : +1

        HADOOP-4383 would benefit from it, as it would not need to rummage in the namesystem to get the relevant web port.

        FSNamesystem.getNameNodeInfoPort() is public -if anyone is using it and its sibling methods their app will break. And as NameNode.fsnamesystem is public, this is effectively a bit of a public API that is being broken. I know my code will break, and I'm happy with that, but I don't know who else is using it.

        Show
        steve_l added a comment - Even though this will be tricky for me to merge in with my lifecycle stuff, I think it is good, hence the : +1 HADOOP-4383 would benefit from it, as it would not need to rummage in the namesystem to get the relevant web port. FSNamesystem.getNameNodeInfoPort() is public -if anyone is using it and its sibling methods their app will break. And as NameNode.fsnamesystem is public, this is effectively a bit of a public API that is being broken. I know my code will break, and I'm happy with that, but I don't know who else is using it.
        Hide
        Hadoop QA added a comment -

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

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        -1 javadoc. The javadoc tool appears to have generated 1 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 warnings.

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3558/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3558/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3558/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3558/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/12393549/MoveHttpServer.patch against trunk revision 712344. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. -1 javadoc. The javadoc tool appears to have generated 1 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 warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3558/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3558/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3558/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3558/console This message is automatically generated.
        Hide
        Konstantin Shvachko added a comment -

        Patch synchronized with trunk.

        Show
        Konstantin Shvachko added a comment - Patch synchronized with trunk.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        The patch does not apply to trunk.

        Show
        Tsz Wo Nicholas Sze added a comment - The patch does not apply to trunk.
        Hide
        Konstantin Shvachko added a comment -

        One more change is that I moved field supportAppends from NameNode to FSNamesystem where all such fields are used to be.

        Show
        Konstantin Shvachko added a comment - One more change is that I moved field supportAppends from NameNode to FSNamesystem where all such fields are used to be.
        Hide
        Konstantin Shvachko added a comment -

        This patch moves the http server to the NameNode class.
        Additionally I removed one redundant NN constructor and redundant fields handlerCount, startTime.
        I changed dfshealth.jsp and nn_browsedfscontent.jsp to not use getFSNamesystem() method.
        I could not get rid of the namenodeAddress field in FSNamesystem because it would require changing JSPHelper constructor, which is used in too many if not all other jsps.

        Show
        Konstantin Shvachko added a comment - This patch moves the http server to the NameNode class. Additionally I removed one redundant NN constructor and redundant fields handlerCount , startTime . I changed dfshealth.jsp and nn_browsedfscontent.jsp to not use getFSNamesystem() method. I could not get rid of the namenodeAddress field in FSNamesystem because it would require changing JSPHelper constructor, which is used in too many if not all other jsps.
        Hide
        Konstantin Shvachko added a comment -

        I came to this conclusion trying to implement a standby node. SNN and NN should be represented by the same class, since one can turn into another and vice versa during their life time. I realized that with current implementation the node startup logic will be spread between two classes (NameNode and FSNamesystem), which is inconvenient, complex, and error prone.

        One of the consequences of the current design is that we expose FSNamesystem to jsp-s and servlets through a static public method getFSNamesystem(), which returns this. This seems to be an unnecessary shortcut, because all jsp-s and servlets have a reference to the NameNode instance and can access FSNamesystem data via name-node.
        This also creates redundant fields in FSNamesystem like name-node's host, port and infoPort.

        Show
        Konstantin Shvachko added a comment - I came to this conclusion trying to implement a standby node. SNN and NN should be represented by the same class, since one can turn into another and vice versa during their life time. I realized that with current implementation the node startup logic will be spread between two classes ( NameNode and FSNamesystem ), which is inconvenient, complex, and error prone. One of the consequences of the current design is that we expose FSNamesystem to jsp-s and servlets through a static public method getFSNamesystem() , which returns this . This seems to be an unnecessary shortcut, because all jsp-s and servlets have a reference to the NameNode instance and can access FSNamesystem data via name-node. This also creates redundant fields in FSNamesystem like name-node's host, port and infoPort.

          People

          • Assignee:
            Konstantin Shvachko
            Reporter:
            Konstantin Shvachko
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development