Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9.0, 3.0.0-beta1
    • Component/s: fs/azure
    • Labels:
      None
    • Target Version/s:
    • Release Note:
      Recursive directory delete improvement for the wasb filesystem.

      Description

      FileSystem.delete(Path path) and delete(Path path, boolean recursive) return false if the path does not exist. The WASB implementation of recursive delete currently fails if one of the entries is deleted by an external agent while a recursive delete is in progress. For example, if you try to delete all of the files in a directory, which can be a very long process, and one of the files contained within is deleted by an external agent, the recursive directory delete operation will fail if it tries to delete that file and discovers that it does not exist. This is not desirable. A recursive directory delete operation should succeeed if the directory initially exists and when the operation completes, the directory and all of its entries do not exist.

      1. HADOOP-14769-001.patch
        9 kB
        Thomas Marquardt
      2. HADOOP-14769-002.patch
        9 kB
        Thomas Marquardt

        Activity

        Hide
        tmarquardt Thomas Marquardt added a comment -

        Attaching HADOOP-14769-001.patch.

        This fixes recursive directory delete so that it will return false as expected if the directory does not exist, but it will return true if the directory is successfully deleted even if one of the path entries existed initially but did not exist when an attempt was made to delete it. Two new test cases have been added. One simulates a file being deleted by an external agent and the other a child directory. One of the existing test cases was removed, as it verified that recursive directory delete failed if a child directory was deleted by an external agent--this test actually had a bug in it and the child directory did exist.

        All tests passing against my tmarql3 account:

        Tests run: 775, Failures: 0, Errors: 0, Skipped: 155

        Show
        tmarquardt Thomas Marquardt added a comment - Attaching HADOOP-14769 -001.patch. This fixes recursive directory delete so that it will return false as expected if the directory does not exist, but it will return true if the directory is successfully deleted even if one of the path entries existed initially but did not exist when an attempt was made to delete it. Two new test cases have been added. One simulates a file being deleted by an external agent and the other a child directory. One of the existing test cases was removed, as it verified that recursive directory delete failed if a child directory was deleted by an external agent--this test actually had a bug in it and the child directory did exist. All tests passing against my tmarql3 account: Tests run: 775, Failures: 0, Errors: 0, Skipped: 155
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 2m 26s Docker mode activated.
              Prechecks
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
              trunk Compile Tests
        +1 mvninstall 13m 42s trunk passed
        +1 compile 0m 19s trunk passed
        +1 checkstyle 0m 13s trunk passed
        +1 mvnsite 0m 21s trunk passed
        +1 findbugs 0m 27s trunk passed
        +1 javadoc 0m 14s trunk passed
              Patch Compile Tests
        +1 mvninstall 0m 17s the patch passed
        +1 compile 0m 17s the patch passed
        +1 javac 0m 17s the patch passed
        +1 checkstyle 0m 11s the patch passed
        +1 mvnsite 0m 18s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 0m 34s the patch passed
        +1 javadoc 0m 11s the patch passed
              Other Tests
        +1 unit 2m 1s hadoop-azure in the patch passed.
        +1 asflicense 0m 13s The patch does not generate ASF License warnings.
        23m 0s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:14b5c93
        JIRA Issue HADOOP-14769
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881686/HADOOP-14769-001.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux b256c5a483a9 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / d8f74c3
        Default Java 1.8.0_144
        findbugs v3.1.0-RC1
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13028/testReport/
        modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13028/console
        Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 2m 26s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 13m 42s trunk passed +1 compile 0m 19s trunk passed +1 checkstyle 0m 13s trunk passed +1 mvnsite 0m 21s trunk passed +1 findbugs 0m 27s trunk passed +1 javadoc 0m 14s trunk passed       Patch Compile Tests +1 mvninstall 0m 17s the patch passed +1 compile 0m 17s the patch passed +1 javac 0m 17s the patch passed +1 checkstyle 0m 11s the patch passed +1 mvnsite 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 34s the patch passed +1 javadoc 0m 11s the patch passed       Other Tests +1 unit 2m 1s hadoop-azure in the patch passed. +1 asflicense 0m 13s The patch does not generate ASF License warnings. 23m 0s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14769 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881686/HADOOP-14769-001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux b256c5a483a9 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / d8f74c3 Default Java 1.8.0_144 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13028/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13028/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        tmarquardt Thomas Marquardt added a comment -

        This patch fixes issues encountered with HBASE where it calls WASB and WASB fails to delete a directory recursively.

        I think we agree that delete recursive should return true if 1) the directory exists and 2) the directory and its content is successfully deleted. It should return true when those conditions are met, even if one of the child entries did not exist when an attempt was made to delete it.

        Show
        tmarquardt Thomas Marquardt added a comment - This patch fixes issues encountered with HBASE where it calls WASB and WASB fails to delete a directory recursively. I think we agree that delete recursive should return true if 1) the directory exists and 2) the directory and its content is successfully deleted. It should return true when those conditions are met, even if one of the child entries did not exist when an attempt was made to delete it.
        Hide
        esmanii Esfandiar Manii added a comment -

        +1 with few comments:
        AzureNativeFileSystemStore.java L2503-2505: Not sure how much we want to invest on this but there are many of this code everywhere, I wish there was only one method doing this.
        NativeAzureFileSystem.java L2099-2108: instead of nested ifs please rewrite it to be like (for better code clarity):
        if (!store.delete(path)) {
        return false;
        }

        if (isDir) {
        }
        else {
        }

        return true;
        TestFileSystemOperationsWithThreads.java L592-594: nit: Please fix indentation

        Show
        esmanii Esfandiar Manii added a comment - +1 with few comments: AzureNativeFileSystemStore.java L2503-2505: Not sure how much we want to invest on this but there are many of this code everywhere, I wish there was only one method doing this. NativeAzureFileSystem.java L2099-2108: instead of nested ifs please rewrite it to be like (for better code clarity): if (!store.delete(path)) { return false; } if (isDir) { } else { } return true; TestFileSystemOperationsWithThreads.java L592-594: nit: Please fix indentation
        Hide
        shanem Shane Mainali added a comment -

        +1. Thanks Thomas Marquardt!

        Show
        shanem Shane Mainali added a comment - +1. Thanks Thomas Marquardt !
        Hide
        tmarquardt Thomas Marquardt added a comment -

        Attaching HADOOP-14769-002.patch. This is only a style change.

        Thanks for feedback.

        1) I will not refactor AzureNativeFileSystemStore.java L2503-2505 at this time. This is a targeted fix for recursive delete.

        2) I removed the nesting at NativeAzureFileSystem.java L2099-2108.

        3) The indentation at TestFileSystemOperationsWithThreads.java L592-594 is one of the recommended styles. Anyhow, I changed it to be a single line break.

        All tests are passing with my tmarql3 endpoint:

        Tests run: 775, Failures: 0, Errors: 0, Skipped: 155

        Show
        tmarquardt Thomas Marquardt added a comment - Attaching HADOOP-14769 -002.patch. This is only a style change. Thanks for feedback. 1) I will not refactor AzureNativeFileSystemStore.java L2503-2505 at this time. This is a targeted fix for recursive delete. 2) I removed the nesting at NativeAzureFileSystem.java L2099-2108. 3) The indentation at TestFileSystemOperationsWithThreads.java L592-594 is one of the recommended styles. Anyhow, I changed it to be a single line break. All tests are passing with my tmarql3 endpoint: Tests run: 775, Failures: 0, Errors: 0, Skipped: 155
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 25s Docker mode activated.
              Prechecks
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
              trunk Compile Tests
        +1 mvninstall 13m 57s trunk passed
        +1 compile 0m 19s trunk passed
        +1 checkstyle 0m 14s trunk passed
        +1 mvnsite 0m 21s trunk passed
        +1 findbugs 0m 28s trunk passed
        +1 javadoc 0m 14s trunk passed
              Patch Compile Tests
        +1 mvninstall 0m 18s the patch passed
        +1 compile 0m 17s the patch passed
        +1 javac 0m 17s the patch passed
        +1 checkstyle 0m 11s the patch passed
        +1 mvnsite 0m 18s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 0m 33s the patch passed
        +1 javadoc 0m 11s the patch passed
              Other Tests
        +1 unit 2m 0s hadoop-azure in the patch passed.
        +1 asflicense 0m 14s The patch does not generate ASF License warnings.
        21m 14s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:14b5c93
        JIRA Issue HADOOP-14769
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882191/HADOOP-14769-002.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 6dba92aba7b9 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / de462da
        Default Java 1.8.0_144
        findbugs v3.1.0-RC1
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13051/testReport/
        modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13051/console
        Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 25s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 13m 57s trunk passed +1 compile 0m 19s trunk passed +1 checkstyle 0m 14s trunk passed +1 mvnsite 0m 21s trunk passed +1 findbugs 0m 28s trunk passed +1 javadoc 0m 14s trunk passed       Patch Compile Tests +1 mvninstall 0m 18s the patch passed +1 compile 0m 17s the patch passed +1 javac 0m 17s the patch passed +1 checkstyle 0m 11s the patch passed +1 mvnsite 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 33s the patch passed +1 javadoc 0m 11s the patch passed       Other Tests +1 unit 2m 0s hadoop-azure in the patch passed. +1 asflicense 0m 14s The patch does not generate ASF License warnings. 21m 14s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HADOOP-14769 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12882191/HADOOP-14769-002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 6dba92aba7b9 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / de462da Default Java 1.8.0_144 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/13051/testReport/ modules C: hadoop-tools/hadoop-azure U: hadoop-tools/hadoop-azure Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/13051/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        stevel@apache.org Steve Loughran added a comment -

        +1
        committed to branch-2 & trunk. Thanks

        Show
        stevel@apache.org Steve Loughran added a comment - +1 committed to branch-2 & trunk. Thanks
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #12210 (See https://builds.apache.org/job/Hadoop-trunk-Commit/12210/)
        HADOOP-14769. WASB: delete recursive should not fail if a file is (stevel: rev c6b4e656b76b68cc1d0dbcc15a5aa5ea23335b7b)

        • (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
        • (edit) hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestFileSystemOperationsWithThreads.java
        • (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #12210 (See https://builds.apache.org/job/Hadoop-trunk-Commit/12210/ ) HADOOP-14769 . WASB: delete recursive should not fail if a file is (stevel: rev c6b4e656b76b68cc1d0dbcc15a5aa5ea23335b7b) (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java (edit) hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestFileSystemOperationsWithThreads.java (edit) hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/AzureNativeFileSystemStore.java

          People

          • Assignee:
            tmarquardt Thomas Marquardt
            Reporter:
            tmarquardt Thomas Marquardt
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development