hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Xiao Chen (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-13191) FileSystem#listStatus should not return null
Date Fri, 26 Aug 2016 23:43:21 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-13191?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15440217#comment-15440217
] 

Xiao Chen commented on HADOOP-13191:
------------------------------------

Thanks for working on this [~jzhuge].

Some review comments from me:
- This duplicates HADOOP-7352, which predates this jira. Maybe we can move the patch and further
discussions to there? The code changes are mostly in common too.
- bq. (Steve L): I'm in favour of this, and returning an empty list.
IMHO returning an empty list is less surprising, throwing an IOE will make the error more
explicit. Although this jira maybe incompatible, I think returning an empty list maintains
a more compatible behavior (Client code may not need any change if empty list), so personally
inclined with empty list too.
- I don't think throwing {{AccessControlException}} in {{RawLocalFileSystem#listStatus}} is
correct. The javadoc of {{File#list}} mentions various of scenarios, {{AccessControlException}}
is one of those scenarios. I think we could do sth similar to [FileUtil|https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java#L1147].
- Also I don't see much value in the comment before throwing. One can just check javadoc to
see what a method returns.
- Not sure about {{@Nonnull}}. Would this add a dependency to javax to common? It's currently
in hdfs only. I wouldn't worry about this normally, but would like other people's thoughts
since this is {{FileSystem.java}}.

> FileSystem#listStatus should not return null
> --------------------------------------------
>
>                 Key: HADOOP-13191
>                 URL: https://issues.apache.org/jira/browse/HADOOP-13191
>             Project: Hadoop Common
>          Issue Type: Bug
>          Components: fs
>    Affects Versions: 2.6.0
>            Reporter: John Zhuge
>            Assignee: John Zhuge
>            Priority: Minor
>         Attachments: HADOOP-13191.001.patch, HADOOP-13191.002.patch, HADOOP-13191.003.patch,
HADOOP-13191.004.patch
>
>
> This came out of discussion in HADOOP-12718. The {{FileSystem#listStatus}} contract does
not indicate {{null}} is a valid return and some callers do not test {{null}} before use:
> AbstractContractGetFileStatusTest#testListStatusEmptyDirectory:
> {code}
>     assertEquals("ls on an empty directory not of length 0", 0,
>         fs.listStatus(subfolder).length);
> {code}
> ChecksumFileSystem#copyToLocalFile:
> {code}
>       FileStatus[] srcs = listStatus(src);
>       for (FileStatus srcFile : srcs) {
> {code}
> SimpleCopyLIsting#getFileStatus:
> {code}
>       FileStatus[] fileStatuses = fileSystem.listStatus(path);
>       if (excludeList != null && excludeList.size() > 0) {
>         ArrayList<FileStatus> fileStatusList = new ArrayList<>();
>         for(FileStatus status : fileStatuses) {
> {code}
> IMHO, there is no good reason for {{listStatus}} to return {{null}}. It should throw
IOExceptions upon errors or return empty list.
> To enforce the contract that null is an invalid return, update javadoc and leverage @Nullable/@NotNull/@Nonnull
annotations.
> So far, I am only aware of the following functions that can return null:
> * RawLocalFileSystem#listStatus



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org


Mime
View raw message