[ https://issues.apache.org/jira/browse/HDFS-11600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16395689#comment-16395689 ]
Xiao Chen commented on HDFS-11600: ---------------------------------- Thanks Sammi for the work here, LGTM in general! Just have 1 question and a few minor comments. Questions: - Looking at the original tests, I think TestDFSStripedOutputStreamWithFailure[000-210] covered continuously the range [0, 219], judging from this [code|https://github.com/apache/hadoop/blob/branch-3.0.0/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSStripedOutputStreamWithFailure.java#L714-L728]. Do you know why this range was chosen? In current patch, we test randomly in the same range but not continuously. But I think that should be fine. Comments: - import * is usually discouraged - less strict for tests, but please change if possible. - Should use slf4j Logger in the new code as we're (slowly) moving away from apache commons logging - This is from existing code, but now may be a good chance to change - could you do {{tearDown}} with a {{@After}} annotation? This way, each test doesn't have to try-finally. - Can still see the unused {{testCloseWithExceptionsInStreamer}} in {{TestDFSStripedOutputStreamWithFailureBase}} > Refactor TestDFSStripedOutputStreamWithFailure test classes > ----------------------------------------------------------- > > Key: HDFS-11600 > URL: https://issues.apache.org/jira/browse/HDFS-11600 > Project: Hadoop HDFS > Issue Type: Improvement > Components: test > Affects Versions: 3.0.0-alpha2 > Reporter: Andrew Wang > Priority: Minor > Attachments: HDFS-11600-1.patch, HDFS-11600.002.patch, > HDFS-11600.003.patch, HDFS-11600.004.patch, HDFS-11600.005.patch > > > TestDFSStripedOutputStreamWithFailure has a great number of subclasses. The > tests are parameterized based on the name of these subclasses. > Seems like we could parameterize these tests with JUnit and then not need all > these separate test classes. > Another note, the tests will randomly return instead of running the test. > Using {{Assume}} instead would make it more clear in the test output that > these tests were skipped. -- This message was sent by Atlassian JIRA (v7.6.3#76005) --------------------------------------------------------------------- To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org