Return-Path: 
 <hdfs-issues-return-57866-apmail-hadoop-hdfs-issues-archive=hadoop.apache.org@hadoop.apache.org>
X-Original-To: apmail-hadoop-hdfs-issues-archive@minotaur.apache.org
Delivered-To: apmail-hadoop-hdfs-issues-archive@minotaur.apache.org
Received: from mail.apache.org (hermes.apache.org [140.211.11.3])
	by minotaur.apache.org (Postfix) with SMTP id A0F12E349
	for <apmail-hadoop-hdfs-issues-archive@minotaur.apache.org>;
 Thu, 21 Feb 2013 20:16:13 +0000 (UTC)
Received: (qmail 91057 invoked by uid 500); 21 Feb 2013 20:16:13 -0000
Delivered-To: apmail-hadoop-hdfs-issues-archive@hadoop.apache.org
Received: (qmail 91011 invoked by uid 500); 21 Feb 2013 20:16:13 -0000
Mailing-List: contact hdfs-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:hdfs-issues-help@hadoop.apache.org>
List-Unsubscribe: <mailto:hdfs-issues-unsubscribe@hadoop.apache.org>
List-Post: <mailto:hdfs-issues@hadoop.apache.org>
List-Id: <hdfs-issues.hadoop.apache.org>
Reply-To: hdfs-issues@hadoop.apache.org
Delivered-To: mailing list hdfs-issues@hadoop.apache.org
Received: (qmail 91001 invoked by uid 99); 21 Feb 2013 20:16:13 -0000
Received: from arcas.apache.org (HELO arcas.apache.org) (140.211.11.28)
    by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 21 Feb 2013 20:16:13 +0000
Date: Thu, 21 Feb 2013 20:16:13 +0000 (UTC)
From: "Chris Nauroth (JIRA)" <jira@apache.org>
To: hdfs-issues@hadoop.apache.org
Message-ID: <JIRA.12632794.1361173242732.319304.1361477773141@arcas>
In-Reply-To: <JIRA.12632794.1361173242732@arcas>
References: <JIRA.12632794.1361173242732@arcas>
Subject: [jira] [Commented] (HDFS-4510) Cover classes
 ClusterJspHelper/NamenodeJspHelper with unit tests
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
X-JIRA-FingerPrint: 30527f35849b9dde25b450d4833f0394


    [ 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