[
https://issues.apache.org/jira/browse/HDFS-14195?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16742009#comment-16742009
]
Adam Antal commented on HDFS-14195:
-----------------------------------
Thanks for the patch [~suxingfate], good job.
I have some general comment regarding your patch:
Although as I see it too, the imports are not in the correct order, but I
think it is more important to keep the git history clean, and not reorganizing
the import.
Also be aware not to use * imports (in
{{TestOfflineImageViewerForStoragePolicy.java:36}} import java.io.*; ).
Making a new test file {{TestOfflineImageViewerForStoragePolicy.java}} to test
only the StoragePolicy field may be a bit too much, but since we do need to
modify the fsimage and can't force it into the {{createOriginalFSImage()}}
function of the {{TestOfflineImageViewer.java}}, I'm okay with it.
There are a bunch of checkstyle issues, please take care of them. You can find
the list here:
[https://builds.apache.org/job/PreCommit-HDFS-Build/25971/artifact/out/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt]
For too long lines you can break up strings.
I had a suggestion that we can add a Builder pattern to the construction of
these entries in {{PBImageDelimitedTextWriter.java}}. It's quite unrelated, I
have filed a separate issue for that (HDFS-14203).
Though comparing the file line by line can also point to the place of the
problem, but maybe you can compare the whole file as well (storing the expected
result in a csv and reading it in the testcase). If the compar fails, the diff
is also logged out anyways. It can be something like in
{{TestOfflineImageViewer$testCorruptionDetectionSingleFileCorruption}}.
I observed some typos:
{code:java}
final static HashMap<String, Byte> writtenStoragePolicys = Maps.newHashMap();
{code}
where should be policies, and also:
{code:java}
Path dir = new Path("/dirWithStoragePolicyUnspecifed");
{code}
and later in some variable names like {{subDirWithStoragePolicyUnspecifed}}:
unspecified is missing an "i".
Also these names can be abbreviated a bit, to variable names of length 15-20
maximum
The FSImage generation is basically the same as in
{{TestOfflineImageViewer$createOriginalFSImage}}, but the comment "We only want
to generate the fsimage file once and use it for multiple tests." is not true
here, since we only have one testcase. As I said earlier, we still do need to
construct it somewhere, so it is ok, but the comment should be modify to not
mislead the person reading the comment.
DFSTestUtil has some StoragePolicy related function, that may or may not be
useful (just pointing to it, I did not read dig into it). Also the
DFSTestUtil.createFile might come handy for you. I wonder what other tests do
to generate a file with some given StoragePolicy. As I saw DFSTestUtil does not
include this kind of function, but you can wrap it into the testfile, if it
makes it more compact: function which creates a file and gives a StoragePolicy
by a parameter.
I think the line
{code:java}
final String DELIMITER = "\t";
{code}
can be move to class-level static variable (also the notice checkstyle
warning). After this you can simply leave the commented part out.
As I see this line
{code:java}
System.out.println(line);
{code}
is not needed in
{{TestOfflineImageViewerForStoragePolicy$testPBDelimitedWriterForStoragePolicy:183}}.
In {{TestOfflineImageViewer}} it is also that way, which was my bad.
The addition of the field and the principle of the test is good though, so I
can't add anything to it. After you take care of these items, the patch will be
fine.
> OIV: print out storage policy id in oiv Delimited output
> --------------------------------------------------------
>
> Key: HDFS-14195
> URL: https://issues.apache.org/jira/browse/HDFS-14195
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: tools
> Reporter: Wang, Xinglong
> Priority: Minor
> Attachments: HDFS-14195.001.patch, HDFS-14195.002.patch
>
>
> There is lacking of a method to get all folders and files with sort of
> specified storage policy via command line, like ALL_SSD type.
> By adding storage policy id to oiv output, it will help with oiv
> post-analysis to have a overview of all folders/files with specified storage
> policy and to apply internal regulation based on this information.
>
> Currently, for PBImageXmlWriter.java, in HDFS-9835 it added function to print
> out xattr which including storage policy already.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
