[
https://issues.apache.org/jira/browse/HADOOP-7360?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13068675#comment-13068675
]
Matt Foley commented on HADOOP-7360:
------------------------------------
I think we're very close, but I do have some more questions:
1. CopyCommands#Get: class instance variable LocalFileSystem localFs is no
longer used and should be deleted.
Also, Get.processPath(PathData,PathData) now assumes that PathData.toFile()
returns a file in the LocalFS, so see next question below.
PathData:
2. PathData.toFile(): Is it the intent that, if caller passes in a non-local
file, the cast to (LocalFileSystem) will throw undeclared ClassCastException?
That okay, if so, but let's document it:
Since the intent (from CopyCommands#Get.processPath() and
CopyCommands#Get.copyFileToLocal()) is to create a file in the LocalFS only,
and assuming that the cast to (LocalFileSystem) is intended to throw an
exception if applied to a non-local fs, please change the method name to
"toLocalFile()" and JavaDoc the requirement that this PathData must be for a
local path, and "@throws ClassCastException if this.fs is not the
LocalFileSystem."
3. PathData.toString(): the comment says should return the "string" if
available, but it just returns uri.toString(), which presumably won't be the
same. Specifically, in related method Path.toString() it says:
{code}
// we can't use uri.toString(), which escapes everything, because we want
// illegal characters unescaped in the string, for glob processing, etc.
{code}
Is this really okay for PathData.toString(), given that your usage specifically
requires un-munged globbish characters?
4. In PathData ctors, the use of URI.create() may throw undeclared
RuntimeExceptions. Will this be handled okay by higher-level code?
5. In main ctor, I'm very happy that this.path is initialized to a fully
qualified path. Does this work correctly for files that don't exist yet? (It
looks like it probably will, but please confirm you've tested this case.)
6. lookupStat() should be changed to take path rather than pathString as its
argument. This uses the qualified path, which we know was correctly resolved at
PathData initialization time. Going thru {{new Path(uri.toString())}} risks
re-resolution of relative pathstrings during refreshStatus(), and we can't know
if the CWD has changed in the meantime.
Or do you expect to do lookupStat() on globstrings with singular resolutions?
Maybe that case should be dealt with specially if needed.
7. checkIfDirectory(boolean flag): Please change name of argument to
"desiredResult" or "testPolarity" or some such, and JavaDoc {{@param
desiredResult - if true, throws an exception when "this" is not a directory; if
false, throws an exception when "this" IS a directory}} and {{@throws
PathIOException if path doesn't exist or if result of check is not
desiredResult}}
8. getStringForChildPath(): if caller provides an absolute path, this won't
behave right. It should check for the path being a relative path, no? Or could
use relativize() if that's the right thing to do, but should probably still
throw an exception if it isn't actually a child of this.
9. PathType: It's actually "scheme" not "schema". And strictly speaking all
scheme-having paths are absolute, so ABSOLUTE could be SCHEMELESS_ABSOLUTE (ie,
slash-relative) to make the three terms mutually exclusive -- but I don't
insist on it.
10. expandAsGlob(): please use "cwd" (current working directory) instead of
"pwd" (print working directory).
11. removeAuthority(): I don't understand the comment at the beginning of the
method.
TestPathData:
Looks okay.
> FsShell does not preserve relative paths with globs
> ---------------------------------------------------
>
> Key: HADOOP-7360
> URL: https://issues.apache.org/jira/browse/HADOOP-7360
> Project: Hadoop Common
> Issue Type: Improvement
> Components: fs
> Affects Versions: 0.23.0
> Reporter: Daryn Sharp
> Assignee: Daryn Sharp
> Attachments: HADOOP-7360-2.patch, HADOOP-7360-3.patch,
> HADOOP-7360-4.patch, HADOOP-7360.patch
>
>
> FsShell currently preserves relative paths that do not contain globs.
> Unfortunately the method {{fs.globStatus()}} is fully qualifying all returned
> paths. This is causing inconsistent display of paths.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
