Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-8576

Lease recovery should return true if the lease can be released and the file can be closed

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.8.0, 2.7.1, 3.0.0-alpha1
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      FSNamesystem#recoverLease , returns false eventhough lease recover happens.
      Hence only on second retry for recovering lease on a file ,returns success after checking if the file is not underconstruction.

      1. HDFS-8576.1.patch
        6 kB
        J.Andreina
      2. HDFS-8576.2.patch
        6 kB
        J.Andreina

        Activity

        Hide
        szetszwo Tsz Wo Nicholas Sze added a comment -

        The javadoc says "@return true if the file is already closed".

        What is the expected behavior in your mind?

        Show
        szetszwo Tsz Wo Nicholas Sze added a comment - The javadoc says "@return true if the file is already closed". What is the expected behavior in your mind?
        Hide
        andreina J.Andreina added a comment -

        Thanks Tsz Wo Nicholas Sze for having a look at this issue.

        My expectation on invoking recoverlease is to return true when:
        1. File is already closed
        2. Lease Recovery is successful and file is closed.

        Say consider following scenario
        Step 1: Client1 created a file with 0 byte or file with blocks in finalized. Client1 is aborted
        Step 2: Explicit Lease recovery is triggered.

        As per current implementation , lease recovery will be successful on first try , but still it returns false.

        Rex@XXXXXXXXX:~/hadoop/bin>./hdfs dfs -appendToFile hadoop /Test_file1
        appendToFile: Failed to APPEND_FILE /Test_file1 for DFSClient_NONMAPREDUCE_-899478418_1 on XXXXXXXXX because this file lease is currently owned by DFSClient_NONMAPREDUCE_1903738807_1 on YYYYYYYYYYYYY
        
        Rex@XXXXXXXXX:~/hadoop/bin> ./hdfs debug recoverLease -path /Test_file1
        recoverLease returned false.
        Giving up on recoverLease for /Test_file1 after 1 try.
        
        Rex@XXXXXXXXX:~/hadoop/bin> echo $?
        1
        Rex@XXXXXXXXX:~/hadoop/bin> ./hdfs dfs -appendToFile hadoop /Test_file1
        Rex@XXXXXXXXX:~/hadoop/bin> echo $?
        0
        

        Expected is to return successful on first try. Attached an initial patch for the same.
        Please review and give your feedback.

        Show
        andreina J.Andreina added a comment - Thanks Tsz Wo Nicholas Sze for having a look at this issue. My expectation on invoking recoverlease is to return true when: 1. File is already closed 2. Lease Recovery is successful and file is closed. Say consider following scenario Step 1: Client1 created a file with 0 byte or file with blocks in finalized. Client1 is aborted Step 2: Explicit Lease recovery is triggered. As per current implementation , lease recovery will be successful on first try , but still it returns false. Rex@XXXXXXXXX:~/hadoop/bin>./hdfs dfs -appendToFile hadoop /Test_file1 appendToFile: Failed to APPEND_FILE /Test_file1 for DFSClient_NONMAPREDUCE_-899478418_1 on XXXXXXXXX because this file lease is currently owned by DFSClient_NONMAPREDUCE_1903738807_1 on YYYYYYYYYYYYY Rex@XXXXXXXXX:~/hadoop/bin> ./hdfs debug recoverLease -path /Test_file1 recoverLease returned false . Giving up on recoverLease for /Test_file1 after 1 try . Rex@XXXXXXXXX:~/hadoop/bin> echo $? 1 Rex@XXXXXXXXX:~/hadoop/bin> ./hdfs dfs -appendToFile hadoop /Test_file1 Rex@XXXXXXXXX:~/hadoop/bin> echo $? 0 Expected is to return successful on first try. Attached an initial patch for the same. Please review and give your feedback.
        Hide
        szetszwo Tsz Wo Nicholas Sze added a comment -

        I see your point now after checking the patch in more details. The summary "Lease recovery returns false , eventhough recovery happens" is missing leading since there is no lease recovery in this case, it only releases the lease.

        Some minor comments on the patch:

        • The javadoc is inaccurate as mentioned previously. Let's change it to "if the file is already closed, or if the lease can be released and the file can be closed."
          +   * @return true if the file is already closed or if lease recovery
          +   *         is successful and file is closed.
          
        • Rename recoverLeaseInternal to isClosed and declare it as final in recoverLease.
        • Let's change the following
                    boolean isClosed = internalReleaseLease(lease, src, iip, null);
                    if(!isClosed)
                      throw new RecoveryInProgressException(
                          op.getExceptionMessage(src, holder, clientMachine,
                              "lease recovery is in progress. Try again later."));
                    return isClosed;
          

          to

                    if (internalReleaseLease(lease, src, iip, null)) {
                      return true;
                    } else {
                      throw new RecoveryInProgressException(
                          op.getExceptionMessage(src, holder, clientMachine,
                              "lease recovery is in progress. Try again later."));
                    }
          
        • asFile() won't return null so that we can change the if-statement in recoverLeaseInternal to:
              INodeFile file = iip.getLastINode().asFile();
              if (!file.isUnderConstruction()) {
                return true;
              } else {
                ...
              }
          

          Notice that we don't need "return false" at the end.

        Show
        szetszwo Tsz Wo Nicholas Sze added a comment - I see your point now after checking the patch in more details. The summary "Lease recovery returns false , eventhough recovery happens" is missing leading since there is no lease recovery in this case, it only releases the lease. Some minor comments on the patch: The javadoc is inaccurate as mentioned previously. Let's change it to "if the file is already closed, or if the lease can be released and the file can be closed." + * @ return true if the file is already closed or if lease recovery + * is successful and file is closed. Rename recoverLeaseInternal to isClosed and declare it as final in recoverLease. Let's change the following boolean isClosed = internalReleaseLease(lease, src, iip, null ); if (!isClosed) throw new RecoveryInProgressException( op.getExceptionMessage(src, holder, clientMachine, "lease recovery is in progress. Try again later." )); return isClosed; to if (internalReleaseLease(lease, src, iip, null )) { return true ; } else { throw new RecoveryInProgressException( op.getExceptionMessage(src, holder, clientMachine, "lease recovery is in progress. Try again later." )); } asFile() won't return null so that we can change the if-statement in recoverLeaseInternal to: INodeFile file = iip.getLastINode().asFile(); if (!file.isUnderConstruction()) { return true ; } else { ... } Notice that we don't need "return false" at the end.
        Hide
        andreina J.Andreina added a comment -

        Thanks Tsz Wo Nicholas Sze , I have updated the patch fixing your review comments.
        Please review.

        Show
        andreina J.Andreina added a comment - Thanks Tsz Wo Nicholas Sze , I have updated the patch fixing your review comments. Please review.
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 18m 38s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 tests included 0m 0s The patch appears to include 1 new or modified test files.
        +1 javac 7m 41s There were no new javac warning messages.
        +1 javadoc 9m 49s There were no new javadoc warning messages.
        +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
        -1 checkstyle 2m 17s The applied patch generated 2 new checkstyle issues (total was 262, now 262).
        -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix.
        +1 install 1m 35s mvn install still works.
        +1 eclipse:eclipse 0m 34s The patch built with eclipse:eclipse.
        +1 findbugs 3m 14s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
        +1 native 3m 18s Pre-build of native portion
        +1 hdfs tests 162m 34s Tests passed in hadoop-hdfs.
            210m 6s  



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12739544/HDFS-8576.2.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / 4c5da9b
        checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/11352/artifact/patchprocess/diffcheckstylehadoop-hdfs.txt
        whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/11352/artifact/patchprocess/whitespace.txt
        hadoop-hdfs test log https://builds.apache.org/job/PreCommit-HDFS-Build/11352/artifact/patchprocess/testrun_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/11352/testReport/
        Java 1.7.0_55
        uname Linux asf904.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/11352/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 18m 38s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 1 new or modified test files. +1 javac 7m 41s There were no new javac warning messages. +1 javadoc 9m 49s There were no new javadoc warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 2m 17s The applied patch generated 2 new checkstyle issues (total was 262, now 262). -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 35s mvn install still works. +1 eclipse:eclipse 0m 34s The patch built with eclipse:eclipse. +1 findbugs 3m 14s The patch does not introduce any new Findbugs (version 3.0.0) warnings. +1 native 3m 18s Pre-build of native portion +1 hdfs tests 162m 34s Tests passed in hadoop-hdfs.     210m 6s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12739544/HDFS-8576.2.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 4c5da9b checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/11352/artifact/patchprocess/diffcheckstylehadoop-hdfs.txt whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/11352/artifact/patchprocess/whitespace.txt hadoop-hdfs test log https://builds.apache.org/job/PreCommit-HDFS-Build/11352/artifact/patchprocess/testrun_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/11352/testReport/ Java 1.7.0_55 uname Linux asf904.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-HDFS-Build/11352/console This message was automatically generated.
        Hide
        szetszwo Tsz Wo Nicholas Sze added a comment -

        +1 the new patch looks good.

        (revised Summary)

        Show
        szetszwo Tsz Wo Nicholas Sze added a comment - +1 the new patch looks good. (revised Summary)
        Hide
        szetszwo Tsz Wo Nicholas Sze added a comment -

        I have committed this. Thanks, J.Andreina!

        Show
        szetszwo Tsz Wo Nicholas Sze added a comment - I have committed this. Thanks, J.Andreina!
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #8019 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8019/)
        HDFS-8576. Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #8019 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8019/ ) HDFS-8576 . Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        andreina J.Andreina added a comment -

        Thanks Tsz Wo Nicholas Sze for the commit.

        Show
        andreina J.Andreina added a comment - Thanks Tsz Wo Nicholas Sze for the commit.
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk #960 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/960/)
        HDFS-8576. Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #960 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/960/ ) HDFS-8576 . Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #230 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/230/)
        HDFS-8576. Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #230 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/230/ ) HDFS-8576 . Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #2158 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2158/)
        HDFS-8576. Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2158 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2158/ ) HDFS-8576 . Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #219 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/219/)
        HDFS-8576. Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a)

        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #219 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/219/ ) HDFS-8576 . Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #228 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/228/)
        HDFS-8576. Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #228 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/228/ ) HDFS-8576 . Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk #2176 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2176/)
        HDFS-8576. Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a)

        • hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #2176 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2176/ ) HDFS-8576 . Lease recovery should return true if the lease can be released and the file can be closed. Contributed by J.Andreina (szetszwo: rev 2cb09e98e392feb5732d0754b539240094edc37a) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestLeaseRecovery.java hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java

          People

          • Assignee:
            andreina J.Andreina
            Reporter:
            andreina J.Andreina
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development