Hadoop Common
  1. Hadoop Common
  2. HADOOP-7635

RetryInvocationHandler should release underlying resources on close

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.24.0
    • Fix Version/s: 2.0.0-alpha
    • Component/s: ipc
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Target Version/s:

      Description

      It is often the case that RPC invocation handlers (e.g. o.a.h.ipc.WritableRpcEngine.Invoker) are wrapped in a RetryInvocationHandler instance to handle RPC retry logic. Since RetryInvocationHandler doesn't have any resources of its own, and is incapable of releasing the resources of the wrapped InvocationHandler, users of RetryInvocationHandler must keep around a reference to the underlying InvocationHandler only for the purpose of closing. For an example of this, see o.a.h.hdfs.DFSClient, in particular the member variables namenode and rpcNamenode.

      1. hadoop-7635.0.txt
        15 kB
        Aaron T. Myers
      2. hadoop-7635.1.txt
        12 kB
        Aaron T. Myers

        Activity

        Hide
        Aaron T. Myers added a comment -

        Here's a patch which addresses the issue by requiring FailoverProxyProvider implementations to also implement Closeable, and then having RetryInvocationHandler call close() on the underlying proxy provider object.

        As an example, I've included in this patch what the change to DFSClient would look like to take advantage of the change in RPC closing. If the patch is approved, I won't include this portion of the patch in the commit, and will instead file a separate HDFS JIRA.

        Show
        Aaron T. Myers added a comment - Here's a patch which addresses the issue by requiring FailoverProxyProvider implementations to also implement Closeable , and then having RetryInvocationHandler call close() on the underlying proxy provider object. As an example, I've included in this patch what the change to DFSClient would look like to take advantage of the change in RPC closing. If the patch is approved, I won't include this portion of the patch in the commit, and will instead file a separate HDFS JIRA.
        Hide
        Aaron T. Myers added a comment -

        I should have also mentioned: I took the liberty of converting TestRPC to a Junit 4-style test.

        Show
        Aaron T. Myers added a comment - I should have also mentioned: I took the liberty of converting TestRPC to a Junit 4-style test.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12494535/hadoop-7635.0.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 6 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/184//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/12494535/hadoop-7635.0.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/184//console This message is automatically generated.
        Hide
        Aaron T. Myers added a comment -

        Not sure why the patch application failed - it works for me against trunk locally. I've just kicked Jenkins.

        Show
        Aaron T. Myers added a comment - Not sure why the patch application failed - it works for me against trunk locally. I've just kicked Jenkins.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12494535/hadoop-7635.0.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 6 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/185//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/12494535/hadoop-7635.0.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/185//console This message is automatically generated.
        Hide
        Aaron T. Myers added a comment -

        Just to test out Jenkins, I'm attaching a patch which is relative to hadoop-common-project, instead of the repo root.

        Show
        Aaron T. Myers added a comment - Just to test out Jenkins, I'm attaching a patch which is relative to hadoop-common-project, instead of the repo root.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12494572/hadoop-7635.1.txt
        against trunk revision .

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

        +1 tests included. The patch appears to include 6 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 generated 9 release audit warnings (more than the trunk's current 0 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/186//testReport/
        Release audit warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/186//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/186//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/12494572/hadoop-7635.1.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 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 generated 9 release audit warnings (more than the trunk's current 0 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/186//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/186//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/186//console This message is automatically generated.
        Hide
        Aaron T. Myers added a comment -

        There we go!

        I feel confident that the release audit warnings are unrelated to this patch.

        Show
        Aaron T. Myers added a comment - There we go! I feel confident that the release audit warnings are unrelated to this patch.
        Hide
        Aaron T. Myers added a comment -

        I believe that the RAT warnings are due to HADOOP-7599. I've just commented over there to get that addressed.

        Show
        Aaron T. Myers added a comment - I believe that the RAT warnings are due to HADOOP-7599 . I've just commented over there to get that addressed.
        Hide
        Todd Lipcon added a comment -

        Patch looks good. Only thought is that it might be worth running the HDFS suite against a common built with this patch before committing - just as a sanity check that it doesn't break anything unexpectedly. Or commit and be ready to revert if it causes a regression in the downstream builds Up to you.

        Show
        Todd Lipcon added a comment - Patch looks good. Only thought is that it might be worth running the HDFS suite against a common built with this patch before committing - just as a sanity check that it doesn't break anything unexpectedly. Or commit and be ready to revert if it causes a regression in the downstream builds Up to you.
        Hide
        Todd Lipcon added a comment -

        By which I mean to say: "+1"

        Show
        Todd Lipcon added a comment - By which I mean to say: "+1"
        Hide
        Aaron T. Myers added a comment -

        Thanks a lot for the quick review, Todd.

        I ran the full HDFS test suite last night and these were the only failures, all of which appear to be failing on trunk:

        org.apache.hadoop.hdfs.server.namenode.TestBackupNode
        org.apache.hadoop.hdfs.server.datanode.TestReplicasMap
        org.apache.hadoop.hdfs.server.blockmanagement.TestHost2NodesMap
        org.apache.hadoop.hdfs.TestDfsOverAvroRpc
        

        I'll commit this momentarily.

        Show
        Aaron T. Myers added a comment - Thanks a lot for the quick review, Todd. I ran the full HDFS test suite last night and these were the only failures, all of which appear to be failing on trunk: org.apache.hadoop.hdfs.server.namenode.TestBackupNode org.apache.hadoop.hdfs.server.datanode.TestReplicasMap org.apache.hadoop.hdfs.server.blockmanagement.TestHost2NodesMap org.apache.hadoop.hdfs.TestDfsOverAvroRpc I'll commit this momentarily.
        Hide
        Aaron T. Myers added a comment -

        I've just committed this.

        Show
        Aaron T. Myers added a comment - I've just committed this.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #907 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/907/)
        HADOOP-7635. RetryInvocationHandler should release underlying resources on close (atm)

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171221
        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/DefaultFailoverProxyProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java
        • /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/ipc/TestRPC.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #907 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/907/ ) HADOOP-7635 . RetryInvocationHandler should release underlying resources on close (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171221 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/DefaultFailoverProxyProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java /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/ipc/TestRPC.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-Commit #895 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/895/)
        HADOOP-7635. RetryInvocationHandler should release underlying resources on close (atm)

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171221
        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/DefaultFailoverProxyProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java
        • /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/ipc/TestRPC.java
        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #895 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/895/ ) HADOOP-7635 . RetryInvocationHandler should release underlying resources on close (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171221 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/DefaultFailoverProxyProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java /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/ipc/TestRPC.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #972 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/972/)
        HADOOP-7635. RetryInvocationHandler should release underlying resources on close (atm)

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171221
        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/DefaultFailoverProxyProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java
        • /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/ipc/TestRPC.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #972 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/972/ ) HADOOP-7635 . RetryInvocationHandler should release underlying resources on close (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171221 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/DefaultFailoverProxyProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java /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/ipc/TestRPC.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #802 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/802/)
        HADOOP-7635. RetryInvocationHandler should release underlying resources on close (atm)

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171221
        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/DefaultFailoverProxyProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java
        • /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/ipc/TestRPC.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #802 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/802/ ) HADOOP-7635 . RetryInvocationHandler should release underlying resources on close (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171221 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/DefaultFailoverProxyProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java /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/ipc/TestRPC.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #832 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/832/)
        HADOOP-7635. RetryInvocationHandler should release underlying resources on close (atm)

        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171221
        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/DefaultFailoverProxyProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java
        • /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/ipc/TestRPC.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #832 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/832/ ) HADOOP-7635 . RetryInvocationHandler should release underlying resources on close (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171221 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/DefaultFailoverProxyProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/retry/FailoverProxyProvider.java /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/ipc/TestRPC.java
        Hide
        Eli Collins added a comment -

        Good candidate for merging to branch 23.

        Show
        Eli Collins added a comment - Good candidate for merging to branch 23.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        I have merged this to 0.23.

        Show
        Tsz Wo Nicholas Sze added a comment - I have merged this to 0.23.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development