Hadoop Common
  1. Hadoop Common
  2. HADOOP-2634

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

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.15.0
    • Fix Version/s: 0.17.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Deprecates exists() from ClientProtocol

      Description

      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?

      1. HADOOP-2634-1.patch
        8 kB
        Lohit Vijayarenu
      2. HADOOP-2634-2.patch
        8 kB
        Lohit Vijayarenu

        Issue Links

          Activity

          Konstantin Shvachko created issue -
          Hide
          Doug Cutting added a comment -

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

          Show
          Doug Cutting added a comment - +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.
          Hide
          Konstantin Shvachko added a comment -
          • 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.

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

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

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

          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.

          Show
          Hairong Kuang added a comment - 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.
          Hairong Kuang made changes -
          Field Original Value New Value
          Link This issue duplicates HADOOP-2470 [ HADOOP-2470 ]
          Konstantin Shvachko made changes -
          Link This issue is related to HADOOP-2566 [ HADOOP-2566 ]
          Hide
          Tsz Wo Nicholas Sze added a comment - - edited

          isDir() will be removed in HADOOP-2470.

          Show
          Tsz Wo Nicholas Sze added a comment - - edited isDir() will be removed in HADOOP-2470 .
          Robert Chansler made changes -
          Fix Version/s 0.17.0 [ 12312913 ]
          Priority Major [ 3 ] Blocker [ 1 ]
          Robert Chansler made changes -
          Assignee lohit vijayarenu [ lohit ]
          Hide
          Lohit Vijayarenu added a comment -

          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.

          Show
          Lohit Vijayarenu added a comment - 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.
          Lohit Vijayarenu made changes -
          Attachment HADOOP-2634-1.patch [ 12379012 ]
          Lohit Vijayarenu made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

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

          Show
          Hadoop QA added a comment - -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.
          Hide
          Lohit Vijayarenu added a comment -

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

          Show
          Lohit Vijayarenu added a comment - This patch deprecates exists() in ClientProtocol so does not include any testcase.
          Hide
          Konstantin Shvachko added a comment -
          1. 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;
            
          2. Same in DFSClient.exists().

          The rest looks good. +1

          Show
          Konstantin Shvachko added a comment - 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
          Hide
          Lohit Vijayarenu added a comment -

          Thanks Konstantine. Attaching new patch handling this case.

          Show
          Lohit Vijayarenu added a comment - Thanks Konstantine. Attaching new patch handling this case.
          Lohit Vijayarenu made changes -
          Attachment HADOOP-2634-2.patch [ 12379203 ]
          Lohit Vijayarenu made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Lohit Vijayarenu made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12379203/HADOOP-2634-2.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/2131/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2131/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2131/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2131/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/12379203/HADOOP-2634-2.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/2131/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2131/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2131/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2131/console This message is automatically generated.
          Lohit Vijayarenu made changes -
          Link This issue relates to HADOOP-3160 [ HADOOP-3160 ]
          Hide
          Chris Douglas added a comment -

          I just committed this. Thanks, Lohit!

          Show
          Chris Douglas added a comment - I just committed this. Thanks, Lohit!
          Chris Douglas made changes -
          Hadoop Flags [Incompatible change, Reviewed]
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -
          Show
          Hudson added a comment - Integrated in Hadoop-trunk #450 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/450/ )
          Lohit Vijayarenu made changes -
          Hadoop Flags [Reviewed, Incompatible change] [Incompatible change, Reviewed]
          Release Note Deprecates exists() from ClientProtocol
          Nigel Daley made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Owen O'Malley made changes -
          Component/s dfs [ 12310710 ]

            People

            • Assignee:
              Lohit Vijayarenu
              Reporter:
              Konstantin Shvachko
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development