[
https://issues.apache.org/jira/browse/HADOOP-13207?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15385088#comment-15385088
]
Chris Nauroth commented on HADOOP-13207:
----------------------------------------
[[email protected]], thank you for the patch.
The tests looks great. I have two minor comments.
# {{AbstractFSContractTestBase#methodName}} is unused. Was this meant to be
used like in {{AbstractS3ATestBase}} to make the JUnit thread's name
descriptive to the specific test case?
# In {{ContractTestUtils#pathsToString}}, I recommend using the
platform-specific line ending via something like {{String.format("%n")}}
instead of {{\n}}.
On the documentation, I have a set of nit-picky proofreading comments.
{code}
### `FileStatus[] listStatus(Path p, PathFilter filter)`
{code}
In other parts of the patch, it looks like you are aiming for consistent use of
"path" instead of "p". Do you want to switch this method signature to use
"path" too?
{code}
then the that file's `FileStatus` entry is returned in a single-element array.
{code}
Something looks off here. Maybe remove "the"?
{code}
If the path refers to a directory, the call returns a list of all its immediate
children
path which are accepted by the filter —and does not include the directory
itself.
{code}
Please change "path" to plural "paths".
{code}
* After an entry at path `P` is created, and before any other
changes are made to the FileSystem, `listStatus(P)` MUST
find the file and return its status.
* After an entry at path `P` is deleted, and before any other
changes are made to the filesystem, `listStatus(P)` MUST
raise a `FileNotFoundException`.
{code}
The various "before any other changes are made" clauses use a mix of
"FileSystem" and "filesystem". Would you please make that consistent?
{code}
There no guarantees as to whether paths are listed in a specific order, only
{code}
I think this was meant to be "There are no guarantees...".
{code}
`listLocatedStatus(Path path):`. Calls to it may deletegated through
{code}
Is this meant to be "...may be delegated..."?
{code}
There is no requirement more the iterator to provide a consistent view
{code}
I'm unclear about the word "more" here.
{code}
if isFile(FS, path) and filter.accept(path) :
resultset = [ getLocatedFileStatus(FS, path) ]
elif isFile(FS, path) and not filter.accept(P) :
{code}
Please switch to "path" in the second filter.accept call for consistency.
{code}
results, *even if no calls to `hasNext()` are made.
{code}
Is there a missing closing '*'?
{code}
on (possibly remote) filesystems. These filesystems are invaariably accessed
{code}
s/invaariably/invariably
> Specify FileSystem listStatus, listFiles and RemoteIterator
> -----------------------------------------------------------
>
> Key: HADOOP-13207
> URL: https://issues.apache.org/jira/browse/HADOOP-13207
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: documentation, fs
> Affects Versions: 2.8.0
> Reporter: Steve Loughran
> Assignee: Steve Loughran
> Attachments: HADOOP-13207-branch-2-001.patch,
> HADOOP-13207-branch-2-002.patch, HADOOP-13207-branch-2-003.patch,
> HADOOP-13207-branch-2-004.patch, HADOOP-13207-branch-2-005.patch,
> HADOOP-13207-branch-2-006.patch, HADOOP-13207-branch-2-007.patch,
> HADOOP-13207-branch-2-008.patch, HADOOP-13207-branch-2-009.patch,
> HADOOP-13207-branch-2-010.patch
>
>
> The many `listStatus`, `listLocatedStatus` and `listFiles` operations have
> not been completely covered in the FS specification. There's lots of implicit
> use of {{listStatus()}} path, but no coverage or tests of the others.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
