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

      Description

      The process to clean up an RPC proxy object is to call RPC.stopProxy, which looks up the RPCEngine previously associated with the interface which that proxy object provides and calls RPCEngine.stopProxy passing in the proxy object. Every concrete implementation of RPCEngine.stopProxy then looks up the invocation handler associated with the proxy object and calls close() on that invocation handler.

      This process can be simplified by cutting out the steps of looking up the previously-registered RPCEngine, and instead just having RPC.stopProxy directly look up the invocation handler for the proxy object and call close() on it.

      1. hadoop-7607.0.patch
        6 kB
        Aaron T. Myers
      2. hadoop-7607.1.patch
        9 kB
        Aaron T. Myers
      3. HADOOP-7607.2.patch
        0.8 kB
        Uma Maheswara Rao G

        Issue Links

          Activity

          Hide
          Aaron T. Myers added a comment -

          Here's a patch which addresses the issue. I chose to log and swallow IllegalArgumentException to maintain backward compatibility with the previous implementation of RPC.stopProxy, which was just a no-op if you called it with a non-proxy object.

          This change will require a few changes to HDFS and MR as well, since RPC.stopProxy can now throw an IOE. If folks are OK with this direction, I'll file those JIRAs and address them there. Alternatively, we could make RPC.stopProxy also swallow IOE, but that seems unwise to me.

          Show
          Aaron T. Myers added a comment - Here's a patch which addresses the issue. I chose to log and swallow IllegalArgumentException to maintain backward compatibility with the previous implementation of RPC.stopProxy, which was just a no-op if you called it with a non-proxy object. This change will require a few changes to HDFS and MR as well, since RPC.stopProxy can now throw an IOE. If folks are OK with this direction, I'll file those JIRAs and address them there. Alternatively, we could make RPC.stopProxy also swallow IOE, but that seems unwise to me.
          Hide
          Todd Lipcon added a comment -
          • I think LOG.error is probably more appropriate if called on a non-proxy
          • The stopProxy code doesn't deal with ClassCastException. I think it might be better like:
            InvocationHandler handler = Proxy.getInvocationHandler(proxy);
            if (handler instanceof Closeable) {
              ((Closeable)handler).close();
            } else {
              warn
            }
            

            what do you think?

          • the javadoc indicates IAE is thrown but you catch it, like you said
          • it seems odd that you maintain backwards-compat in terms of not throwing IAE for non-proxy objects, but you now throw IOE where the old version didn't. I think we should either maintain complete back-compat (catch and warn on either exception) or ditch that and just throw both exceptions. I'm leaning towards the catch-and-warn route since RPC.stopProxy is probably used in some finally{} clauses where adding a new exception could break error recovery paths unless we're careful
          Show
          Todd Lipcon added a comment - I think LOG.error is probably more appropriate if called on a non-proxy The stopProxy code doesn't deal with ClassCastException. I think it might be better like: InvocationHandler handler = Proxy.getInvocationHandler(proxy); if (handler instanceof Closeable) { ((Closeable)handler).close(); } else { warn } what do you think? the javadoc indicates IAE is thrown but you catch it, like you said it seems odd that you maintain backwards-compat in terms of not throwing IAE for non-proxy objects, but you now throw IOE where the old version didn't. I think we should either maintain complete back-compat (catch and warn on either exception) or ditch that and just throw both exceptions. I'm leaning towards the catch-and-warn route since RPC.stopProxy is probably used in some finally{} clauses where adding a new exception could break error recovery paths unless we're careful
          Hide
          Aaron T. Myers added a comment -

          Thanks a lot for the review, Todd. I attempted to incorporate all of your feedback. Please have a look.

          This change will also require a small change to MR, as well. I'll file that JIRA in a momemnt.

          Show
          Aaron T. Myers added a comment - Thanks a lot for the review, Todd. I attempted to incorporate all of your feedback. Please have a look. This change will also require a small change to MR, as well. I'll file that JIRA in a momemnt.
          Hide
          Todd Lipcon added a comment -

          +1 pending hudson (I just kicked it)

          Show
          Todd Lipcon added a comment - +1 pending hudson (I just kicked it)
          Hide
          Hadoop QA added a comment -

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

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

          -1 tests included. 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 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 hadoop-common-project.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/143//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/143//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-annotations.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/143//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-auth-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/143//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/143//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-auth.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/143//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/12493098/hadoop-7607.1.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. 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 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 hadoop-common-project. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/143//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/143//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-annotations.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/143//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-auth-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/143//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/143//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-auth.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/143//console This message is automatically generated.
          Hide
          Aaron T. Myers added a comment -

          No tests included since this is just a refactor. I'll commit this in a few hours if there are no objections.

          Show
          Aaron T. Myers added a comment - No tests included since this is just a refactor. I'll commit this in a few hours if there are no objections.
          Hide
          Aaron T. Myers added a comment -

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

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

          Integrated in Hadoop-Mapreduce-trunk-Commit #871 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/871/)
          HADOOP-7607 and MAPREDUCE-2934. Simplify the RPC proxy cleanup process. (atm)

          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167318
          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/ipc/AvroRpcEngine.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RpcEngine.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/WritableRpcEngine.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/ipc/ProtoOverHadoopRpcEngine.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #871 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/871/ ) HADOOP-7607 and MAPREDUCE-2934 . Simplify the RPC proxy cleanup process. (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167318 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/ipc/AvroRpcEngine.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RpcEngine.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/WritableRpcEngine.java /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/ipc/ProtoOverHadoopRpcEngine.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #860 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/860/)
          HADOOP-7607 and MAPREDUCE-2934. Simplify the RPC proxy cleanup process. (atm)

          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167318
          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/ipc/AvroRpcEngine.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RpcEngine.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/WritableRpcEngine.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/ipc/ProtoOverHadoopRpcEngine.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #860 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/860/ ) HADOOP-7607 and MAPREDUCE-2934 . Simplify the RPC proxy cleanup process. (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167318 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/ipc/AvroRpcEngine.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RpcEngine.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/WritableRpcEngine.java /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/ipc/ProtoOverHadoopRpcEngine.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #937 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/937/)
          HADOOP-7607 and MAPREDUCE-2934. Simplify the RPC proxy cleanup process. (atm)

          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167318
          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/ipc/AvroRpcEngine.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RpcEngine.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/WritableRpcEngine.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/ipc/ProtoOverHadoopRpcEngine.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #937 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/937/ ) HADOOP-7607 and MAPREDUCE-2934 . Simplify the RPC proxy cleanup process. (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167318 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/ipc/AvroRpcEngine.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RpcEngine.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/WritableRpcEngine.java /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/ipc/ProtoOverHadoopRpcEngine.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #789 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/789/)
          HADOOP-7607 and MAPREDUCE-2934. Simplify the RPC proxy cleanup process. (atm)

          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167318
          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/ipc/AvroRpcEngine.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RpcEngine.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/WritableRpcEngine.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/ipc/ProtoOverHadoopRpcEngine.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #789 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/789/ ) HADOOP-7607 and MAPREDUCE-2934 . Simplify the RPC proxy cleanup process. (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167318 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/ipc/AvroRpcEngine.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RpcEngine.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/WritableRpcEngine.java /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/ipc/ProtoOverHadoopRpcEngine.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #813 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/813/)
          HADOOP-7607 and MAPREDUCE-2934. Simplify the RPC proxy cleanup process. (atm)

          atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167318
          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/ipc/AvroRpcEngine.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RpcEngine.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/WritableRpcEngine.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/ipc/ProtoOverHadoopRpcEngine.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #813 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/813/ ) HADOOP-7607 and MAPREDUCE-2934 . Simplify the RPC proxy cleanup process. (atm) atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167318 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/ipc/AvroRpcEngine.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RPC.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RpcEngine.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/WritableRpcEngine.java /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/ipc/ProtoOverHadoopRpcEngine.java
          Hide
          Uma Maheswara Rao G added a comment -

          Hi Aaron,

              } else {
          +      LOG.error("Could not get invocation handler " + invocationHandler +
          +          " for proxy " + proxy + ", or invocation handler is not closeable.");
               }
          

          I think printing proxy may create problem.If invocationHandler is non Closeable and has valid proxy, it may throw NoSuchMethodException.

          I just removed closeable from RetryInvocationHandler and ran some tests. Since it will invoke toString on proxy obj, it started throwing NoSuchMethodException.

          Caused by: java.lang.NoSuchMethodException: org.apache.hadoop.hdfs.protocol.ClientProtocol.toString()
          at java.lang.Class.getMethod(Unknown Source)
          at org.apache.hadoop.io.retry.RetryInvocationHandler.invoke(RetryInvocationHandler.java:70)
          ... 37 more

          I think we can just change this message to like below

          LOG.error("Could not get invocation handler " + invocationHandler +
          " for proxy " + proxy.getClass() + ", or invocation handler is not closeable.");

          am i missing something here?

          Thanks,
          Uma

          Show
          Uma Maheswara Rao G added a comment - Hi Aaron, } else { + LOG.error( "Could not get invocation handler " + invocationHandler + + " for proxy " + proxy + ", or invocation handler is not closeable." ); } I think printing proxy may create problem.If invocationHandler is non Closeable and has valid proxy, it may throw NoSuchMethodException. I just removed closeable from RetryInvocationHandler and ran some tests. Since it will invoke toString on proxy obj, it started throwing NoSuchMethodException. Caused by: java.lang.NoSuchMethodException: org.apache.hadoop.hdfs.protocol.ClientProtocol.toString() at java.lang.Class.getMethod(Unknown Source) at org.apache.hadoop.io.retry.RetryInvocationHandler.invoke(RetryInvocationHandler.java:70) ... 37 more I think we can just change this message to like below LOG.error("Could not get invocation handler " + invocationHandler + " for proxy " + proxy.getClass() + ", or invocation handler is not closeable."); am i missing something here? Thanks, Uma
          Hide
          Aaron T. Myers added a comment -

          Hey Uma, I'm afraid I'm not able to reproduce this issue. Can you maybe upload a patch with the changes you made, and describe the exact test scenario you ran?

          Show
          Aaron T. Myers added a comment - Hey Uma, I'm afraid I'm not able to reproduce this issue. Can you maybe upload a patch with the changes you made, and describe the exact test scenario you ran?
          Hide
          Uma Maheswara Rao G added a comment -

          Ok, i will upload the patch with.

          Thanks,
          Uma

          Show
          Uma Maheswara Rao G added a comment - Ok, i will upload the patch with. Thanks, Uma
          Hide
          Uma Maheswara Rao G added a comment -

          Hi Aaron,

          I have uploaded the patch!

          Scenario to reproduce it:
          1)Remove the implement Closeable and close method from RetryInvocationHandler.
          This is just to simulate Non closeable InvocationHandler.

          2) Try to run some tests. (I ran the TestBalancer.java)

          in RPC.java's stopProxy API (see below)

          if (proxy != null && invocationHandler != null &&
                  invocationHandler instanceof Closeable) {
                try {
                  ((Closeable)invocationHandler).close();
                } catch (IOException e) {
                  LOG.error("Stopping RPC invocation handler caused exception", e);
                }
              } else {
                LOG.error("Could not get invocation handler " + invocationHandler +
                    " for proxy " + proxy + ", or invocation handler is not closeable.");
              }
          

          since it is not a Closeable it will go to else. Here when printing proxy in log, it will try to invoke toString API.

          This will throw NosuchMethodException.

          I don't have ready made test case to reproduce it.
          Below is the trace:

          java.lang.reflect.UndeclaredThrowableException
          at $Proxy11.toString(Unknown Source)
          at java.lang.String.valueOf(Unknown Source)
          at java.lang.StringBuilder.append(Unknown Source)
          at org.apache.hadoop.ipc.RPC.stopProxy(RPC.java:492)
          at org.apache.hadoop.hdfs.DFSClient.close(DFSClient.java:381)
          at org.apache.hadoop.hdfs.MiniDFSCluster.waitActive(MiniDFSCluster.java:1510)
          at org.apache.hadoop.hdfs.MiniDFSCluster.waitActive(MiniDFSCluster.java:1521)
          at org.apache.hadoop.hdfs.MiniDFSCluster.startDataNodes(MiniDFSCluster.java:916)
          at org.apache.hadoop.hdfs.MiniDFSCluster.startDataNodes(MiniDFSCluster.java:782)
          at org.apache.hadoop.hdfs.MiniDFSCluster.initMiniDFSCluster(MiniDFSCluster.java:563)
          at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:257)
          at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:250)
          at org.apache.hadoop.hdfs.MiniDFSCluster$Builder.build(MiniDFSCluster.java:243)
          at org.apache.hadoop.hdfs.server.balancer.TestBalancer.doTest(TestBalancer.java:305)
          at org.apache.hadoop.hdfs.server.balancer.TestBalancer.oneNodeTest(TestBalancer.java:346)
          at org.apache.hadoop.hdfs.server.balancer.TestBalancer.testBalancer0(TestBalancer.java:366)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
          at java.lang.reflect.Method.invoke(Unknown Source)
          at junit.framework.TestCase.runTest(TestCase.java:168)
          at junit.framework.TestCase.runBare(TestCase.java:134)
          at junit.framework.TestResult$1.protect(TestResult.java:110)
          at junit.framework.TestResult.runProtected(TestResult.java:128)
          at junit.framework.TestResult.run(TestResult.java:113)
          at junit.framework.TestCase.run(TestCase.java:124)
          at junit.framework.TestSuite.runTest(TestSuite.java:232)
          at junit.framework.TestSuite.run(TestSuite.java:227)
          at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:83)
          at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:46)
          at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
          at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)
          Caused by: java.lang.NoSuchMethodException: org.apache.hadoop.hdfs.protocol.ClientProtocol.toString()
          at java.lang.Class.getMethod(Unknown Source)
          at org.apache.hadoop.io.retry.RetryInvocationHandler.invoke(RetryInvocationHandler.java:70)
          ... 35 more

          Show
          Uma Maheswara Rao G added a comment - Hi Aaron, I have uploaded the patch! Scenario to reproduce it: 1)Remove the implement Closeable and close method from RetryInvocationHandler. This is just to simulate Non closeable InvocationHandler. 2) Try to run some tests. (I ran the TestBalancer.java) in RPC.java's stopProxy API (see below) if (proxy != null && invocationHandler != null && invocationHandler instanceof Closeable) { try { ((Closeable)invocationHandler).close(); } catch (IOException e) { LOG.error( "Stopping RPC invocation handler caused exception" , e); } } else { LOG.error( "Could not get invocation handler " + invocationHandler + " for proxy " + proxy + ", or invocation handler is not closeable." ); } since it is not a Closeable it will go to else. Here when printing proxy in log, it will try to invoke toString API. This will throw NosuchMethodException. I don't have ready made test case to reproduce it. Below is the trace: java.lang.reflect.UndeclaredThrowableException at $Proxy11.toString(Unknown Source) at java.lang.String.valueOf(Unknown Source) at java.lang.StringBuilder.append(Unknown Source) at org.apache.hadoop.ipc.RPC.stopProxy(RPC.java:492) at org.apache.hadoop.hdfs.DFSClient.close(DFSClient.java:381) at org.apache.hadoop.hdfs.MiniDFSCluster.waitActive(MiniDFSCluster.java:1510) at org.apache.hadoop.hdfs.MiniDFSCluster.waitActive(MiniDFSCluster.java:1521) at org.apache.hadoop.hdfs.MiniDFSCluster.startDataNodes(MiniDFSCluster.java:916) at org.apache.hadoop.hdfs.MiniDFSCluster.startDataNodes(MiniDFSCluster.java:782) at org.apache.hadoop.hdfs.MiniDFSCluster.initMiniDFSCluster(MiniDFSCluster.java:563) at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:257) at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:250) at org.apache.hadoop.hdfs.MiniDFSCluster$Builder.build(MiniDFSCluster.java:243) at org.apache.hadoop.hdfs.server.balancer.TestBalancer.doTest(TestBalancer.java:305) at org.apache.hadoop.hdfs.server.balancer.TestBalancer.oneNodeTest(TestBalancer.java:346) at org.apache.hadoop.hdfs.server.balancer.TestBalancer.testBalancer0(TestBalancer.java:366) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) at java.lang.reflect.Method.invoke(Unknown Source) at junit.framework.TestCase.runTest(TestCase.java:168) at junit.framework.TestCase.runBare(TestCase.java:134) at junit.framework.TestResult$1.protect(TestResult.java:110) at junit.framework.TestResult.runProtected(TestResult.java:128) at junit.framework.TestResult.run(TestResult.java:113) at junit.framework.TestCase.run(TestCase.java:124) at junit.framework.TestSuite.runTest(TestSuite.java:232) at junit.framework.TestSuite.run(TestSuite.java:227) at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:83) at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:46) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197) Caused by: java.lang.NoSuchMethodException: org.apache.hadoop.hdfs.protocol.ClientProtocol.toString() at java.lang.Class.getMethod(Unknown Source) at org.apache.hadoop.io.retry.RetryInvocationHandler.invoke(RetryInvocationHandler.java:70) ... 35 more
          Hide
          Aaron T. Myers added a comment -

          OK, I've looked into this issue quite a bit. The thing that surprised me about Uma's description of the problem is that the proxy object (like all Objects) should have an implementation of toString(), so I was surprised that anything would throw a noSuchMethodException. For that matter, note that the last line of the stack trace Uma posted references RetryInvocationHandler.java:70, which is inside a catch clause which only gets reached if the method invocation fails. I was obviously surprised that a call to toString() would throw an exception.

          Turns out this happens because RetryInvocationHandler.invoke assumes that the interface implemented by the proxy declares all the methods which get invoked on it. This is done to check for the @Idempotent annotation on the method, and this is what throws the NoSuchMethodException. The reason the call to toString() throws an exception is because the underlying WritableRpcEngine checks for the declaring class's (Object's) versionID field, which obviously doesn't exist on Object.

          So, this patch doesn't really introduce the issue, except to cause toString() to be potentially called on an implementation of ClientProtocol. Ideally we would do something to allow the calling of toString() on RPC proxy objects without weird exceptions, but that seems like it's beyond the scope of this JIRA. I'll file a separate one to address the issue of calling toString on a proxy object. The patch Uma supplied won't quite work as-is, since the proxy and invocationHandler objects can't be assumed to be non-null, but it's close. I'll file a separate JIRA to get this addressed as well.

          Show
          Aaron T. Myers added a comment - OK, I've looked into this issue quite a bit. The thing that surprised me about Uma's description of the problem is that the proxy object (like all Objects) should have an implementation of toString() , so I was surprised that anything would throw a noSuchMethodException . For that matter, note that the last line of the stack trace Uma posted references RetryInvocationHandler.java:70 , which is inside a catch clause which only gets reached if the method invocation fails. I was obviously surprised that a call to toString() would throw an exception. Turns out this happens because RetryInvocationHandler.invoke assumes that the interface implemented by the proxy declares all the methods which get invoked on it. This is done to check for the @Idempotent annotation on the method, and this is what throws the NoSuchMethodException . The reason the call to toString() throws an exception is because the underlying WritableRpcEngine checks for the declaring class's ( Object 's) versionID field, which obviously doesn't exist on Object . So, this patch doesn't really introduce the issue, except to cause toString() to be potentially called on an implementation of ClientProtocol . Ideally we would do something to allow the calling of toString() on RPC proxy objects without weird exceptions, but that seems like it's beyond the scope of this JIRA. I'll file a separate one to address the issue of calling toString on a proxy object. The patch Uma supplied won't quite work as-is, since the proxy and invocationHandler objects can't be assumed to be non-null, but it's close. I'll file a separate JIRA to get this addressed as well.
          Hide
          Aaron T. Myers added a comment -

          Filed HADOOP-7694 and HADOOP-7695 to get these two issues addressed.

          Show
          Aaron T. Myers added a comment - Filed HADOOP-7694 and HADOOP-7695 to get these two issues addressed.
          Hide
          Uma Maheswara Rao G added a comment -

          Make sense to me, Thanks for addressing the issue & filing Jiras.

          Show
          Uma Maheswara Rao G added a comment - Make sense to me, Thanks for addressing the issue & filing Jiras.
          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:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development