Open and isDir should be removed from ClientProtocol

• Key: HADOOP-2470
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Tsz Wo (Nicholas), SZE
• Reporter: Hairong Kuang
• Affects Version/s: 0.15.1
• Fix Version/s: 0.17.0

Description

Methods open and isDir in ClientProtocol are no longer used. DFSClient uses getBlockLocations and getFileInfo instead. So open and isDir should be removed from ClientProtocol.

Comments

[Tsz Wo (Nicholas), SZE]

> Methods open and isDir in ClientProtocol are no longer used.

I think you mean open and isDir can be replaced. Currently, open and isDir are called in a few places but they can be obviously replaced by getBlockLocations and getFileInfo.

[Doug Cutting]

> I think you mean open and isDir can be replaced.

FileSystem#isDirectory() is implemented in terms of FileSystem#getFileInfo(), so removing the DistributedFileSystem implementation should work fine, no?

And, yes, open() is still called. It's implementation on the NameNode is identical to getBlockLocations, except for the update of a counter. So, if that counter is not critical, open() could also be removed.

[Sanjay Radia]

Being able to distinguish between open counts and getLocation counts is not necessary.
From that point getting rid of the open rpc call seems reasonable.

The only advantage the open call has (and we don't take advantage of this) is to have only open
check the file perms once and for the future getLocations of the opened file, pass the openFileId.
(Current getLocations has the pathname). Such as scheme
would be more efficient for permission checking and would also have the desired property that once you have opened a file for reading you can continue to read it in face of permission changes. The cost of doing this is to maintain a openFileDescriptors on the NN side + leases to recover. The cost of this would be quite high given that NN can support tens of thousands readers. I doubt if we will ever maintain open-file descriptors on NN side.

SO my vote is +1

[Tsz Wo (Nicholas), SZE]

2470_20080310.patch: deprecated of isDir(...) and open(...) in NameNode and ClientProtocol and removed the use of them. Since there are already a lot of tests for testing the functionality, no additional tests are added.

[Hairong Kuang]

I would prefer to simply remove the methods open & isDir in ClientProtocol, Namenode, and FSNameSystem. Since they are not user-facing public APIs, we do not need to deprecate them first.

[Tsz Wo (Nicholas), SZE]

2470_20080312.patch: removed open & isDir. Also removed getContentLength which is already deprecated.

[Hairong Kuang]

+1 The patch looks good.

[Hadoop QA]

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12377714/2470_20080312.patch
against trunk revision 619744.

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

tests included +1. The patch appears to include 3 new or modified tests.

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

javac -1. The applied patch generated 600 javac compiler warnings (more than the trunk's current 598 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/1955/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1955/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1955/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1955/console

This message is automatically generated.

[Tsz Wo (Nicholas), SZE]

The new javac warnings are due to the newly deprecated DFSClient.isDirectory(String).

[Chris Douglas]

I just committed this. Thanks, Nicholas!

[Hudson]

Integrated in Hadoop-trunk #428 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/428/)

[Robert Chansler]

Noted as incompatible in changes.txt