[
https://issues.apache.org/jira/browse/HADOOP-12056?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14570060#comment-14570060
]
Lei (Eddy) Xu commented on HADOOP-12056:
----------------------------------------
Hi, [~zxu] Nice find.
This patch looks good in general. It'd be nice to address a few small comments,
mostly regarding the unit tests.
* The test name should be {{testCheckDirsOnFile()}}.
* Can you create the test file under test dirs, which can be obtained from
{{PathUtils#getTestDir()}}:
{code}
File localDir = File.createTempFile("test", "tmp");
{code}
* I have a question in the following code:
{code}
File spyLocalDir = spy(localDir);
doReturn(null).when(spyLocalDir).listFiles();
{code}
Why not pass a file directly? Would the case, which the input is a file, be
handled by {{checkDir(dir)}} directly? I think you can pass a regular file to
see what happens.
* We can use {{GenericTestUtils#assertExceptionContains}} to verify the
{{DiskErrorException}} is correct.
+1 non-binding once these are addressed. Thanks again for working on this.
> DiskChecker#checkDirs should check null pointer for the return value from
> File#listFiles to avoid NPE.
> ------------------------------------------------------------------------------------------------------
>
> Key: HADOOP-12056
> URL: https://issues.apache.org/jira/browse/HADOOP-12056
> Project: Hadoop Common
> Issue Type: Bug
> Components: util
> Affects Versions: 2.7.0
> Reporter: zhihai xu
> Assignee: zhihai xu
> Attachments: HADOOP-12056.000.patch
>
>
> DiskChecker#checkDirs should check null pointer for the return value from
> File#listFiles. Based on the document for File#listFiles at
> https://docs.oracle.com/javase/7/docs/api/java/io/File.html#listFiles() :
> {code}
> Returns null if an I/O error occurs.
> {code}
> So it will be good to check null pointer and throw DiskErrorException if it
> is null.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
