[
https://issues.apache.org/jira/browse/SUBMARINE-68?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16841418#comment-16841418
]
Adam Antal commented on SUBMARINE-68:
-------------------------------------
Thanks for the patch [~snemeth]!
- It seems a bit red flag for me that {{MockRemoteDirectoryManager.java}}'s
getJobStagingArea must be called before its methods are used. I don't have a
constructive way to change this, but I'd rather call that function {{init()}}
or explicitly say it in the javadoc of the class and that method how to use it.
- In the javadoc of {{downloadAndZip}} you should say "May _call the_
downloadInternal function.." rather than "May downloadInternal...".
- how many times will you be using {{getFilePathInTempDir}}? It makes sense to
cache the output of {{System.getProperty("java.io.tmpdir")}} in as private
attribute of the class.
- In {{downloadRemoteFile}} I'd rather say
{noformat}
LOG.info("Downloaded {} remote file to this local path: {}.", remoteDir,
zipDirPath);
{noformat}
- Also in {{validFileSize()}} I'd refine the log phrase to
{noformat}
LOG.info("{} file / directory path is {} with size {} bytes."
+ " Allowed max file / directory size is {} bytes",
locationType, uri, actualSizeInBytes, maxFileSizeInBytes);
{noformat}
- In {{getMaxRemoteFileSizeMB()}} the JVM can optimize these constants better,
if you move it to a static field (like lots of other places in hadoop) e.g.:
{noformat}
public class NodePage extends NMView {
private static final long BYTES_IN_MB = 1024 * 1024;
{noformat}
- In {{Localizer$handleLocalizations}} this comment just does not make any
sense. Could you care of that?
{noformat}
// Non HDFS remote uri. Non directory, no need to zip
{noformat}
- Please use the other constructor to obtain a File object
{{FileUtilitiesForTests $createDirInDir}}. So instead of this:
{noformat}
File file =
new File(dir.toUri().getPath() + "/" + new Path(newDir).getName());
{noformat}
Use this:
{noformat}
File file =
new File(dir.toUri().getPath(), newDir);
{noformat}
Also do we need to have this: _new Path(newDir).getName()_ ? Isn't enough to
just simply call with parameter _newDir_?
Also if you ever came across the literal "/", I encourage you to rather use
File.separator to be FileSystem-compatible (for e.g. to work with Windows).
Regarding the testcases:
- {{setupService()}} looks meaningless. You construct an object and discard it.
You can remove it.
- For supportability you may move {{FILE_SCHEME}} to {{FileUtilitiesForTests}}.
- Also as I checked the files constructed in
{{testUploadToRemoteFileMultipleFiles}} and other places by
{{FileUtilitiesForTests}} object:
{noformat}
File testFile1 = fileUtils.createFileInTempDir("testFile1");
File testFile2 = fileUtils.createFileInTempDir("testFile2");
{noformat}
Are they getting cleaned up after the run? It seems to me that in
{{FileUtilitiesForTests}} they're not getting added to the {{cleanupFiles}}.
> Add tests to FileSystemOperations class
> ---------------------------------------
>
> Key: SUBMARINE-68
> URL: https://issues.apache.org/jira/browse/SUBMARINE-68
> Project: Hadoop Submarine
> Issue Type: Improvement
> Reporter: Szilard Nemeth
> Assignee: Szilard Nemeth
> Priority: Minor
> Attachments: SUBMARINE-68.001.patch, SUBMARINE-68.002.patch,
> SUBMARINE-68.003.patch
>
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
