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_20080312.patch
        12 kB
        Tsz Wo Nicholas Sze
      2. 2470_20080310.patch
        9 kB
        Tsz Wo Nicholas Sze

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          83d 21h 44m 1 Tsz Wo Nicholas Sze 12/Mar/08 22:17
          Patch Available Patch Available Resolved Resolved
          1d 4h 33m 1 Chris Douglas 14/Mar/08 02:51
          Resolved Resolved Closed Closed
          68d 17h 14m 1 Nigel Daley 21/May/08 21:05
          Owen O'Malley made changes -
          Component/s dfs [ 12310710 ]
          Nigel Daley made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Tsz Wo Nicholas Sze made changes -
          Release Note Open and isDir were removed from ClientProtocol.
          Robert Chansler made changes -
          Hadoop Flags [Incompatible change]
          Hide
          Robert Chansler added a comment -

          Noted as incompatible in changes.txt

          Show
          Robert Chansler added a comment - Noted as incompatible in changes.txt
          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/ )
          Chris Douglas made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Fix Version/s 0.17.0 [ 12312913 ]
          Resolution Fixed [ 1 ]
          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
          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
          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.
          Tsz Wo Nicholas Sze made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hairong Kuang added a comment -

          +1 The patch looks good.

          Show
          Hairong Kuang added a comment - +1 The patch looks good.
          Tsz Wo Nicholas Sze made changes -
          Attachment 2470_20080312.patch [ 12377714 ]
          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 -

          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.
          Tsz Wo Nicholas Sze made changes -
          Attachment 2470_20080310.patch [ 12377560 ]
          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
          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
          Tsz Wo Nicholas Sze made changes -
          Assignee Tsz Wo (Nicholas), SZE [ szetszwo ]
          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
          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.
          Nigel Daley made changes -
          Fix Version/s 0.16.0 [ 12312740 ]
          Hairong Kuang made changes -
          Field Original Value New Value
          Link This issue is duplicated by HADOOP-2634 [ HADOOP-2634 ]
          Hairong Kuang created issue -

            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