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 Tue, 19 Feb 2013 20:53:13 GMT

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

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

Hi, Vadim.  Thanks for working on improving the test coverage in this area.  Here are a few
suggestions:

{code}
  @AfterClass
  public static void after() {
    cluster.shutdown();
  }
{code}

{{MiniDFSCluster#Builder#build}} could throw an exception, which would cause {{cluster}} to
be null.  Then, a {{NullPointerException}} here in {{after}} might mask the cause of the problem.
 Could you check this for null?  {{TestNameNodeJspHelper}} already has a check for null. 
Could you add the same check to {{TestClusterJspHelper}}?

{code}
    } catch (Exception ex) {
      fail("testDefaultNamenode ex error !!!" + ex);
    }
{code}

Instead, could you throw the exception out of the test method?  It looks like that's generally
the established pattern in the Hadoop tests.

{code}
  private static final String REDIRECT_URL_EPISODE = "/browseDirectory.jsp?namenodeInfoPort=";
{code}

I was confused by "episode" in this name.  Should it be {{REDIRECT_URL_LOCATION}} instead?

{code}
  private static Configuration conf = new HdfsConfiguration();
{code}

I recommend creating a new instance for {{conf}} during {{setUp}}, because the individual
tests mutate the configuration.  Creating a new instance per test would prevent any accidental
ordering requirements or logical dependencies between the tests.

{code}
    UserGroupInformation.setConfiguration(conf);
{code}

Similarly, I wonder if there is any need to restore {{UserGroupInformation}} configuration
to its original state after each test run, since this is mutating global state that is used
by subsequent tests.  I wonder if this explains any of the test failures?

{code}
  
  <property>
	  <name>fs.defaultFS</name>
	  <value>hdfs://localhost.localdomain:45541</value>
	  <description>The name of the default file system.  A URI whose
	  scheme and authority determine the FileSystem implementation.  The
	  uri's scheme determines the config property (fs.SCHEME.impl) naming
	  the FileSystem implementation class.  The uri's authority is used to
	  determine the host, port, etc. for a filesystem.</description>
  </property>	
{code}

It looks like this change caused multiple other tests to fail.  Perhaps it's easier to override
the configuration in each test that needs it instead of changing it in the shared hdfs-site.xml.
 If it needs to stay in hdfs-site.xml, could you please reformat it to match the rest of the
XML?  (i.e. 2 spaces for indentation)

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-2-a.patch, HADOOP-4510-trunk-a.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