Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-13249

RetryInvocationHandler need wrap InterruptedException in IOException when call Thread.sleep

    Details

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

      Description

      RetryInvocationHandler need wrap InterruptedException in IOException when call Thread.sleep. Otherwise InterruptedException can't be handled correctly by other components such as HDFS.

      1. HADOOP-13249.002.patch
        3 kB
        zhihai xu
      2. HADOOP-13249.001.patch
        1 kB
        zhihai xu
      3. HADOOP-13249.000.patch
        1.0 kB
        zhihai xu

        Activity

        Hide
        jingzhao Jing Zhao added a comment -

        Thanks zhihai xu for working on this.

        Otherwise InterruptedException can't be handled correctly by other components such as HDFS.

        Could you please elaborate more about how HDFS fails to handle the InterruptException correctly?

        Show
        jingzhao Jing Zhao added a comment - Thanks zhihai xu for working on this. Otherwise InterruptedException can't be handled correctly by other components such as HDFS. Could you please elaborate more about how HDFS fails to handle the InterruptException correctly?
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 24s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 mvninstall 6m 53s trunk passed
        +1 compile 7m 6s trunk passed
        +1 checkstyle 0m 23s trunk passed
        +1 mvnsite 1m 2s trunk passed
        +1 mvneclipse 0m 11s trunk passed
        +1 findbugs 1m 22s trunk passed
        +1 javadoc 0m 56s trunk passed
        +1 mvninstall 0m 43s the patch passed
        +1 compile 7m 19s the patch passed
        +1 javac 7m 19s the patch passed
        +1 checkstyle 0m 23s the patch passed
        +1 mvnsite 0m 54s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 34s the patch passed
        +1 javadoc 0m 56s the patch passed
        -1 unit 8m 5s hadoop-common in the patch failed.
        +1 asflicense 0m 21s The patch does not generate ASF License warnings.
        39m 25s



        Reason Tests
        Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics
          hadoop.io.retry.TestRetryProxy
          hadoop.net.TestDNS



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:2c91fd8
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12809035/HADOOP-13249.000.patch
        JIRA Issue HADOOP-13249
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 61f610d1604c 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
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / ae04765
        Default Java 1.8.0_91
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9695/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9695/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9695/console
        Powered by Apache Yetus 0.4.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 24s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 6m 53s trunk passed +1 compile 7m 6s trunk passed +1 checkstyle 0m 23s trunk passed +1 mvnsite 1m 2s trunk passed +1 mvneclipse 0m 11s trunk passed +1 findbugs 1m 22s trunk passed +1 javadoc 0m 56s trunk passed +1 mvninstall 0m 43s the patch passed +1 compile 7m 19s the patch passed +1 javac 7m 19s the patch passed +1 checkstyle 0m 23s the patch passed +1 mvnsite 0m 54s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 34s the patch passed +1 javadoc 0m 56s the patch passed -1 unit 8m 5s hadoop-common in the patch failed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 39m 25s Reason Tests Failed junit tests hadoop.metrics2.impl.TestGangliaMetrics   hadoop.io.retry.TestRetryProxy   hadoop.net.TestDNS Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12809035/HADOOP-13249.000.patch JIRA Issue HADOOP-13249 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 61f610d1604c 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 Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / ae04765 Default Java 1.8.0_91 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9695/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9695/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9695/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        stevel@apache.org Steve Loughran added a comment -

        It looks like the problem is that if the thread sleeping for a retry is interrupted, it's thrown all the way up and things expecting IOEs don't like that.

        Wrapping would fix that, but it MUST be an InterruptedIOException, so that the fact there wasn't interrupt doesn't get lost in the chain.

        However: is it that the right action? What about rethrowing the exception that was being retried?

        Show
        stevel@apache.org Steve Loughran added a comment - It looks like the problem is that if the thread sleeping for a retry is interrupted, it's thrown all the way up and things expecting IOEs don't like that. Wrapping would fix that, but it MUST be an InterruptedIOException , so that the fact there wasn't interrupt doesn't get lost in the chain. However: is it that the right action? What about rethrowing the exception that was being retried?
        Hide
        jingzhao Jing Zhao added a comment -

        +1 for throwing InterruptedIOException.

        Currently if the interruption happens in the RetryInvocationHandler level, the thread that is making the RPC call (and also is blocked in the RPC call) gets interrupted (instead of a worker thread inside of the RPC Client.java). Thus looks like the upper level caller aims to interrupt the RPC invocation. So I think throwing InterruptedIOException may be better than throwing the exception that was being retried.

        Show
        jingzhao Jing Zhao added a comment - +1 for throwing InterruptedIOException . Currently if the interruption happens in the RetryInvocationHandler level, the thread that is making the RPC call (and also is blocked in the RPC call) gets interrupted (instead of a worker thread inside of the RPC Client.java). Thus looks like the upper level caller aims to interrupt the RPC invocation. So I think throwing InterruptedIOException may be better than throwing the exception that was being retried.
        Hide
        zxu zhihai xu added a comment -

        Thanks for the review, Steve Loughran and Jing Zhao! Yes, throwing InterruptedIOException makes sense to me also. I attached a new patch HADOOP-13249.001.patch which use InterruptedIOException instead of IOException, Please review it. thanks.

        Show
        zxu zhihai xu added a comment - Thanks for the review, Steve Loughran and Jing Zhao ! Yes, throwing InterruptedIOException makes sense to me also. I attached a new patch HADOOP-13249 .001.patch which use InterruptedIOException instead of IOException, Please review it. thanks.
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 22s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 mvninstall 6m 38s trunk passed
        +1 compile 7m 25s trunk passed
        +1 checkstyle 0m 26s trunk passed
        +1 mvnsite 1m 8s trunk passed
        +1 mvneclipse 0m 11s trunk passed
        +1 findbugs 1m 25s trunk passed
        +1 javadoc 1m 0s trunk passed
        +1 mvninstall 1m 5s the patch passed
        +1 compile 10m 19s the patch passed
        +1 javac 10m 19s the patch passed
        +1 checkstyle 0m 45s the patch passed
        +1 mvnsite 1m 29s the patch passed
        +1 mvneclipse 0m 17s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 2m 31s the patch passed
        +1 javadoc 1m 16s the patch passed
        -1 unit 8m 47s hadoop-common in the patch failed.
        +1 asflicense 0m 20s The patch does not generate ASF License warnings.
        46m 27s



        Reason Tests
        Failed junit tests hadoop.io.retry.TestRetryProxy



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:2c91fd8
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12809072/HADOOP-13249.001.patch
        JIRA Issue HADOOP-13249
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 535069a35456 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
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / ae04765
        Default Java 1.8.0_91
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9696/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9696/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9696/console
        Powered by Apache Yetus 0.4.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 22s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 6m 38s trunk passed +1 compile 7m 25s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 1m 8s trunk passed +1 mvneclipse 0m 11s trunk passed +1 findbugs 1m 25s trunk passed +1 javadoc 1m 0s trunk passed +1 mvninstall 1m 5s the patch passed +1 compile 10m 19s the patch passed +1 javac 10m 19s the patch passed +1 checkstyle 0m 45s the patch passed +1 mvnsite 1m 29s the patch passed +1 mvneclipse 0m 17s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 31s the patch passed +1 javadoc 1m 16s the patch passed -1 unit 8m 47s hadoop-common in the patch failed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 46m 27s Reason Tests Failed junit tests hadoop.io.retry.TestRetryProxy Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12809072/HADOOP-13249.001.patch JIRA Issue HADOOP-13249 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 535069a35456 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 Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / ae04765 Default Java 1.8.0_91 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/9696/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9696/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9696/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        zxu zhihai xu added a comment -

        If InterruptException is triggered, it will be contained in an UndeclaredThrowableException because InterruptException is an undeclared checked exception that is thrown by the invocation handler. Currently UndeclaredThrowableException is not handled by DFSClient and layer–up.

        Show
        zxu zhihai xu added a comment - If InterruptException is triggered, it will be contained in an UndeclaredThrowableException because InterruptException is an undeclared checked exception that is thrown by the invocation handler. Currently UndeclaredThrowableException is not handled by DFSClient and layer–up.
        Hide
        jingzhao Jing Zhao added a comment -

        The patch looks good to me overall. Some minor comments:

        1. The failed unit test is related to the patch
        2. No need to convert InterruptedIOException into IOException since InterruptedIOException is already an IOException.
          306	        throw (IOException)new InterruptedIOException("Retry interrupted"
          307	            ).initCause(e);
          

        Other comments, Steve Loughran?

        Show
        jingzhao Jing Zhao added a comment - The patch looks good to me overall. Some minor comments: The failed unit test is related to the patch No need to convert InterruptedIOException into IOException since InterruptedIOException is already an IOException . 306 throw (IOException) new InterruptedIOException( "Retry interrupted" 307 ).initCause(e); Other comments, Steve Loughran ?
        Hide
        zxu zhihai xu added a comment -

        thanks for the review Jing Zhao! Good comments! I attached a new patch HADOOP-13249.002.patch which addressed all your comments. Please review it!

        Show
        zxu zhihai xu added a comment - thanks for the review Jing Zhao ! Good comments! I attached a new patch HADOOP-13249 .002.patch which addressed all your comments. Please review it!
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 28s Docker mode activated.
        +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.
        +1 mvninstall 7m 23s trunk passed
        +1 compile 9m 29s trunk passed
        +1 checkstyle 0m 26s trunk passed
        +1 mvnsite 1m 9s trunk passed
        +1 mvneclipse 0m 15s trunk passed
        +1 findbugs 1m 49s trunk passed
        +1 javadoc 1m 3s trunk passed
        +1 mvninstall 0m 50s the patch passed
        +1 compile 9m 26s the patch passed
        +1 javac 9m 26s the patch passed
        +1 checkstyle 0m 28s the patch passed
        +1 mvnsite 1m 5s the patch passed
        +1 mvneclipse 0m 14s the patch passed
        -1 whitespace 0m 0s The patch has 20 line(s) that end in whitespace. Use git apply --whitespace=fix.
        +1 findbugs 1m 59s the patch passed
        +1 javadoc 1m 2s the patch passed
        +1 unit 10m 12s hadoop-common in the patch passed.
        +1 asflicense 0m 24s The patch does not generate ASF License warnings.
        48m 35s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:2c91fd8
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12809357/HADOOP-13249.002.patch
        JIRA Issue HADOOP-13249
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 5a17e4d17e74 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
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 9581fb7
        Default Java 1.8.0_91
        findbugs v3.0.0
        whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/9717/artifact/patchprocess/whitespace-eol.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9717/testReport/
        modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9717/console
        Powered by Apache Yetus 0.4.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 28s Docker mode activated. +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. +1 mvninstall 7m 23s trunk passed +1 compile 9m 29s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 1m 9s trunk passed +1 mvneclipse 0m 15s trunk passed +1 findbugs 1m 49s trunk passed +1 javadoc 1m 3s trunk passed +1 mvninstall 0m 50s the patch passed +1 compile 9m 26s the patch passed +1 javac 9m 26s the patch passed +1 checkstyle 0m 28s the patch passed +1 mvnsite 1m 5s the patch passed +1 mvneclipse 0m 14s the patch passed -1 whitespace 0m 0s The patch has 20 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 findbugs 1m 59s the patch passed +1 javadoc 1m 2s the patch passed +1 unit 10m 12s hadoop-common in the patch passed. +1 asflicense 0m 24s The patch does not generate ASF License warnings. 48m 35s Subsystem Report/Notes Docker Image:yetus/hadoop:2c91fd8 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12809357/HADOOP-13249.002.patch JIRA Issue HADOOP-13249 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 5a17e4d17e74 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 Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 9581fb7 Default Java 1.8.0_91 findbugs v3.0.0 whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/9717/artifact/patchprocess/whitespace-eol.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9717/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9717/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        zxu zhihai xu added a comment -

        White space issue is not related to my patch, ./hadoop-build-tools/src/main/resources/META-INF/LICENSE.txt is a generated file.

        Show
        zxu zhihai xu added a comment - White space issue is not related to my patch, ./hadoop-build-tools/src/main/resources/META-INF/LICENSE.txt is a generated file.
        Hide
        jingzhao Jing Zhao added a comment -

        +1. I will commit the patch shortly.

        Show
        jingzhao Jing Zhao added a comment - +1. I will commit the patch shortly.
        Hide
        jingzhao Jing Zhao added a comment -

        Thanks for the contribution, Zhihai! I've committed this into trunk and branch-2. Please see if you want to also include it in 2.8, zhihai xu.

        Show
        jingzhao Jing Zhao added a comment - Thanks for the contribution, Zhihai! I've committed this into trunk and branch-2. Please see if you want to also include it in 2.8, zhihai xu .
        Hide
        zxu zhihai xu added a comment -

        Great, Thanks for reviewing and committing the patch Jing Zhao! Thanks for the review Steve Loughran!

        Show
        zxu zhihai xu added a comment - Great, Thanks for reviewing and committing the patch Jing Zhao ! Thanks for the review Steve Loughran !
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-trunk-Commit #9946 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9946/)
        HADOOP-13249. RetryInvocationHandler need wrap InterruptedException in (jing9: rev 0bbb4ddd793063c87927035969884a34f60f2076)

        • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestRetryProxy.java
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #9946 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9946/ ) HADOOP-13249 . RetryInvocationHandler need wrap InterruptedException in (jing9: rev 0bbb4ddd793063c87927035969884a34f60f2076) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestRetryProxy.java hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
        Hide
        szetszwo Tsz Wo Nicholas Sze added a comment -

        Merged this to branch-2.8.

        Show
        szetszwo Tsz Wo Nicholas Sze added a comment - Merged this to branch-2.8.

          People

          • Assignee:
            zxu zhihai xu
            Reporter:
            zxu zhihai xu
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development