[
https://issues.apache.org/jira/browse/HADOOP-13254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15332673#comment-15332673
]
Daniel Templeton commented on HADOOP-13254:
-------------------------------------------
Thanks, [~yufei]. A few more comments:
* Would you mind adding some comments to
{{DiskValidatorFactory.getInstance(Class)}} that explains why you're checking
the result of the put and using it if it's not null? I don't think it's
obvious that it's because it could be used by multiple threads.
* This assert message:
{code}
assertTrue("checkDir success", success);
{code}
still needs to be clearer. Something like, "call to checkDir() succeeded even
though it was expected to fail"
* In {{TestBasicDiskValidator.checkDirs()}}, I think the try for the
try-finally should start a bit earlier so that it encompasses all possible
unexpected exit points.
* In {{TestBasicDiskValidator.checkDirs()}}, this code:
{code}
File localDir = File.createTempFile("test", "tmp");
if (isDir) {
localDir.delete();
localDir.mkdir();
}
{code}
doesn't make any sense to me. Shouldn't you test if it should be a dir *first*
instead of deleting the file and creating a dir if that's what's needed?
* In {{TestDiskValidatorFactory.testGetInstance()}}, at the end you try to get
a bad instance, but you don't do anything with the result. If that's on
purpose, you should at least document it.
> Make Diskchecker Pluggable
> --------------------------
>
> Key: HADOOP-13254
> URL: https://issues.apache.org/jira/browse/HADOOP-13254
> Project: Hadoop Common
> Issue Type: Bug
> Components: util
> Reporter: Yufei Gu
> Assignee: Yufei Gu
> Attachments: HADOOP-13254.001.patch, HADOOP-13254.002.patch,
> HADOOP-13254.003.patch, HADOOP-13254.004.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
