Hadoop Common
  1. Hadoop Common
  2. HADOOP-2470

Open and isDir should be removed from ClientProtocol

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.15.1
    • Fix Version/s: 0.17.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change
    • Release Note:
      Open and isDir were removed from ClientProtocol.

      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.

      1. 2470_20080310.patch
        9 kB
        Tsz Wo Nicholas Sze
      2. 2470_20080312.patch
        12 kB
        Tsz Wo Nicholas Sze

        Issue Links

          Activity

          Hide
          Tsz Wo Nicholas Sze added a comment -

          > 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.

          Show
          Tsz Wo Nicholas Sze added a comment - > 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.
          Hide
          Doug Cutting added a comment -

          > 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.

          Show
          Doug Cutting added a comment - > 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.
          Hide
          Sanjay Radia added a comment -

          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

          Show
          Sanjay Radia added a comment - 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
          Hide
          Tsz Wo Nicholas Sze added a comment -

          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.

          Show
          Tsz Wo Nicholas Sze added a comment - 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.
          Hide
          Hairong Kuang added a comment -

          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.

          Show
          Hairong Kuang added a comment - 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.
          Hide
          Tsz Wo Nicholas Sze added a comment -

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

          Show
          Tsz Wo Nicholas Sze added a comment - 2470_20080312.patch: removed open & isDir. Also removed getContentLength which is already deprecated.
          Hide
          Hairong Kuang added a comment -

          +1 The patch looks good.

          Show
          Hairong Kuang added a comment - +1 The patch looks good.
          Hide
          Hadoop QA added a comment -

          -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.

          Show
          Hadoop QA added a comment - -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.
          Hide
          Tsz Wo Nicholas Sze added a comment -

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

          Show
          Tsz Wo Nicholas Sze added a comment - The new javac warnings are due to the newly deprecated DFSClient.isDirectory(String).
          Hide
          Chris Douglas added a comment -

          I just committed this. Thanks, Nicholas!

          Show
          Chris Douglas added a comment - I just committed this. Thanks, Nicholas!
          Hide
          Hudson added a comment -
          Show
          Hudson added a comment - Integrated in Hadoop-trunk #428 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/428/ )
          Hide
          Robert Chansler added a comment -

          Noted as incompatible in changes.txt

          Show
          Robert Chansler added a comment - Noted as incompatible in changes.txt

            People

            • Assignee:
              Tsz Wo Nicholas Sze
              Reporter:
              Hairong Kuang
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development