hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Nauroth (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-4510) Cover classes ClusterJspHelper/NamenodeJspHelper with unit tests
Date Thu, 21 Feb 2013 20:16:13 GMT

    [ https://issues.apache.org/jira/browse/HDFS-4510?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13583513#comment-13583513
] 

Chris Nauroth commented on HDFS-4510:
-------------------------------------

Thank you for addressing the feedback, Vadim.  I tested the new patch successfully.  Please
disregard my earlier comments about not creating conf as a static in {{TestNameNodeJspHelper}}.
 I can see now that it's safe for this test.

Here are just a few more really minor things:

{code}
  static final class ConfigurationForTestClusterJspHelper extends Configuration {
    static {
      addDefaultResource("testClusterJspHelperProp.xml");
    }
  }
{code}

Is the subclass necessary?  I think calling {{Configuration#addDefaultResource}} from a static
initialization block in {{TestClusterJspHelper}} and then using {{new Configuration()}} would
have the same effect.

{code}
--- hadoop-hdfs-project/hadoop-hdfs/src/test/resources/hdfs-site.xml
+++ hadoop-hdfs-project/hadoop-hdfs/src/test/resources/hdfs-site.xml
@@ -24,6 +24,5 @@
   <property>
     <name>hadoop.security.authentication</name>
     <value>simple</value>
-  </property>
-
+  </property>    
 </configuration>
{code}

Is there actually a change in hdfs-site.xml?  If not, could you remove it from the patch?

{code}
<configuration>
  <property>
    <name>fs.defaultFS</name>
    <value>hdfs://localhost.localdomain:45541</value>
    <description></description>
  </property>
  <property>
    <name>hadoop.security.authentication</name>
    <value>simple</value>
    </property>
</configuration>
{code}

Minor nitpicks: could you remove the empty <description> and indent the last </property>
2 spaces instead of 4?

{quote}
-1 one of tests included doesn't have a timeout
{quote}

There was a change submitted just this morning to test-patch.sh to enforce that all new tests
must specify a timeout in the annotation, i.e. @Test(timeout=30000), so let's add that to
the new tests.

Thanks again!

                
> Cover classes ClusterJspHelper/NamenodeJspHelper with unit tests
> ----------------------------------------------------------------
>
>                 Key: HDFS-4510
>                 URL: https://issues.apache.org/jira/browse/HDFS-4510
>             Project: Hadoop HDFS
>          Issue Type: Test
>    Affects Versions: 3.0.0, 2.0.3-alpha, 0.23.6
>            Reporter: Vadim Bondarev
>         Attachments: HADOOP-4510-branch-0.23-a.patch, HADOOP-4510-branch-0.23-b.patch,
HADOOP-4510-branch-2-a.patch, HADOOP-4510-branch-2-b.patch, HADOOP-4510-trunk-a.patch, HADOOP-4510-trunk-b.patch
>
>


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message