Hadoop Common
  1. Hadoop Common
  2. HADOOP-7717

Move handling of concurrent client fail-overs to RetryInvocationHandler

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.23.0, 0.24.0
    • Fix Version/s: 2.0.0-alpha
    • Component/s: ipc
    • Labels:
      None

      Description

      Currently every implementation of a FailoverProxyProvider will need to perform its own synchronization to ensure that multiple concurrent failed calls to a single client proxy object don't result in multiple client fail over events. It would be better to put this logic in RetryInvocationHandler.invoke.

      This is based on feedback provided by Todd in this comment.

      1. HADOOP-7717.patch
        9 kB
        Aaron T. Myers
      2. HADOOP-7717.patch
        10 kB
        Aaron T. Myers

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-0.23-Commit #660 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Commit/660/)
        HADOOP-7717. Merge r1179483 from trunk to 0.23 (Revision 1298080)

        Result = SUCCESS
        suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298080
        Files :

        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-0.23-Commit #660 (See https://builds.apache.org/job/Hadoop-Mapreduce-0.23-Commit/660/ ) HADOOP-7717 . Merge r1179483 from trunk to 0.23 (Revision 1298080) Result = SUCCESS suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298080 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-0.23-Commit #659 (See https://builds.apache.org/job/Hadoop-Common-0.23-Commit/659/)
        HADOOP-7717. Merge r1179483 from trunk to 0.23 (Revision 1298080)

        Result = SUCCESS
        suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298080
        Files :

        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Show
        Hudson added a comment - Integrated in Hadoop-Common-0.23-Commit #659 (See https://builds.apache.org/job/Hadoop-Common-0.23-Commit/659/ ) HADOOP-7717 . Merge r1179483 from trunk to 0.23 (Revision 1298080) Result = SUCCESS suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298080 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-0.23-Commit #650 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Commit/650/)
        HADOOP-7717. Merge r1179483 from trunk to 0.23 (Revision 1298080)

        Result = SUCCESS
        suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298080
        Files :

        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Commit #650 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Commit/650/ ) HADOOP-7717 . Merge r1179483 from trunk to 0.23 (Revision 1298080) Result = SUCCESS suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1298080 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #852 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/852/)
        HADOOP-7717. Move handling of concurrent client fail-overs to RetryInvocationHandler (atm)

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179483
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #852 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/852/ ) HADOOP-7717 . Move handling of concurrent client fail-overs to RetryInvocationHandler (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179483 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #822 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/822/)
        HADOOP-7717. Move handling of concurrent client fail-overs to RetryInvocationHandler (atm)

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179483
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #822 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/822/ ) HADOOP-7717 . Move handling of concurrent client fail-overs to RetryInvocationHandler (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179483 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #1040 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1040/)
        HADOOP-7717. Move handling of concurrent client fail-overs to RetryInvocationHandler (atm)

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179483
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #1040 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1040/ ) HADOOP-7717 . Move handling of concurrent client fail-overs to RetryInvocationHandler (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179483 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-Commit #1024 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1024/)
        HADOOP-7717. Move handling of concurrent client fail-overs to RetryInvocationHandler (atm)

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179483
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #1024 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1024/ ) HADOOP-7717 . Move handling of concurrent client fail-overs to RetryInvocationHandler (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179483 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #1102 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1102/)
        HADOOP-7717. Move handling of concurrent client fail-overs to RetryInvocationHandler (atm)

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179483
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #1102 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/1102/ ) HADOOP-7717 . Move handling of concurrent client fail-overs to RetryInvocationHandler (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1179483 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/RetryInvocationHandler.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/TestFailoverProxy.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableImplementation.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/retry/UnreliableInterface.java
        Hide
        Aaron T. Myers added a comment -

        Thanks a lot for the review, Todd. I've just committed this.

        Show
        Aaron T. Myers added a comment - Thanks a lot for the review, Todd. I've just committed this.
        Hide
        Todd Lipcon added a comment -

        +1

        Show
        Todd Lipcon added a comment - +1
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12497587/HADOOP-7717.patch
        against trunk revision .

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

        +1 tests included. The patch appears to include 9 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 unit tests in .

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/262//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/262//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/12497587/HADOOP-7717.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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 unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/262//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/262//console This message is automatically generated.
        Hide
        Aaron T. Myers added a comment -

        Thanks a lot for the review, Todd. I agree with your analysis. Here's an updated patch which addresses your concern.

        I also took this opportunity to change some of the fail-over count variable names, which hopefully will make things more clear.

        Show
        Aaron T. Myers added a comment - Thanks a lot for the review, Todd. I agree with your analysis. Here's an updated patch which addresses your concern. I also took this opportunity to change some of the fail-over count variable names, which hopefully will make things more clear.
        Hide
        Todd Lipcon added a comment -

        I think there's a race here around the use of the atomic integer. If one thread increments the integer and is about to call performFailover, then gets switched out, another thread can run, decide another failover has occurred, and then get the old proxy object (ie not failover).

        Instead I think you should use a regular int, then synchronize around both the check, the failover, and the resulting failoverCount++

        Show
        Todd Lipcon added a comment - I think there's a race here around the use of the atomic integer. If one thread increments the integer and is about to call performFailover, then gets switched out, another thread can run, decide another failover has occurred, and then get the old proxy object (ie not failover). Instead I think you should use a regular int, then synchronize around both the check, the failover, and the resulting failoverCount++
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12497583/HADOOP-7717.patch
        against trunk revision .

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

        +1 tests included. The patch appears to include 9 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 unit tests in .

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/260//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/260//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/12497583/HADOOP-7717.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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 unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/260//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/260//console This message is automatically generated.
        Hide
        Aaron T. Myers added a comment -

        Here's a patch which addresses the issue.

        Show
        Aaron T. Myers added a comment - Here's a patch which addresses the issue.

          People

          • Assignee:
            Aaron T. Myers
            Reporter:
            Aaron T. Myers
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development