Hadoop Common
  1. Hadoop Common
  2. HADOOP-6531

add FileUtil.fullyDeleteContents(dir) api to delete contents of a directory

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: fs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Added an api FileUtil.fullyDeleteContents(String dir) to delete contents of the directory.

      Description

      Add FileUtil.fullyDeleteContents(dir) api to delete contents of a directory, not directory itself. This will be useful if we want to clear the contents of cwd.

      1. patch-6531.txt
        4 kB
        Amareshwari Sriramadasu
      2. patch-6531-1.txt
        4 kB
        Amareshwari Sriramadasu
      3. patch-6531-2.txt
        4 kB
        Amareshwari Sriramadasu
      4. patch-6531-3.txt
        6 kB
        Amareshwari Sriramadasu

        Issue Links

          Activity

          Hide
          Amareshwari Sriramadasu added a comment -

          Patch adding the api.

          Show
          Amareshwari Sriramadasu added a comment - Patch adding the api.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12434506/patch-6531.txt
          against trunk revision 904975.

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

          +1 tests included. The patch appears to include 3 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 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 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/312/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/312/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/312/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/312/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/12434506/patch-6531.txt against trunk revision 904975. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 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 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/312/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/312/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/312/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/312/console This message is automatically generated.
          Hide
          Amareshwari Sriramadasu added a comment -

          Changed the unit test to use Junit v4 style

          Show
          Amareshwari Sriramadasu added a comment - Changed the unit test to use Junit v4 style
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12434508/patch-6531-1.txt
          against trunk revision 904975.

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

          +1 tests included. The patch appears to include 3 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 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 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/313/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/313/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/313/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/313/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/12434508/patch-6531-1.txt against trunk revision 904975. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 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 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/313/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/313/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/313/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/313/console This message is automatically generated.
          Hide
          Ravi Gummadi added a comment -

          We are not checking the return value of fullyDeleteContents() in fullyDelete() method. But if fullyDeleteContents() returns false, next line "dir.delete()" will be executed extra for all the directories in the heirarchy that we traversed. Though it is harmless with this existing patch, it looks better to have the check here and return false if fullyDeleteContents() returns false.

          Minor nit in testcase: Last 3 assertions in both testcases in TestFileUtil.java are same and can be moved to a separate method (say validateTmpDir() ) to avoid code-duplication.

          Show
          Ravi Gummadi added a comment - We are not checking the return value of fullyDeleteContents() in fullyDelete() method. But if fullyDeleteContents() returns false, next line "dir.delete()" will be executed extra for all the directories in the heirarchy that we traversed. Though it is harmless with this existing patch, it looks better to have the check here and return false if fullyDeleteContents() returns false. Minor nit in testcase: Last 3 assertions in both testcases in TestFileUtil.java are same and can be moved to a separate method (say validateTmpDir() ) to avoid code-duplication.
          Hide
          Amareshwari Sriramadasu added a comment -

          Thanks for the quick review Ravi.
          Patch with comments incorporated.

          Show
          Amareshwari Sriramadasu added a comment - Thanks for the quick review Ravi. Patch with comments incorporated.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12434518/patch-6531-2.txt
          against trunk revision 904975.

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

          +1 tests included. The patch appears to include 3 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 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 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/314/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/314/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/314/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/314/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/12434518/patch-6531-2.txt against trunk revision 904975. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 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 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/314/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/314/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/314/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/314/console This message is automatically generated.
          Hide
          Hemanth Yamijala added a comment -

          I would improve the test cases just a little bit:

          • Please check for return values of FileUtil.fullyDeleteContents and FileUtil.fullyDelete in test case.
          • Also add a failure test case for both fullyDelete and fullyDeleteContents. We can create a new directory for which we turn off write permissions. Then fullyDelete and fullyDeleteContents should both fail and we should get a false return value that can be checked.

          Code changes seem fine to me.

          Show
          Hemanth Yamijala added a comment - I would improve the test cases just a little bit: Please check for return values of FileUtil.fullyDeleteContents and FileUtil.fullyDelete in test case. Also add a failure test case for both fullyDelete and fullyDeleteContents. We can create a new directory for which we turn off write permissions. Then fullyDelete and fullyDeleteContents should both fail and we should get a false return value that can be checked. Code changes seem fine to me.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Can we also fix the javadoc of each of the fullyDelete*() methods to describe their behavior with directories, files and symlinks?

          Show
          Vinod Kumar Vavilapalli added a comment - Can we also fix the javadoc of each of the fullyDelete*() methods to describe their behavior with directories, files and symlinks?
          Hide
          Amareshwari Sriramadasu added a comment -

          Patch modifying the testcase as suggested.

          Can we also fix the javadoc of each of the fullyDelete*() methods to describe their behavior with directories, files and symlinks?

          Created HADOOP-6536 for this.

          Show
          Amareshwari Sriramadasu added a comment - Patch modifying the testcase as suggested. Can we also fix the javadoc of each of the fullyDelete*() methods to describe their behavior with directories, files and symlinks? Created HADOOP-6536 for this.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12434646/patch-6531-3.txt
          against trunk revision 905860.

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

          +1 tests included. The patch appears to include 3 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 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 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/322/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/322/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/322/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/322/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/12434646/patch-6531-3.txt against trunk revision 905860. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 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 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/322/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/322/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/322/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/322/console This message is automatically generated.
          Hide
          Hemanth Yamijala added a comment -

          Seems fine to me. +1.

          I think HADOOP-6536 will result in a fix to code, not just to documentation. Hence, I think it is OK to address it in a separate JIRA.

          Show
          Hemanth Yamijala added a comment - Seems fine to me. +1. I think HADOOP-6536 will result in a fix to code, not just to documentation. Hence, I think it is OK to address it in a separate JIRA.
          Hide
          Hemanth Yamijala added a comment -

          I just committed this to trunk. Thanks, Amareshwari !

          Can you please update the release note with the relevant API added.

          Show
          Hemanth Yamijala added a comment - I just committed this to trunk. Thanks, Amareshwari ! Can you please update the release note with the relevant API added.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #156 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/156/)
          . Enhance FileUtil with an API to delete all contents of a directory. Contributed by Amareshwari Sriramadasu.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #156 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/156/ ) . Enhance FileUtil with an API to delete all contents of a directory. Contributed by Amareshwari Sriramadasu.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #242 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/242/)
          . Enhance FileUtil with an API to delete all contents of a directory. Contributed by Amareshwari Sriramadasu.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #242 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/242/ ) . Enhance FileUtil with an API to delete all contents of a directory. Contributed by Amareshwari Sriramadasu.

            People

            • Assignee:
              Amareshwari Sriramadasu
              Reporter:
              Amareshwari Sriramadasu
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development