[
https://issues.apache.org/jira/browse/HDFS-9188?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14949284#comment-14949284
]
Colin Patrick McCabe commented on HDFS-9188:
--------------------------------------------
Thanks, [~eddyxu].
{code}
38 abstract class Factory<D extends FsDatasetTestUtils> {
39 /**
40 * By default returns the Flood version of the factory.
41 *
42 * @return The configured Factory.
43 */
{code}
This comment seems out of place.
{code}
26 public class FsDatasetTestUtilsFactory
27 extends FsDatasetTestUtils.Factory<FsDatasetTestUtils> {
28 @Override
29 public FsDatasetTestUtils newInstance(DataNode datanode) {
30 return new FsDatasetImplTestUtils(datanode);
31 }
{code}
The factory name should be {{FsDatasetImplTestUtilsFactory}}, since it's
returning an instance of {{FsDatasetImplTestUtils}}. Also, this should be
{{final}}.
{code}
60 private static boolean corrupt(File file) throws IOException {
61 if (file == null || !file.exists()) {
62 return false;
63 }
64 LOG.debug("Corrupting file: " + file);
65 StringBuilder content = new
StringBuilder(DFSTestUtil.readFile(file));
66 char c = content.charAt(0);
67 content.setCharAt(0, ++c);
68 try (PrintWriter out = new PrintWriter(file)) {
69 out.print(content);
70 out.flush();
71 }
72 return true;
{code}
This is assuming that the content is a string. It would be better to just
manipulate the file as a binary array. This also doesn't specify the string
encoding (unfortunately, we can't take UTF-8 for granted here).
{code}
88 /** Corrupt a block / crc file by deleting it. */
89 private static boolean delete(File file) {
90 if (file == null || !file.exists()) {
91 return false;
92 }
93 return file.delete();
94 }
{code}
Please use {{Files#delete}}, which throws an exception when it fails. And
don't return a boolean. See
http://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#delete(java.nio.file.Path)
{code}
101 @Override
102 public boolean corruptData() throws IOException {
103 LOG.info("Corrupting block file: " + blockFile);
104 return corrupt(blockFile);
105 }
{code}
Why does this return a boolean? If it fails it should throw an exception with
the reason why it failed. Same question with truncate.
If we need a {{MaterializedReplica#exists}} method then let's add one rather
than making everything return false if the replica doesn't exist.
> Make block corruption related tests FsDataset-agnostic.
> --------------------------------------------------------
>
> Key: HDFS-9188
> URL: https://issues.apache.org/jira/browse/HDFS-9188
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: HDFS, test
> Affects Versions: 2.7.1
> Reporter: Lei (Eddy) Xu
> Assignee: Lei (Eddy) Xu
> Attachments: HDFS-9188.000.patch, HDFS-9188.001.patch,
> HDFS-9188.002.patch, HDFS-9188.003.patch
>
>
> Currently, HDFS does block corruption tests by directly accessing the files
> stored on the storage directories, which assumes {{FsDatasetImpl}} is the
> dataset implementation. However, with works like OZone (HDFS-7240) and
> HDFS-8679, there will be different FsDataset implementations.
> So we need a general way to run whitebox tests like corrupting blocks and crc
> files.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
