Details

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

      Description

      JournalService class was added in HDFS-3099. Since it was not used in HDFS-3077, which has JournalNodeRpcServer instead, I propose deleting this dead code.

      1. HDFS-4904.patch
        22 kB
        Arpit Agarwal
      2. HDFS-4904.patch
        21 kB
        Arpit Agarwal

        Activity

        Hide
        arpitagarwal Arpit Agarwal added a comment -

        Not sure about this reference to journalservice in hadoop-hdfs-project/hadoop-hdfs/pom.xml. It was introduced by HDFS-3077.

                  <execution>
                    <id>journal</id>
                    <phase>generate-sources</phase>
                    <goals>
                      <goal>compile</goal>
                    </goals>
                    <configuration>
                      <compile>false</compile>
                      <workingDirectory>${project.build.directory}/generated-sources/java</workingDirectory>
                      <webFragmentFile>${project.build.directory}/journal-jsp-servlet-definitions.xml</webFragmentFile>
                      <packageName>org.apache.hadoop.hdfs.server.journalservice</packageName>
                      <sources>
                        <directory>${basedir}/src/main/webapps/journal</directory>
                        <includes>
                          <include>*.jsp</include>
                        </includes>
                      </sources>
        
        Show
        arpitagarwal Arpit Agarwal added a comment - Not sure about this reference to journalservice in hadoop-hdfs-project/hadoop-hdfs/pom.xml. It was introduced by HDFS-3077 . <execution> <id> journal </id> <phase> generate-sources </phase> <goals> <goal> compile </goal> </goals> <configuration> <compile> false </compile> <workingDirectory> ${project.build.directory}/generated-sources/java </workingDirectory> <webFragmentFile> ${project.build.directory}/journal-jsp-servlet-definitions.xml </webFragmentFile> <packageName> org.apache.hadoop.hdfs.server.journalservice </packageName> <sources> <directory> ${basedir}/src/main/webapps/journal </directory> <includes> <include> *.jsp </include> </includes> </sources>
        Hide
        hadoopqa Hadoop QA added a comment -

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

        +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 new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +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 passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

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

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

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12587769/HDFS-4904.patch against trunk revision . +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 new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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 passed unit tests in hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/4516//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/4516//console This message is automatically generated.
        Hide
        cnauroth Chris Nauroth added a comment -

        Not sure about this reference to journalservice in hadoop-hdfs-project/hadoop-hdfs/pom.xml.

        This is pre-compiling the JSP files under hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/journal into class files for the distro/deployment. Those JSP files don't appear to be doing anything, so I think the whole directory can be removed as a part of this change, and we can remove the <execution> from pom.xml.

        Show
        cnauroth Chris Nauroth added a comment - Not sure about this reference to journalservice in hadoop-hdfs-project/hadoop-hdfs/pom.xml. This is pre-compiling the JSP files under hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/journal into class files for the distro/deployment. Those JSP files don't appear to be doing anything, so I think the whole directory can be removed as a part of this change, and we can remove the <execution> from pom.xml.
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        Thanks for the confirmation Chris. Updated the patch.

        Show
        arpitagarwal Arpit Agarwal added a comment - Thanks for the confirmation Chris. Updated the patch.
        Hide
        hadoopqa Hadoop QA added a comment -

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

        +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 new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 javac. The patch appears to cause the build to fail.

        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/4519//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12587854/HDFS-4904.patch against trunk revision . +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 new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 javac . The patch appears to cause the build to fail. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/4519//console This message is automatically generated.
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        No new tests should be needed since we are just removing unused code.

        Show
        arpitagarwal Arpit Agarwal added a comment - No new tests should be needed since we are just removing unused code.
        Hide
        cnauroth Chris Nauroth added a comment -

        Sorry, Arpit. I think I led us astray. Those JSP pages are still in use. Running "hdfs journalnode" launches JournalNode, which creates JournalNodeHttpServer, which creates an HttpServer with name "journal". This causes the HttpServer to load the JSP pages from webapps/journal.

        I said earlier that these JSP pages don't do much, but there is one thing:

        <b><a href="/logs/">Logs</a></b>
        

        If a user looking for the journal node logs points a browser at the root, then they'll get a friendly link to the correct URL for logs. Therefore, I think the JSP pages need to stay in place. This means that your first patch is good. So sorry for the churn.

        +1 for the first patch. I'll commit that soon.

        Show
        cnauroth Chris Nauroth added a comment - Sorry, Arpit. I think I led us astray. Those JSP pages are still in use. Running "hdfs journalnode" launches JournalNode , which creates JournalNodeHttpServer , which creates an HttpServer with name "journal". This causes the HttpServer to load the JSP pages from webapps/journal. I said earlier that these JSP pages don't do much, but there is one thing: <b><a href= "/logs/" >Logs</a></b> If a user looking for the journal node logs points a browser at the root, then they'll get a friendly link to the correct URL for logs. Therefore, I think the JSP pages need to stay in place. This means that your first patch is good. So sorry for the churn. +1 for the first patch. I'll commit that soon.
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        Thanks, good to learn!

        Show
        arpitagarwal Arpit Agarwal added a comment - Thanks, good to learn!
        Hide
        cnauroth Chris Nauroth added a comment -

        Setting Affects Version/s and Target Version/s to 3.0.0. The unused code only exists in trunk. It does not exist in branch-2 or branch-2.1-beta. (Presumably it was never merged from trunk.)

        Show
        cnauroth Chris Nauroth added a comment - Setting Affects Version/s and Target Version/s to 3.0.0. The unused code only exists in trunk. It does not exist in branch-2 or branch-2.1-beta. (Presumably it was never merged from trunk.)
        Hide
        cnauroth Chris Nauroth added a comment -

        I committed this to trunk. Arpit, thank you for the patch!

        Show
        cnauroth Chris Nauroth added a comment - I committed this to trunk. Arpit, thank you for the patch!
        Hide
        hudson Hudson added a comment -

        Integrated in Hadoop-trunk-Commit #3926 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3926/)
        HDFS-4904. Remove JournalService. Contributed by Arpit Agarwal. (Revision 1493235)

        Result = SUCCESS
        cnauroth : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1493235
        Files :

        • /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/server/journalservice
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/journalservice/TestJournalService.java
        Show
        hudson Hudson added a comment - Integrated in Hadoop-trunk-Commit #3926 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3926/ ) HDFS-4904 . Remove JournalService. Contributed by Arpit Agarwal. (Revision 1493235) Result = SUCCESS cnauroth : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1493235 Files : /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/server/journalservice /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/journalservice/TestJournalService.java
        Hide
        hudson Hudson added a comment -

        Integrated in Hadoop-Yarn-trunk #241 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/241/)
        HDFS-4904. Remove JournalService. Contributed by Arpit Agarwal. (Revision 1493235)

        Result = SUCCESS
        cnauroth : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1493235
        Files :

        • /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/server/journalservice
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/journalservice/TestJournalService.java
        Show
        hudson Hudson added a comment - Integrated in Hadoop-Yarn-trunk #241 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/241/ ) HDFS-4904 . Remove JournalService. Contributed by Arpit Agarwal. (Revision 1493235) Result = SUCCESS cnauroth : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1493235 Files : /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/server/journalservice /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/journalservice/TestJournalService.java
        Hide
        hudson Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #1431 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1431/)
        HDFS-4904. Remove JournalService. Contributed by Arpit Agarwal. (Revision 1493235)

        Result = FAILURE
        cnauroth : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1493235
        Files :

        • /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/server/journalservice
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/journalservice/TestJournalService.java
        Show
        hudson Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1431 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1431/ ) HDFS-4904 . Remove JournalService. Contributed by Arpit Agarwal. (Revision 1493235) Result = FAILURE cnauroth : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1493235 Files : /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/server/journalservice /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/journalservice/TestJournalService.java
        Hide
        hudson Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #1458 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1458/)
        HDFS-4904. Remove JournalService. Contributed by Arpit Agarwal. (Revision 1493235)

        Result = FAILURE
        cnauroth : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1493235
        Files :

        • /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/server/journalservice
        • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/journalservice/TestJournalService.java
        Show
        hudson Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1458 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1458/ ) HDFS-4904 . Remove JournalService. Contributed by Arpit Agarwal. (Revision 1493235) Result = FAILURE cnauroth : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1493235 Files : /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/server/journalservice /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/journalservice/TestJournalService.java

          People

          • Assignee:
            arpitagarwal Arpit Agarwal
            Reporter:
            sureshms Suresh Srinivas
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development