[
https://issues.apache.org/jira/browse/HADOOP-13254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15330842#comment-15330842
]
Daniel Templeton commented on HADOOP-13254:
-------------------------------------------
Thanks, [~yufeigu]. A few comments:
* Can we add {{@Override}} to {{BasicDiskValidator.checkStatus()}}?
* In {{DiskValidatorFactory. getInstance(Class)}}, shouldn't the {{put()}} call
be {{putIfAbsent()}}?
* In {{DiskValidatorFactory. getInstance(Class)}}, can you use a local variable
to return at the end instead of having three exits points from the method?
* In {{DiskValidatorFactory. getInstance(String)}}, the {{toLowerCase()}} is
superfluous.
* In {{DiskValidatorFactory. getInstance(String)}}, when you create the
exception, please include the source exception as the cause.
* In {{TestBasicDiskValidator}}, I'm pretty sure that all of the tests
inherited from {{TestDiskValidator}} will be run anyway; there's no need to
wrap them. I think you're making the tests run twice.
* In {{TestBasicDiskValidator.checkDirs()}}, you should make sure the temp file
gets deleted. Either try-finally or use {{deleteOnExit()}}.
* In {{TestBasicDiskValidator.checkDirs()}}, please remove the
{{System.out.println()}}.
* In {{TestBasicDiskValidator.checkDirs()}}, please make the assert messages
more useful. What exactly failed and most likely why?
* In {{TestDiskValidatorFactory.testGetInstance()}}, you should use
{{assertNotNull()}} instead of {{assertTrue()}}.
* In {{TestDiskValidatorFactory.testGetInstance()}}, it would be good to check
that the instance type that you get back in the right one. It would also be
nice to test that you're actually caching correctly. You might have to make
{{INSTANCES}} {{@VisibleForTesting}} to make that work.
> 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
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
