Hadoop Common
  1. Hadoop Common
  2. HADOOP-7322

Adding a util method in FileUtil for JDK File.listFiles

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.23.0
    • Component/s: None
    • Labels:
      None
    • Release Note:
      Use of this new utility method avoids null result from File.listFiles(), and consequent NPEs.

      Description

      While testing Disk Fail Inplace, we encountered lots of NPE from Dir.listFiles API. This API can return null when Dir is not directory or disk is bad. I am proposing to have a File Util which can be used consistently across to deal with disk issues. This util api will do the following:

      1. When error happens it will throw IOException
      2. Else it will return empty list or list of files.

      Signature:
      File[] FileUtil.listFiles(File dir) throws IOException {}

      This way we no need to write wrapper code every where. Also, API is consistent with the signature.

      1. HADOOP-7322-1.patch
        4 kB
        Bharath Mundlapudi
      2. HADOOP-7322-2.patch
        3 kB
        Bharath Mundlapudi
      3. HADOOP-7322-3.patch
        4 kB
        Bharath Mundlapudi

        Issue Links

          Activity

          Bharath Mundlapudi created issue -
          Bharath Mundlapudi made changes -
          Field Original Value New Value
          Link This issue is related to HADOOP-7125 [ HADOOP-7125 ]
          Bharath Mundlapudi made changes -
          Attachment HADOOP-7322-1.patch [ 12480313 ]
          Hide
          Matt Foley added a comment -

          Hi Bharath,
          1. suggest javadoc comment for FileUtil.listFiles() to read:
          + * A wrapper for

          {@link File#listFiles()}

          . This java.io API returns null
          + * when dir is not directory or for any I/O error. Instead of having to
          + * null check everywhere File#listFiles() is used, we will add this utility
          + * api to get around this problem, for the majority of cases where we prefer
          + * an IOException to be thrown.

          2. Not sure about the change to RunJar.main(). Isn't jar extraction usually pretty forgiving? It is currently written to skip any directory it can't read. Can you please give an argument for why that's wrong?

          3. In the last line of testlistFiles(), to assure that the referenced directory can't possibly exist, why not use the same name as the directory you just deleted in the previous line? Maybe after asserting:
          Assert.assertTrue("Failed to delete test dir", !newDir.exists());

          Show
          Matt Foley added a comment - Hi Bharath, 1. suggest javadoc comment for FileUtil.listFiles() to read: + * A wrapper for {@link File#listFiles()} . This java.io API returns null + * when dir is not directory or for any I/O error. Instead of having to + * null check everywhere File#listFiles() is used, we will add this utility + * api to get around this problem, for the majority of cases where we prefer + * an IOException to be thrown. 2. Not sure about the change to RunJar.main(). Isn't jar extraction usually pretty forgiving? It is currently written to skip any directory it can't read. Can you please give an argument for why that's wrong? 3. In the last line of testlistFiles(), to assure that the referenced directory can't possibly exist, why not use the same name as the directory you just deleted in the previous line? Maybe after asserting: Assert.assertTrue("Failed to delete test dir", !newDir.exists());
          Hide
          Bharath Mundlapudi added a comment -

          Thanks for the comments, Matt.

          Regarding 2: Yes, we can be forgiving on this case. I added this because of eliminating null check.

          Attaching a patch which addresses all these comments.

          Show
          Bharath Mundlapudi added a comment - Thanks for the comments, Matt. Regarding 2: Yes, we can be forgiving on this case. I added this because of eliminating null check. Attaching a patch which addresses all these comments.
          Bharath Mundlapudi made changes -
          Attachment HADOOP-7322-2.patch [ 12480342 ]
          Hide
          Matt Foley added a comment -

          Thanks, Bharath. Unfortunately I have one more item. Looking more at testlistFiles(), there are many calls that could cause an IOException, but only the last one should be an allowed IOException. Therefore, I think we have to remove the "expected=IOException.class" from the @Test annotation, and instead use the idiom:

          try {
            files = FileUtil.listFiles(newDir);
            fail("IOException expected on listFiles() for non-existent dir " + newDir.getString());
          } catch (IOException ioe) {
            //expected
          }
          
          Show
          Matt Foley added a comment - Thanks, Bharath. Unfortunately I have one more item. Looking more at testlistFiles(), there are many calls that could cause an IOException, but only the last one should be an allowed IOException. Therefore, I think we have to remove the "expected=IOException.class" from the @Test annotation, and instead use the idiom: try { files = FileUtil.listFiles(newDir); fail( "IOException expected on listFiles() for non-existent dir " + newDir.getString()); } catch (IOException ioe) { //expected }
          Hide
          Bharath Mundlapudi added a comment -

          Not a problem. I have updated the testcase.

          Show
          Bharath Mundlapudi added a comment - Not a problem. I have updated the testcase.
          Bharath Mundlapudi made changes -
          Attachment HADOOP-7322-3.patch [ 12480373 ]
          Bharath Mundlapudi 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/12480373/HADOOP-7322-3.patch
          against trunk revision 1127215.

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

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

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

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/523//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/523//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/523//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/12480373/HADOOP-7322-3.patch against trunk revision 1127215. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/523//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/523//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HADOOP-Build/523//console This message is automatically generated.
          Hide
          Matt Foley added a comment -

          +1. Committed to trunk. Thanks, Bharath!

          Show
          Matt Foley added a comment - +1. Committed to trunk. Thanks, Bharath!
          Matt Foley made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Release Note Use of this new utility method avoids null result from File.listFiles(), and consequent NPEs.
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #622 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/622/)
          HADOOP-7322. Adding a util method in FileUtil for directory listing, avoid NPEs on File.listFiles(). Contributed by Bharath Mundlapudi.

          mattf : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1127697
          Files :

          • /hadoop/common/trunk/CHANGES.txt
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/RawLocalFileSystem.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FileUtil.java
          • /hadoop/common/trunk/src/test/core/org/apache/hadoop/fs/TestFileUtil.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #622 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/622/ ) HADOOP-7322 . Adding a util method in FileUtil for directory listing, avoid NPEs on File.listFiles(). Contributed by Bharath Mundlapudi. mattf : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1127697 Files : /hadoop/common/trunk/CHANGES.txt /hadoop/common/trunk/src/java/org/apache/hadoop/fs/RawLocalFileSystem.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FileUtil.java /hadoop/common/trunk/src/test/core/org/apache/hadoop/fs/TestFileUtil.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #700 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/700/)
          HADOOP-7322. Adding a util method in FileUtil for directory listing, avoid NPEs on File.listFiles(). Contributed by Bharath Mundlapudi.

          mattf : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1127697
          Files :

          • /hadoop/common/trunk/CHANGES.txt
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/RawLocalFileSystem.java
          • /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FileUtil.java
          • /hadoop/common/trunk/src/test/core/org/apache/hadoop/fs/TestFileUtil.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #700 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/700/ ) HADOOP-7322 . Adding a util method in FileUtil for directory listing, avoid NPEs on File.listFiles(). Contributed by Bharath Mundlapudi. mattf : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1127697 Files : /hadoop/common/trunk/CHANGES.txt /hadoop/common/trunk/src/java/org/apache/hadoop/fs/RawLocalFileSystem.java /hadoop/common/trunk/src/java/org/apache/hadoop/fs/FileUtil.java /hadoop/common/trunk/src/test/core/org/apache/hadoop/fs/TestFileUtil.java
          Matt Foley made changes -
          Summary Adding a util method in FileUtil for directory listing Adding a util method in FileUtil for JDK File.listFiles
          Matt Foley made changes -
          Link This issue blocks HDFS-1934 [ HDFS-1934 ]
          Matt Foley made changes -
          Link This issue blocks HDFS-2023 [ HDFS-2023 ]
          Arun C Murthy made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Gavin made changes -
          Link This issue blocks HDFS-2023 [ HDFS-2023 ]
          Gavin made changes -
          Link This issue is depended upon by HDFS-2023 [ HDFS-2023 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          1d 9h 38m 1 Bharath Mundlapudi 25/May/11 08:34
          Patch Available Patch Available Resolved Resolved
          14h 21m 1 Matt Foley 25/May/11 22:55
          Resolved Resolved Closed Closed
          173d 2h 54m 1 Arun C Murthy 15/Nov/11 00:50

            People

            • Assignee:
              Bharath Mundlapudi
              Reporter:
              Bharath Mundlapudi
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development