[#HADOOP-2634] Deprecate exists() and isDir() to simplify ClientProtocol.

Hadoop Common

Deprecate exists() and isDir() to simplify ClientProtocol.

Created: 17/Jan/08 03:34 AM Updated: 08/Jul/09 04:42 PM

Return to search

Component/s:

None

Affects Version/s:

0.15.0

Fix Version/s:

0.17.0

Time Tracking:

Not Specified

File Attachments:

File Name	Date Attached	Attached By	Size
HADOOP-2634-1.patch	2008-04-01 09:07 AM	Lohit Vijayarenu	8 kB
HADOOP-2634-2.patch	2008-04-02 11:27 PM	Lohit Vijayarenu	8 kB

Issue Links:

Duplicate

This issue duplicates:

~~HADOOP-2470~~ Open and isDir should be removed from ClientProtocol

Reference

This issue relates to:

~~HADOOP-3160~~ remove exists() from ClientProtocol and NameNode

This issue is related to:

~~HADOOP-2566~~ need FileSystem#globStatus method

Hadoop Flags:	Reviewed, Incompatible change
Release Note:	Deprecates exists() from ClientProtocol
Resolution Date:	03/Apr/08 04:25 AM

Description

« Hide

ClientProtocol can be simplified by removing two methods

public boolean exists(String src) throws IOException;
public boolean isDir(String src) throws IOException;

This is a redundant api, which can be implemented in DFSClient as convenience methods using

public DFSFileInfo getFileInfo(String src) throws IOException;

Note that we already deprecated several Filesystem method and advised to use getFileStatus() instead.
Should we deprecate them in 0.16?

All

Comments

Work Log

Change History

Subversion Commits

Sort Order:

[ Permlink | « Hide ]

Doug Cutting added a comment - 17/Jan/08 07:42 PM

+1 for removing those protocol methods.

FileSystem#exists() should probably be made a concrete method in FileSystem.java, defined in terms of getFileStatus(), most existing implementations can probably be removed, and it could probably be deprecated.

BTW, what is getFileStatus() supposed to do when a file does not exist? Throw an IOException or return null? The former is generally preferable, but the latter makes implementing exists() easier, since we should not use exception handling for normal program flow.

I don't see a need to do this the day before 0.16 feature freeze, and it could be destabilizing.

[ Permlink | « Hide ]

Konstantin Shvachko added a comment - 17/Jan/08 09:52 PM

Right now getFileInfo() - an HDFS variant of getFileStatus() - throws
```
IOException("File does not exist: " + srcs);
```
LocalFileSystem does not throw anything but actually returns a valid FileStatus with some default values.

S3FileSystem throws

IOException(f.toString() + ": No such file or directory.");

And kfs does not seem to be throwing anything just like LocalFileSystem, please correct me if I'm wrong.

So this is all really inconsistent. And to make it consistent I would vote for throwing rather than returning null, but throwing FileNotFoundException instead of the base IOException. Then it would make implementation of exists() rather simple.

[ Permlink | « Hide ]

Doug Cutting added a comment - 17/Jan/08 09:58 PM

Hairong has addressed these inconsistencies in ~~HADOOP-2566~~.

Yes, the implementation of exists() in terms of getFileStatus() would be simple. However it is considered bad style to use exceptions for normal control flow, and exists() returning false is a normal condition. We might just have to live with that...

[ Permlink | « Hide ]

Hairong Kuang added a comment - 17/Jan/08 10:08 PM

~~HADOOP-2470~~ also discussed the unused methods in ClientProtocol. There is one more method that could be removed: open.

> Hairong has addressed these inconsistencies in ~~HADOOP-2566~~.
For dfs, I changed the client side to throw a FileNotFoundException. But I did not change the server side. It would be nice if getFileInfo in namenode also throw a FileNotFoundException.

[ Permlink | « Hide ]

Tsz Wo (Nicholas), SZE added a comment - 10/Mar/08 11:22 PM - edited

isDir() will be removed in ~~HADOOP-2470~~.

[ Permlink | « Hide ]

Lohit Vijayarenu added a comment - 01/Apr/08 09:07 AM

Talked to Konstantine regarding this. Even though the idea of catching exception and returning bool does not look correct, for time being that approach was felt acceptable. The attached patch implements exists(Path) in terms of getFileStatus(Path). This patch also deprecates ClientProtocol#exists and Serverside exists.

[ Permlink | « Hide ]

Hadoop QA added a comment - 01/Apr/08 08:51 PM

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12379012/HADOOP-2634-1.patch
against trunk revision 643282.

@author +1. The patch does not contain any @author tags.

tests included -1. The patch doesn't appear to include any new or modified tests.
Please justify why no tests are needed for this patch.

javadoc +1. The javadoc tool did not generate any warning messages.

javac +1. The applied patch does not generate any new javac compiler warnings.

release audit +1. The applied patch does not generate any new release audit warnings.

findbugs +1. The patch does not introduce any new Findbugs warnings.

core tests +1. The patch passed core unit tests.

contrib tests +1. The patch passed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2115/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2115/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2115/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2115/console

This message is automatically generated.

[ Permlink | « Hide ]

Lohit Vijayarenu added a comment - 01/Apr/08 09:28 PM

This patch deprecates exists() in ClientProtocol so does not include any testcase.

[ Permlink | « Hide ]

Konstantin Shvachko added a comment - 02/Apr/08 09:48 PM

Filesystem.exists() can return true even if getFileStatus() returns null.
We so not know about all file systems how they implement exists, so I would do
```
return getFileStatus() != null;
```
Same in DFSClient.exists().

The rest looks good. +1