Hadoop Common
  1. Hadoop Common
  2. HADOOP-8202

stopproxy() is not closing the proxies correctly

    Details

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

      Description

      I was running testbackupnode and noticed that NNprotocol proxy was not being closed. Talked with Suresh and he observed that most of the protocols do not implement ProtocolTranslator and hence the logic in stopproxy() does not work. Instead, since all of them are closeable, Suresh suggested that closeable property should be used at close.

      1. HADOOP-8202.patch
        2 kB
        Hari Mankude
      2. HADOOP-8202.patch
        2 kB
        Hari Mankude
      3. HADOOP-8202-1.patch
        2 kB
        Hari Mankude
      4. HADOOP-8202-2.patch
        4 kB
        Hari Mankude
      5. HADOOP-8202-3.patch
        5 kB
        Hari Mankude
      6. HADOOP-8202-4.patch
        5 kB
        Suresh Srinivas

        Issue Links

          Activity

          Hide
          Hari Mankude added a comment -

          This jira is related to hadoop-7607

          Show
          Hari Mankude added a comment - This jira is related to hadoop-7607
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12519670/HADOOP-8202.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 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 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 failed these unit tests:
          org.apache.hadoop.fs.viewfs.TestViewFsTrash

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/754//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/754//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/754//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/12519670/HADOOP-8202.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 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to introduce 1 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 failed these unit tests: org.apache.hadoop.fs.viewfs.TestViewFsTrash +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/754//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/754//artifact/trunk/hadoop-common-project/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/754//console This message is automatically generated.
          Hide
          Suresh Srinivas added a comment -

          Does this fix the exception that you observed in the test, where proxy was not stopped?

          Show
          Suresh Srinivas added a comment - Does this fix the exception that you observed in the test, where proxy was not stopped?
          Hide
          Hari Mankude added a comment -

          Does this fix the exception that you observed in the test, where proxy was not stopped?

          Yes, it does.

          Show
          Hari Mankude added a comment - Does this fix the exception that you observed in the test, where proxy was not stopped? Yes, it does.
          Hide
          Aaron T. Myers added a comment -

          Hi Hari, a few comments:

          1. "c = (Closeable) Proxy.getInvocationHandler(proxy);" - this could potentially cause an uncaught ClassCastException, if the InvocationHandler itself doesn't implement Closeable.
          2. Given the above, the error message at the bottom of the message should perhaps also include "or invocation handler does not implement Closeable"
          3. 'LOG.error(... + "or does not provide invocation handler for proxy class"' - should put a space after "class"
          4. 'LOG.error("Cannot close proxy since it is null ");' - unnecessary whitespace at the end of the string.
          5. Seems like it shouldn't be too tough to write a test for this with some mock objects.
          6. Rather than have a single catch-all error message at the bottom, and return early to avoid it, I think it'd be better to only ever log a single error, and include the relevant information which caused the failure to close the proxy in that log message.

          Also:

          Does this fix the exception that you observed in the test, where proxy was not stopped?

          What exception? In what test?

          Show
          Aaron T. Myers added a comment - Hi Hari, a few comments: "c = (Closeable) Proxy.getInvocationHandler(proxy);" - this could potentially cause an uncaught ClassCastException, if the InvocationHandler itself doesn't implement Closeable. Given the above, the error message at the bottom of the message should perhaps also include "or invocation handler does not implement Closeable" 'LOG.error(... + "or does not provide invocation handler for proxy class"' - should put a space after "class" 'LOG.error("Cannot close proxy since it is null ");' - unnecessary whitespace at the end of the string. Seems like it shouldn't be too tough to write a test for this with some mock objects. Rather than have a single catch-all error message at the bottom, and return early to avoid it, I think it'd be better to only ever log a single error, and include the relevant information which caused the failure to close the proxy in that log message. Also: Does this fix the exception that you observed in the test, where proxy was not stopped? What exception? In what test?
          Hide
          Aaron T. Myers added a comment -

          Also, given that implementing the ProtocolTranslator interface is necessary in order for RPC#getServerAddress to work, perhaps a better solution would be to leave RPC#stopProxy as it is now, and change the protocol translators that currently implement Closeable to instead implement ProtocolTrnaslator.

          Thoughts?

          Show
          Aaron T. Myers added a comment - Also, given that implementing the ProtocolTranslator interface is necessary in order for RPC#getServerAddress to work, perhaps a better solution would be to leave RPC#stopProxy as it is now, and change the protocol translators that currently implement Closeable to instead implement ProtocolTrnaslator . Thoughts?
          Hide
          Suresh Srinivas added a comment -

          "c = (Closeable) Proxy.getInvocationHandler(proxy);" - this could potentially cause an uncaught ClassCastException, if the InvocationHandler itself doesn't implement Closeable.

          Not necessary - all invocation handlers in Hadoop are RpcInvocationHandlers and they implement Closeable.

          Seems like it shouldn't be too tough to write a test for this with some mock objects.

          I think this seems wholly unnecessary. Currently stopProxy is completely failing. I know that multiple times things have changed in this part of the code, and has been broken for some time.

          I am +1 without any tests.

          Rather than have a single catch-all error message at the bottom, and return early to avoid it, I think it'd be better to only ever log a single error, and include the relevant information which caused the failure to close the proxy in that log message.

          This is your coding style. I am not sure if it should be followed by every one.

          Again, I am okay to commit this, once we have log statement from the existing test.

          Show
          Suresh Srinivas added a comment - "c = (Closeable) Proxy.getInvocationHandler(proxy);" - this could potentially cause an uncaught ClassCastException, if the InvocationHandler itself doesn't implement Closeable. Not necessary - all invocation handlers in Hadoop are RpcInvocationHandlers and they implement Closeable. Seems like it shouldn't be too tough to write a test for this with some mock objects. I think this seems wholly unnecessary. Currently stopProxy is completely failing. I know that multiple times things have changed in this part of the code, and has been broken for some time. I am +1 without any tests. Rather than have a single catch-all error message at the bottom, and return early to avoid it, I think it'd be better to only ever log a single error, and include the relevant information which caused the failure to close the proxy in that log message. This is your coding style. I am not sure if it should be followed by every one. Again, I am okay to commit this, once we have log statement from the existing 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/12519675/HADOOP-8202.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 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 failed these unit tests:
          org.apache.hadoop.fs.viewfs.TestViewFsTrash

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/755//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/755//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/12519675/HADOOP-8202.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 eclipse:eclipse. The patch built with eclipse:eclipse. +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 failed these unit tests: org.apache.hadoop.fs.viewfs.TestViewFsTrash +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/755//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/755//console This message is automatically generated.
          Hide
          Suresh Srinivas added a comment -

          Also, given that implementing the ProtocolTranslator interface is necessary in order for RPC#getServerAddress to work, perhaps a better solution would be to leave RPC#stopProxy as it is now, and change the protocol translators that currently implement Closeable to instead implement ProtocolTrnaslator.

          I want to look at the ProtocolTranslator and why it is needed in detail. In this part of the code clearly ProtocolTranslator is not required.

          Show
          Suresh Srinivas added a comment - Also, given that implementing the ProtocolTranslator interface is necessary in order for RPC#getServerAddress to work, perhaps a better solution would be to leave RPC#stopProxy as it is now, and change the protocol translators that currently implement Closeable to instead implement ProtocolTrnaslator. I want to look at the ProtocolTranslator and why it is needed in detail. In this part of the code clearly ProtocolTranslator is not required.
          Hide
          Hari Mankude added a comment -

          The test was TestBackupNode. Including the relevant output from the test.

          2012-03-23 12:45:31,277 INFO namenode.NameNode (NameNodeRpcServer.java:errorReport(321)) - Error report from NamenodeRegistration(localhost:64139, role=Backup Node): Shutting down.

          2012-03-23 12:45:31,277 INFO namenode.FSEditLog (FSEditLog.java:releaseBackupStream(1030)) - Removing backup journal BackupJournalManager

          2012-03-23 12:45:31,277 ERROR ipc.RPC (RPC.java:stopProxy(593)) - Tried to call RPC.stopProxy on an object that is not a proxy.
          java.lang.IllegalArgumentException: not a proxy instance
          at java.lang.reflect.Proxy.getInvocationHandler(Proxy.java:637) at org.apache.hadoop.ipc.RPC.stopProxy(RPC.java:591)
          at org.apache.hadoop.hdfs.server.namenode.EditLogBackupOutputStream.abort(EditLogBackupOutputStream.java:106) at org.apache.hadoop.hdfs.server.namenode.JournalSet$JournalAndStream.abort(JournalSet.java:98)
          at org.apache.hadoop.hdfs.server.namenode.JournalSet.remove(JournalSet.java:531)
          at org.apache.hadoop.hdfs.server.namenode.FSEditLog.releaseBackupStream(FSEditLog.java:1031)
          at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.releaseBackupNode(FSNamesystem.java:4663)
          at org.apache.hadoop.hdfs.server.namenode.NameNodeRpcServer.errorReport(NameNodeRpcServer.java:323)
          at org.apache.hadoop.hdfs.protocolPB.NamenodeProtocolServerSideTranslatorPB.errorReport(NamenodeProtocolServerSideTranslatorPB.java:125)
          at org.apache.hadoop.hdfs.protocol.proto.NamenodeProtocolProtos$NamenodeProtocolService$2.callBlockingMethod(NamenodeProtocolProtos.java:8072)
          at org.apache.hadoop.ipc.ProtobufRpcEngine$Server$ProtoBufRpcInvoker.call(ProtobufRpcEngine.java:417)
          at org.apache.hadoop.ipc.RPC$Server.call(RPC.java:884)
          at org.apache.hadoop.ipc.Server$Handler$1.run(Server.java:1661)
          at org.apache.hadoop.ipc.Server$Handler$1.run(Server.java:1657)
          at java.security.AccessController.doPrivileged(Native Method)
          at javax.security.auth.Subject.doAs(Subject.java:396)
          at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1205)
          at org.apache.hadoop.ipc.Server$Handler.run(Server.java:1655)
          2012-03-23 12:45:31,277 ERROR ipc.RPC (RPC.java:stopProxy(603)) - Could not get invocation handler null for proxy c
          lass class org.apache.hadoop.hdfs.protocolPB.JournalProtocolTranslatorPB, or invocation handler is not closeable.
          2012-03-23 12:45:31,278 ERROR ipc.RPC (RPC.java:stopProxy(593)) - Tried to call RPC.stopProxy on an object that is
          not a proxy.java.lang.IllegalArgumentException: not a proxy instance
          at java.lang.reflect.Proxy.getInvocationHandler(Proxy.java:637)
          at org.apache.hadoop.ipc.RPC.stopProxy(RPC.java:591) at org.apache.hadoop.hdfs.server.namenode.BackupNode.stop(BackupNode.java:194)
          at org.apache.hadoop.hdfs.server.namenode.TestBackupNode.testBackupNodeTailsEdits(TestBackupNode.java:169)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:597)
          at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:44)
          at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
          at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:41)
          at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
          at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
          at org.junit.runners.BlockJUnit4ClassRunner.runNotIgnored(BlockJUnit4ClassRunner.java:79)
          at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:71)
          at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:49)
          at org.junit.runners.ParentRunner$3.run(ParentRunner.java:193)
          at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:52)
          at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:191)
          at org.junit.runners.ParentRunner.access$000(ParentRunner.java:42)
          at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:184)
          at org.junit.runners.ParentRunner.run(ParentRunner.java:236)
          at org.apache.maven.surefire.junit4.JUnit4TestSet.execute(JUnit4TestSet.java:53)
          at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:123)
          at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:104)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at java.lang.reflect.Method.invoke(Method.java:597)
          at org.apache.maven.surefire.util.ReflectionUtils.invokeMethodWithArray(ReflectionUtils.java:164)
          at org.apache.maven.surefire.booter.ProviderFactory$ProviderProxy.invoke(ProviderFactory.java:110)
          at org.apache.maven.surefire.booter.SurefireStarter.invokeProvider(SurefireStarter.java:175)
          at org.apache.maven.surefire.booter.SurefireStarter.runSuitesInProcessWhenForked(SurefireStarter.java:81)
          at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:68)

          2012-03-23 12:45:31,278 ERROR ipc.RPC (RPC.java:stopProxy(603)) - Could not get invocation handler null for proxy class class org.apache.hadoop.hdfs.protocolPB.NamenodeProtocolTranslatorPB, or invocation handler is not closeable. ---------> Proxy was not closed.

          Show
          Hari Mankude added a comment - The test was TestBackupNode. Including the relevant output from the test. 2012-03-23 12:45:31,277 INFO namenode.NameNode (NameNodeRpcServer.java:errorReport(321)) - Error report from NamenodeRegistration(localhost:64139, role=Backup Node): Shutting down. 2012-03-23 12:45:31,277 INFO namenode.FSEditLog (FSEditLog.java:releaseBackupStream(1030)) - Removing backup journal BackupJournalManager 2012-03-23 12:45:31,277 ERROR ipc.RPC (RPC.java:stopProxy(593)) - Tried to call RPC.stopProxy on an object that is not a proxy. java.lang.IllegalArgumentException: not a proxy instance at java.lang.reflect.Proxy.getInvocationHandler(Proxy.java:637) at org.apache.hadoop.ipc.RPC.stopProxy(RPC.java:591) at org.apache.hadoop.hdfs.server.namenode.EditLogBackupOutputStream.abort(EditLogBackupOutputStream.java:106) at org.apache.hadoop.hdfs.server.namenode.JournalSet$JournalAndStream.abort(JournalSet.java:98) at org.apache.hadoop.hdfs.server.namenode.JournalSet.remove(JournalSet.java:531) at org.apache.hadoop.hdfs.server.namenode.FSEditLog.releaseBackupStream(FSEditLog.java:1031) at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.releaseBackupNode(FSNamesystem.java:4663) at org.apache.hadoop.hdfs.server.namenode.NameNodeRpcServer.errorReport(NameNodeRpcServer.java:323) at org.apache.hadoop.hdfs.protocolPB.NamenodeProtocolServerSideTranslatorPB.errorReport(NamenodeProtocolServerSideTranslatorPB.java:125) at org.apache.hadoop.hdfs.protocol.proto.NamenodeProtocolProtos$NamenodeProtocolService$2.callBlockingMethod(NamenodeProtocolProtos.java:8072) at org.apache.hadoop.ipc.ProtobufRpcEngine$Server$ProtoBufRpcInvoker.call(ProtobufRpcEngine.java:417) at org.apache.hadoop.ipc.RPC$Server.call(RPC.java:884) at org.apache.hadoop.ipc.Server$Handler$1.run(Server.java:1661) at org.apache.hadoop.ipc.Server$Handler$1.run(Server.java:1657) at java.security.AccessController.doPrivileged(Native Method) at javax.security.auth.Subject.doAs(Subject.java:396) at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1205) at org.apache.hadoop.ipc.Server$Handler.run(Server.java:1655) 2012-03-23 12:45:31,277 ERROR ipc.RPC (RPC.java:stopProxy(603)) - Could not get invocation handler null for proxy c lass class org.apache.hadoop.hdfs.protocolPB.JournalProtocolTranslatorPB, or invocation handler is not closeable. 2012-03-23 12:45:31,278 ERROR ipc.RPC (RPC.java:stopProxy(593)) - Tried to call RPC.stopProxy on an object that is not a proxy.java.lang.IllegalArgumentException: not a proxy instance at java.lang.reflect.Proxy.getInvocationHandler(Proxy.java:637) at org.apache.hadoop.ipc.RPC.stopProxy(RPC.java:591) at org.apache.hadoop.hdfs.server.namenode.BackupNode.stop(BackupNode.java:194) at org.apache.hadoop.hdfs.server.namenode.TestBackupNode.testBackupNodeTailsEdits(TestBackupNode.java:169) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:44) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:41) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28) at org.junit.runners.BlockJUnit4ClassRunner.runNotIgnored(BlockJUnit4ClassRunner.java:79) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:71) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:49) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:193) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:52) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:191) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:42) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:184) at org.junit.runners.ParentRunner.run(ParentRunner.java:236) at org.apache.maven.surefire.junit4.JUnit4TestSet.execute(JUnit4TestSet.java:53) at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:123) at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:104) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.apache.maven.surefire.util.ReflectionUtils.invokeMethodWithArray(ReflectionUtils.java:164) at org.apache.maven.surefire.booter.ProviderFactory$ProviderProxy.invoke(ProviderFactory.java:110) at org.apache.maven.surefire.booter.SurefireStarter.invokeProvider(SurefireStarter.java:175) at org.apache.maven.surefire.booter.SurefireStarter.runSuitesInProcessWhenForked(SurefireStarter.java:81) at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:68) 2012-03-23 12:45:31,278 ERROR ipc.RPC (RPC.java:stopProxy(603)) - Could not get invocation handler null for proxy class class org.apache.hadoop.hdfs.protocolPB.NamenodeProtocolTranslatorPB, or invocation handler is not closeable. ---------> Proxy was not closed.
          Hide
          Aaron T. Myers added a comment -

          Not necessary - all invocation handlers in Hadoop are RpcInvocationHandlers and they implement Closeable.

          Can you guarantee that all future invocation handlers in Hadoop will implement Closeable?

          I think this seems wholly unnecessary. Currently stopProxy is completely failing. I know that multiple times things have changed in this part of the code, and has been broken for some time.

          This is a great reason to add a test - so that such things don't regress in the future.

          This is your coding style. I am not sure if it should be followed by every one.

          Sure, it's a preference, but that in itself isn't a good reason to not address the comment. Can you comment on why it's better to split the reasons for an error across several log statements?

          I want to look at the ProtocolTranslator and why it is needed in detail. In this part of the code clearly ProtocolTranslator is not required.

          If you have a better way to implement RPC.getServerAddress, I'm happy to hear it.

          The test was TestBackupNode.

          Thanks for the info, Hari.

          Show
          Aaron T. Myers added a comment - Not necessary - all invocation handlers in Hadoop are RpcInvocationHandlers and they implement Closeable. Can you guarantee that all future invocation handlers in Hadoop will implement Closeable? I think this seems wholly unnecessary. Currently stopProxy is completely failing. I know that multiple times things have changed in this part of the code, and has been broken for some time. This is a great reason to add a test - so that such things don't regress in the future. This is your coding style. I am not sure if it should be followed by every one. Sure, it's a preference, but that in itself isn't a good reason to not address the comment. Can you comment on why it's better to split the reasons for an error across several log statements? I want to look at the ProtocolTranslator and why it is needed in detail. In this part of the code clearly ProtocolTranslator is not required. If you have a better way to implement RPC.getServerAddress, I'm happy to hear it. The test was TestBackupNode. Thanks for the info, Hari.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12519700/HADOOP-8202-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 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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/758//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/758//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/12519700/HADOOP-8202-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 eclipse:eclipse. The patch built with eclipse:eclipse. +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/758//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/758//console This message is automatically generated.
          Hide
          Suresh Srinivas added a comment -

          The problem with the code that was written is, it silently ignored the error and just printed a log and did not indicate the error. This is what is being fixed now.

          Can you guarantee that all future invocation handlers in Hadoop will implement Closeable?

          Write the code with either proxy is closeable or has an invocation handler that is closeable. If that is not the case, then it is programming error! Throw RuntimeException, so it is found early and not silently ignored.

          This is a great reason to add a test - so that such things don't regress in the future.

          HADOOP-7607 did not add tests either. Hence this bug. I suggest, lets practice what we preach!

          On a related note, I am also not happy for simple changes, we keep mandating adding unit tests. Some times, it is okay to use judgement call and not add unnecessary tests. Adding useless tests comes at a cost. When we did the federation feature, we spent half the time fixing poorly documented and lame tests. Not that these tests were finding bugs, the tests simply did not work well the code changes.

          That said, Hari, if you want you can add tests.

          Sure, it's a preference, but that in itself isn't a good reason to not address the comment. Can you comment on why it's better to split the reasons for an error across several log statements?

          You cannot argue matter of taste. Is the code incorrect? Else, I like this code better, because I do not have to handle exception twice, but in one single place around closeable. I am observing that our code reviews are becoming too strict. Not every patch I review should look like the code I would write. As long it is correct, follows coding standards, it should be good. I have been seeing some comments these days, to say, can we call variable name as ioe instead of e. I believe, we should relax these.

          If you have a better way to implement RPC.getServerAddress, I'm happy to hear it.

          Perhaps this is the only way. I need to look into it. But that is not required while stopping proxy. It is orthogonal.

          Hari, after thinking a bit, I believe we should throw HadoopIllegalArgumentException, if either the proxy is not closeable or does not have Invocation handler.

          Show
          Suresh Srinivas added a comment - The problem with the code that was written is, it silently ignored the error and just printed a log and did not indicate the error. This is what is being fixed now. Can you guarantee that all future invocation handlers in Hadoop will implement Closeable? Write the code with either proxy is closeable or has an invocation handler that is closeable. If that is not the case, then it is programming error! Throw RuntimeException, so it is found early and not silently ignored. This is a great reason to add a test - so that such things don't regress in the future. HADOOP-7607 did not add tests either. Hence this bug. I suggest, lets practice what we preach! On a related note, I am also not happy for simple changes, we keep mandating adding unit tests. Some times, it is okay to use judgement call and not add unnecessary tests. Adding useless tests comes at a cost. When we did the federation feature, we spent half the time fixing poorly documented and lame tests. Not that these tests were finding bugs, the tests simply did not work well the code changes. That said, Hari, if you want you can add tests. Sure, it's a preference, but that in itself isn't a good reason to not address the comment. Can you comment on why it's better to split the reasons for an error across several log statements? You cannot argue matter of taste. Is the code incorrect? Else, I like this code better, because I do not have to handle exception twice, but in one single place around closeable. I am observing that our code reviews are becoming too strict. Not every patch I review should look like the code I would write. As long it is correct, follows coding standards, it should be good. I have been seeing some comments these days, to say, can we call variable name as ioe instead of e. I believe, we should relax these. If you have a better way to implement RPC.getServerAddress, I'm happy to hear it. Perhaps this is the only way. I need to look into it. But that is not required while stopping proxy. It is orthogonal. Hari, after thinking a bit, I believe we should throw HadoopIllegalArgumentException, if either the proxy is not closeable or does not have Invocation handler.
          Hide
          Hari Mankude added a comment -

          Including a patch that takes care of concerns

          1. Throws HadoopIllegalArgumentexception if proxy is not closeable
          2. Throws HadoopIllegalArgumentexception if invocation handler is not closeable
          3. New test to catch the error scenario when a null proxy is used.
          4. Modified existing negative test so that it does not fail since exception is thrown.
          5. Fixed javadoc comments

          Show
          Hari Mankude added a comment - Including a patch that takes care of concerns 1. Throws HadoopIllegalArgumentexception if proxy is not closeable 2. Throws HadoopIllegalArgumentexception if invocation handler is not closeable 3. New test to catch the error scenario when a null proxy is used. 4. Modified existing negative test so that it does not fail since exception is thrown. 5. Fixed javadoc comments
          Hide
          Aaron T. Myers added a comment -

          The problem with the code that was written is, it silently ignored the error and just printed a log and did not indicate the error. This is what is being fixed now.

          Note that when HADOOP-7607 was being implemented, it was a conscious decision to log a warning instead of throwing, so as to maintain backward compatibility with the previous implementation. See this comment. We're now making a conscious decision to change that here, which is fine, but it should be noted.

          HADOOP-7607 did not add tests either. Hence this bug. I suggest, lets practice what we preach!

          HADOOP-7607 didn't introduce this bug. HADOOP-7607 was just a refactor, hence why it had no tests.

          This bug was introduced because of the following events:

          1. It used to be that all proxy objects used by client classes were directly referenced by RPC engines. During this time, the code in RPC#stopProxy worked just fine, but required users of proxy objects that wrapped other proxy objects to hold a reference to the underlying proxy object just for the purpose of calling RPC#stopProxy. This is why for a long time DFSClient had two references to ClientProtocol objects - one to call methods on, the other to call RPC#stopProxy on.
          2. HADOOP-7607 refactored the code in RPC#stopProxy to actually call close on the invocation handler of the proxy object directly, instead of going through the RPCEngine. This would allow the invocation handler to bubble this down to underlying proxies. This was committed in September of 2011.
          3. We began to introduce protocol translators for protobuf support in December of 2011. This caused the code in RPC#stopProxy to stop actually releasing underlying resources when RPC#stopProxy was called with a translator object provided as the argument, since the objects we were calling RPC#stopProxy on were no longer actual proxy objects, and hence Proxy#getInvocationHandler would fail. Unfortunately, no one noticed this until now. The introduction of protocol translators also caused RPC#getServerAddress to break, but again no one noticed. I introduced the ProtocolTranslator interface because I needed RPC#getServerAddress to work in order to implement HDFS-2792.

          On a related note, I am also not happy for simple changes, we keep mandating adding unit tests. Some times, it is okay to use judgement call and not add unnecessary tests.

          Sure, I agree with you. The question here is what constitutes a "simple change."

          FWIW, we often add tests for "simple changes," not to prove that the new code works, but to prevent it from ever regressing in the future. See HDFS-3099 as an example.

          I am observing that our code reviews are becoming too strict. Not every patch I review should look like the code I would write. As long it is correct, follows coding standards, it should be good. I have been seeing some comments these days, to say, can we call variable name as ioe instead of e. I believe, we should relax these.

          Sure, they're definitely just nits, but it's not like they're difficult to address. It almost certainly would take less time to address these comments than it does to argue about them.

          Show
          Aaron T. Myers added a comment - The problem with the code that was written is, it silently ignored the error and just printed a log and did not indicate the error. This is what is being fixed now. Note that when HADOOP-7607 was being implemented, it was a conscious decision to log a warning instead of throwing, so as to maintain backward compatibility with the previous implementation. See this comment . We're now making a conscious decision to change that here, which is fine, but it should be noted. HADOOP-7607 did not add tests either. Hence this bug. I suggest, lets practice what we preach! HADOOP-7607 didn't introduce this bug. HADOOP-7607 was just a refactor, hence why it had no tests. This bug was introduced because of the following events: It used to be that all proxy objects used by client classes were directly referenced by RPC engines. During this time, the code in RPC#stopProxy worked just fine, but required users of proxy objects that wrapped other proxy objects to hold a reference to the underlying proxy object just for the purpose of calling RPC#stopProxy. This is why for a long time DFSClient had two references to ClientProtocol objects - one to call methods on, the other to call RPC#stopProxy on. HADOOP-7607 refactored the code in RPC#stopProxy to actually call close on the invocation handler of the proxy object directly, instead of going through the RPCEngine. This would allow the invocation handler to bubble this down to underlying proxies. This was committed in September of 2011. We began to introduce protocol translators for protobuf support in December of 2011. This caused the code in RPC#stopProxy to stop actually releasing underlying resources when RPC#stopProxy was called with a translator object provided as the argument, since the objects we were calling RPC#stopProxy on were no longer actual proxy objects, and hence Proxy#getInvocationHandler would fail. Unfortunately, no one noticed this until now. The introduction of protocol translators also caused RPC#getServerAddress to break, but again no one noticed. I introduced the ProtocolTranslator interface because I needed RPC#getServerAddress to work in order to implement HDFS-2792 . On a related note, I am also not happy for simple changes, we keep mandating adding unit tests. Some times, it is okay to use judgement call and not add unnecessary tests. Sure, I agree with you. The question here is what constitutes a "simple change." FWIW, we often add tests for "simple changes," not to prove that the new code works, but to prevent it from ever regressing in the future. See HDFS-3099 as an example. I am observing that our code reviews are becoming too strict. Not every patch I review should look like the code I would write. As long it is correct, follows coding standards, it should be good. I have been seeing some comments these days, to say, can we call variable name as ioe instead of e. I believe, we should relax these. Sure, they're definitely just nits, but it's not like they're difficult to address. It almost certainly would take less time to address these comments than it does to argue about them.
          Hide
          Aaron T. Myers added a comment -

          The latest patch looks good to me. +1 pending Jenkins.

          Show
          Aaron T. Myers added a comment - The latest patch looks good to me. +1 pending Jenkins.
          Hide
          Suresh Srinivas added a comment -

          HADOOP-7607

          I take what you are saying - since I do not want to waste time on this.

          Sure, they're definitely just nits, but it's not like they're difficult to address. It almost certainly would take less time to address these comments than it does to argue about them.

          In the comments, I post this kind of comments as optional - as a suggestion. If the contributor likes it/wants to address it, then it is up to him. Perhaps, adding optional/suggestion for such comments would be good, instead of cycles of nits and time wasted on addressing them. We can make better use of time.

          Show
          Suresh Srinivas added a comment - HADOOP-7607 I take what you are saying - since I do not want to waste time on this. Sure, they're definitely just nits, but it's not like they're difficult to address. It almost certainly would take less time to address these comments than it does to argue about them. In the comments, I post this kind of comments as optional - as a suggestion. If the contributor likes it/wants to address it, then it is up to him. Perhaps, adding optional/suggestion for such comments would be good, instead of cycles of nits and time wasted on addressing them. We can make better use of time.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 3 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 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 failed these unit tests:
          org.apache.hadoop.ha.TestHealthMonitor

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/772//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/772//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/12519747/HADOOP-8202-2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 eclipse:eclipse. The patch built with eclipse:eclipse. +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 failed these unit tests: org.apache.hadoop.ha.TestHealthMonitor +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/772//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/772//console This message is automatically generated.
          Hide
          Hari Mankude added a comment -

          testhealthmonitor failure is due to hadoop-8204.

          However, I am running the entire suite of hdfs tests to make sure that nothing else is broken.

          Show
          Hari Mankude added a comment - testhealthmonitor failure is due to hadoop-8204. However, I am running the entire suite of hdfs tests to make sure that nothing else is broken.
          Hide
          Hari Mankude added a comment -

          hdfs tests went through. There were test failures. However, it does not seem to be related to stopproxy() changes. I will file jiras on the test failures.

          Show
          Hari Mankude added a comment - hdfs tests went through. There were test failures. However, it does not seem to be related to stopproxy() changes. I will file jiras on the test failures.
          Hide
          Hari Mankude added a comment -

          hadoop-common tests also passed except for testheatlhmonitor. Looking at the testhealthmonitor test, it is creating a proxy using mockito and I am guessing that this is not closeable. This is the reason for the error.

          Show
          Hari Mankude added a comment - hadoop-common tests also passed except for testheatlhmonitor. Looking at the testhealthmonitor test, it is creating a proxy using mockito and I am guessing that this is not closeable. This is the reason for the error.
          Hide
          Aaron T. Myers added a comment -

          Thanks for looking into the errors, Hari. We will have to do something to address the TestHealthMonitor failure before we can commit this patch - either update TestHealthMonitor so that it passes with this change as-written, or change this patch to allow for proxies/invocation handlers which do not necessarily implement Closeable.

          Show
          Aaron T. Myers added a comment - Thanks for looking into the errors, Hari. We will have to do something to address the TestHealthMonitor failure before we can commit this patch - either update TestHealthMonitor so that it passes with this change as-written, or change this patch to allow for proxies/invocation handlers which do not necessarily implement Closeable.
          Hide
          Todd Lipcon added a comment -

          I think maintaining the ability to support mockito spies/mocks for proxies is important. We use it for simulating all kinds of failure conditions – I'm surprised you didn't have a lot of HDFS failures from the same issue.

          Show
          Todd Lipcon added a comment - I think maintaining the ability to support mockito spies/mocks for proxies is important. We use it for simulating all kinds of failure conditions – I'm surprised you didn't have a lot of HDFS failures from the same issue.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 3 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 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 failed these unit tests:
          org.apache.hadoop.fs.viewfs.TestViewFsTrash

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/774//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/774//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/12519847/HADOOP-8202-3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 eclipse:eclipse. The patch built with eclipse:eclipse. +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 failed these unit tests: org.apache.hadoop.fs.viewfs.TestViewFsTrash +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/774//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/774//console This message is automatically generated.
          Hide
          Hari Mankude added a comment -

          testviewfstrash failure is unrelated to this change.

          Show
          Hari Mankude added a comment - testviewfstrash failure is unrelated to this change.
          Hide
          Aaron T. Myers added a comment -

          Hey Hari, thanks for updating the patch to fix TestHealthMonitor.

          There were test failures. However, it does not seem to be related to stopproxy() changes. I will file jiras on the test failures.

          Can you list here which tests failed? Can you comment on Todd's comment that he's surprised many HDFS tests didn't fail? Perhaps it's because in most tests where we mock out a protocol object, we never try to close that protocol object. Just a guess.

          Show
          Aaron T. Myers added a comment - Hey Hari, thanks for updating the patch to fix TestHealthMonitor. There were test failures. However, it does not seem to be related to stopproxy() changes. I will file jiras on the test failures. Can you list here which tests failed? Can you comment on Todd's comment that he's surprised many HDFS tests didn't fail? Perhaps it's because in most tests where we mock out a protocol object, we never try to close that protocol object. Just a guess.
          Hide
          Hari Mankude added a comment -

          Can you list here which tests failed? Can you comment on Todd's comment that he's surprised many HDFS tests didn't fail? Perhaps it's because in most tests where we mock out a protocol object, we never try to close that protocol object. Just a guess.

          Sure, the tests that failed were
          1. TestStartup
          2. TestValidateConfigurationSettings
          3. TestGetBlocks

          I have a trivial fix for TestGetBlocks, even though unrelated to this jira. Other tests definitely don't seem to be doing stopproxy(). So, will file the jiras for now.

          Regarding Todd's comment, my guess is also that we never try to close mockito based proxy objects.

          Show
          Hari Mankude added a comment - Can you list here which tests failed? Can you comment on Todd's comment that he's surprised many HDFS tests didn't fail? Perhaps it's because in most tests where we mock out a protocol object, we never try to close that protocol object. Just a guess. Sure, the tests that failed were 1. TestStartup 2. TestValidateConfigurationSettings 3. TestGetBlocks I have a trivial fix for TestGetBlocks, even though unrelated to this jira. Other tests definitely don't seem to be doing stopproxy(). So, will file the jiras for now. Regarding Todd's comment, my guess is also that we never try to close mockito based proxy objects.
          Hide
          Todd Lipcon added a comment -

          Instead of adding the instanceof check anywhere we use an object that might be a mock, can we instead change the protocol interfaces themselves to "extend Closeable"? That will make sure that any proxy implementations themselves take care of extending it, and also will solve the mock issue (since the mock itself will then also extend Closeable).

          Show
          Todd Lipcon added a comment - Instead of adding the instanceof check anywhere we use an object that might be a mock, can we instead change the protocol interfaces themselves to "extend Closeable"? That will make sure that any proxy implementations themselves take care of extending it, and also will solve the mock issue (since the mock itself will then also extend Closeable).
          Hide
          Aaron T. Myers added a comment -

          Sure, the tests that failed were

          Thanks, Hari. I agree those test failures are unrelated. TestGetBlocks is currently failing on trunk (HDFS-3143) and I've seen TestStartup and TestValidateConfigurationSettings fail spuriously before.

          Show
          Aaron T. Myers added a comment - Sure, the tests that failed were Thanks, Hari. I agree those test failures are unrelated. TestGetBlocks is currently failing on trunk ( HDFS-3143 ) and I've seen TestStartup and TestValidateConfigurationSettings fail spuriously before.
          Hide
          Suresh Srinivas added a comment -

          Instead of adding the instanceof check anywhere we use an object that might be a mock, can we instead change the protocol interfaces themselves to "extend Closeable"? That will make sure that any proxy implementations themselves take care of extending it, and also will solve the mock issue (since the mock itself will then also extend Closeable).

          Given that it is done once, this patch should get committed. Todd, if you feel strongly about Closeable, please can you create a jira.

          Show
          Suresh Srinivas added a comment - Instead of adding the instanceof check anywhere we use an object that might be a mock, can we instead change the protocol interfaces themselves to "extend Closeable"? That will make sure that any proxy implementations themselves take care of extending it, and also will solve the mock issue (since the mock itself will then also extend Closeable). Given that it is done once, this patch should get committed. Todd, if you feel strongly about Closeable, please can you create a jira.
          Hide
          Suresh Srinivas added a comment -

          I am going to commit this in an hour or so. As I said in my previous comment, please create another jira, if further changes are needed.

          Show
          Suresh Srinivas added a comment - I am going to commit this in an hour or so. As I said in my previous comment, please create another jira, if further changes are needed.
          Hide
          Todd Lipcon added a comment -

          Sure, go ahead and commit. Thanks.

          Show
          Todd Lipcon added a comment - Sure, go ahead and commit. Thanks.
          Hide
          Suresh Srinivas added a comment -

          While committing I found a merge conflict - uploading the newer patch with merge conflict resolved.

          Show
          Suresh Srinivas added a comment - While committing I found a merge conflict - uploading the newer patch with merge conflict resolved.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 3 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 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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/783//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/783//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/12519992/HADOOP-8202-4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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 eclipse:eclipse. The patch built with eclipse:eclipse. +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/783//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/783//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          This broke TestZKFailoverController, since it's now getting IllegalArgumentException trying to close the proxy.

          Show
          Todd Lipcon added a comment - This broke TestZKFailoverController, since it's now getting IllegalArgumentException trying to close the proxy.
          Hide
          Suresh Srinivas added a comment -

          I committed the patch to both trunk and 0.23.3

          Show
          Suresh Srinivas added a comment - I committed the patch to both trunk and 0.23.3
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2009 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2009/)
          HADOOP-8202. RPC stopProxy() does not close the proxy correctly. Contributed by Hari Mankude. (Revision 1305704)

          Result = SUCCESS
          suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1305704
          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/ha/HealthMonitor.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/test/java/org/apache/hadoop/ipc/TestRPC.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2009 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2009/ ) HADOOP-8202 . RPC stopProxy() does not close the proxy correctly. Contributed by Hari Mankude. (Revision 1305704) Result = SUCCESS suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1305704 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/ha/HealthMonitor.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/test/java/org/apache/hadoop/ipc/TestRPC.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #1934 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1934/)
          HADOOP-8202. RPC stopProxy() does not close the proxy correctly. Contributed by Hari Mankude. (Revision 1305704)

          Result = SUCCESS
          suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1305704
          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/ha/HealthMonitor.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/test/java/org/apache/hadoop/ipc/TestRPC.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #1934 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1934/ ) HADOOP-8202 . RPC stopProxy() does not close the proxy correctly. Contributed by Hari Mankude. (Revision 1305704) Result = SUCCESS suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1305704 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/ha/HealthMonitor.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/test/java/org/apache/hadoop/ipc/TestRPC.java
          Hide
          Suresh Srinivas added a comment - - edited

          This broke TestZKFailoverController, since it's now getting IllegalArgumentException trying to close the proxy.

          HADOOP-8219 created.

          Show
          Suresh Srinivas added a comment - - edited This broke TestZKFailoverController, since it's now getting IllegalArgumentException trying to close the proxy. HADOOP-8219 created.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #1947 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1947/)
          HADOOP-8202. RPC stopProxy() does not close the proxy correctly. Contributed by Hari Mankude. (Revision 1305704)

          Result = ABORTED
          suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1305704
          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/ha/HealthMonitor.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/test/java/org/apache/hadoop/ipc/TestRPC.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #1947 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1947/ ) HADOOP-8202 . RPC stopProxy() does not close the proxy correctly. Contributed by Hari Mankude. (Revision 1305704) Result = ABORTED suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1305704 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/ha/HealthMonitor.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/test/java/org/apache/hadoop/ipc/TestRPC.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #997 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/997/)
          HADOOP-8202. RPC stopProxy() does not close the proxy correctly. Contributed by Hari Mankude. (Revision 1305704)

          Result = FAILURE
          suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1305704
          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/ha/HealthMonitor.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/test/java/org/apache/hadoop/ipc/TestRPC.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #997 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/997/ ) HADOOP-8202 . RPC stopProxy() does not close the proxy correctly. Contributed by Hari Mankude. (Revision 1305704) Result = FAILURE suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1305704 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/ha/HealthMonitor.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/test/java/org/apache/hadoop/ipc/TestRPC.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1032 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1032/)
          HADOOP-8202. RPC stopProxy() does not close the proxy correctly. Contributed by Hari Mankude. (Revision 1305704)

          Result = SUCCESS
          suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1305704
          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/ha/HealthMonitor.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/test/java/org/apache/hadoop/ipc/TestRPC.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1032 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1032/ ) HADOOP-8202 . RPC stopProxy() does not close the proxy correctly. Contributed by Hari Mankude. (Revision 1305704) Result = SUCCESS suresh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1305704 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/ha/HealthMonitor.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/test/java/org/apache/hadoop/ipc/TestRPC.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2014 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2014/)
          HDFS-3156. TestDFSHAAdmin is failing post HADOOP-8202. Contributed by Aaron T. Myers. (Revision 1306517)

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

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2014 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2014/ ) HDFS-3156 . TestDFSHAAdmin is failing post HADOOP-8202 . Contributed by Aaron T. Myers. (Revision 1306517) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1306517 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #1939 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1939/)
          HDFS-3156. TestDFSHAAdmin is failing post HADOOP-8202. Contributed by Aaron T. Myers. (Revision 1306517)

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

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #1939 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/1939/ ) HDFS-3156 . TestDFSHAAdmin is failing post HADOOP-8202 . Contributed by Aaron T. Myers. (Revision 1306517) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1306517 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #1952 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1952/)
          HDFS-3156. TestDFSHAAdmin is failing post HADOOP-8202. Contributed by Aaron T. Myers. (Revision 1306517)

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

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #1952 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/1952/ ) HDFS-3156 . TestDFSHAAdmin is failing post HADOOP-8202 . Contributed by Aaron T. Myers. (Revision 1306517) Result = ABORTED atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1306517 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1034 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1034/)
          HDFS-3156. TestDFSHAAdmin is failing post HADOOP-8202. Contributed by Aaron T. Myers. (Revision 1306517)

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

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1034 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1034/ ) HDFS-3156 . TestDFSHAAdmin is failing post HADOOP-8202 . Contributed by Aaron T. Myers. (Revision 1306517) Result = FAILURE atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1306517 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #999 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/999/)
          HDFS-3156. TestDFSHAAdmin is failing post HADOOP-8202. Contributed by Aaron T. Myers. (Revision 1306517)

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

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #999 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/999/ ) HDFS-3156 . TestDFSHAAdmin is failing post HADOOP-8202 . Contributed by Aaron T. Myers. (Revision 1306517) Result = FAILURE atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1306517 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1035 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1035/)
          HDFS-3156. TestDFSHAAdmin is failing post HADOOP-8202. Contributed by Aaron T. Myers. (Revision 1306517)

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

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1035 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1035/ ) HDFS-3156 . TestDFSHAAdmin is failing post HADOOP-8202 . Contributed by Aaron T. Myers. (Revision 1306517) Result = SUCCESS atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1306517 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSHAAdmin.java
          Hide
          Todd Lipcon added a comment -

          This patch also introduced the following bug: if the proxy.close() function throws an IOException, then HadoopIllegalArgumentException will be thrown, claiming that the proxy doesn't implement Closeable. This is the wrong error to throw, and is a regression in behavior (failure to close due to IOE should just be a warning, as it was previously). Hari, would you mind fixing this?

          Show
          Todd Lipcon added a comment - This patch also introduced the following bug: if the proxy.close() function throws an IOException, then HadoopIllegalArgumentException will be thrown, claiming that the proxy doesn't implement Closeable. This is the wrong error to throw, and is a regression in behavior (failure to close due to IOE should just be a warning, as it was previously). Hari, would you mind fixing this?
          Hide
          Hari Mankude added a comment -

          Will take a look.

          Show
          Hari Mankude added a comment - Will take a look.

            People

            • Assignee:
              Hari Mankude
              Reporter:
              Hari Mankude
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development