[
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
