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

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development