Uploaded image for project: 'Tajo'
  1. Tajo
  2. TAJO-1569

BlockingRpcClient can make other request fail

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.9.0, 0.10.0
    • Fix Version/s: 0.11.0
    • Component/s: Java Client, RPC
    • Labels:
      None

      Description

      A callback is set to fail but It rethrow exception and then other normal request is set to fail.

      Tests run: 23, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 31.131 sec <<< FAILURE! - in org.apache.tajo.client.TestTajoClient
      testSelectDatabaseToInvalidOne(org.apache.tajo.client.TestTajoClient)  Time elapsed: 0.034 sec  <<< ERROR!
      com.google.protobuf.ServiceException: org.apache.tajo.rpc.TajoServiceException: org.apache.tajo.ipc.TajoMasterClientProtocol(127.0.0.1:33642): com.google.protobuf.ServiceException: org.apache.tajo.catalog.exception.NoSuchDatabaseException: ERROR: database "invaliddatabase" does not exist
      	at org.apache.tajo.rpc.BlockingRpcClient$ProxyRpcChannel.callBlockingMethod(BlockingRpcClient.java:121)
      	at org.apache.tajo.ipc.TajoMasterClientProtocol$TajoMasterClientProtocolService$BlockingStub.getAllDatabases(TajoMasterClientProtocol.java:2163)
      	at org.apache.tajo.client.CatalogAdminClientImpl$4.call(CatalogAdminClientImpl.java:106)
      	at org.apache.tajo.client.CatalogAdminClientImpl$4.call(CatalogAdminClientImpl.java:100)
      	at org.apache.tajo.rpc.ServerCallable.withRetries(ServerCallable.java:86)
      	at org.apache.tajo.client.CatalogAdminClientImpl.getAllDatabaseNames(CatalogAdminClientImpl.java:99)
      	at org.apache.tajo.client.TajoClientImpl.getAllDatabaseNames(TajoClientImpl.java:194)
      	at org.apache.tajo.client.TestTajoClient.testSelectDatabaseToInvalidOne(TestTajoClient.java:145)
      

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user jinossy opened a pull request:

          https://github.com/apache/tajo/pull/541

          TAJO-1569: BlockingRpcClient can make other request fail

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/jinossy/tajo TAJO-1569

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tajo/pull/541.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #541


          commit c724259fbcdaaa1f52a098a20f5f774416c2a4d7
          Author: Jinho Kim <jhkim@apache.org>
          Date: 2015-04-18T05:13:25Z

          TAJO-1569: BlockingRpcClient can make other request fail


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user jinossy opened a pull request: https://github.com/apache/tajo/pull/541 TAJO-1569 : BlockingRpcClient can make other request fail You can merge this pull request into a Git repository by running: $ git pull https://github.com/jinossy/tajo TAJO-1569 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tajo/pull/541.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #541 commit c724259fbcdaaa1f52a098a20f5f774416c2a4d7 Author: Jinho Kim <jhkim@apache.org> Date: 2015-04-18T05:13:25Z TAJO-1569 : BlockingRpcClient can make other request fail
          Hide
          tajoqa Tajo QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12726344/TAJO-1569.patch
          against master revision release-0.9.0-rc0-260-ga745385.

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

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

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The applied patch does not increase the total number of javadoc warnings.

          +1 checkstyle. The patch generated 0 code style errors.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 tajo-rpc/tajo-rpc-protobuf.

          Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/743//testReport/
          Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/743//console

          This message is automatically generated.

          Show
          tajoqa Tajo QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12726344/TAJO-1569.patch against master revision release-0.9.0-rc0-260-ga745385. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The applied patch does not increase the total number of javadoc warnings. +1 checkstyle. The patch generated 0 code style errors. +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 tajo-rpc/tajo-rpc-protobuf. Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/743//testReport/ Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/743//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/541#discussion_r28641879

          — Diff: tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java —
          @@ -188,10 +187,12 @@ protected void channelRead0(ChannelHandlerContext ctx, RpcResponse rpcResponse)
          @Override
          public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)
          throws Exception {
          + /* Current requests will be failed */
          for(ProtoCallFuture callback: requests.values())

          { callback.setFailed(cause.getMessage(), cause); }
          • + requests.clear();

              • End diff –

          This change will remove other request handles, prohibiting subsequent responses. It should be done only if we cannot able to recover this rpc connection. I'm not sure that ``exceptionCaught`` means a unrecoverable situation.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/541#discussion_r28641879 — Diff: tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java — @@ -188,10 +187,12 @@ protected void channelRead0(ChannelHandlerContext ctx, RpcResponse rpcResponse) @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { + /* Current requests will be failed */ for(ProtoCallFuture callback: requests.values()) { callback.setFailed(cause.getMessage(), cause); } + requests.clear(); End diff – This change will remove other request handles, prohibiting subsequent responses. It should be done only if we cannot able to recover this rpc connection. I'm not sure that ``exceptionCaught`` means a unrecoverable situation.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on the pull request:

          https://github.com/apache/tajo/pull/541#issuecomment-94134783

          Even though It is trivial change, it seems to be a critical issue. Other changes look good to me. I left one comment.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/541#issuecomment-94134783 Even though It is trivial change, it seems to be a critical issue. Other changes look good to me. I left one comment.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jinossy commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/541#discussion_r28641985

          — Diff: tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java —
          @@ -188,10 +187,12 @@ protected void channelRead0(ChannelHandlerContext ctx, RpcResponse rpcResponse)
          @Override
          public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)
          throws Exception {
          + /* Current requests will be failed */
          for(ProtoCallFuture callback: requests.values())

          { callback.setFailed(cause.getMessage(), cause); }
          • + requests.clear();

              • End diff –

          This case is fatal error and It will be internal error.
          All request is set to fail and then requests should be removed.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jinossy commented on a diff in the pull request: https://github.com/apache/tajo/pull/541#discussion_r28641985 — Diff: tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java — @@ -188,10 +187,12 @@ protected void channelRead0(ChannelHandlerContext ctx, RpcResponse rpcResponse) @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { + /* Current requests will be failed */ for(ProtoCallFuture callback: requests.values()) { callback.setFailed(cause.getMessage(), cause); } + requests.clear(); End diff – This case is fatal error and It will be internal error. All request is set to fail and then requests should be removed.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/541#discussion_r28642056

          — Diff: tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java —
          @@ -188,10 +187,12 @@ protected void channelRead0(ChannelHandlerContext ctx, RpcResponse rpcResponse)
          @Override
          public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)
          throws Exception {
          + /* Current requests will be failed */
          for(ProtoCallFuture callback: requests.values())

          { callback.setFailed(cause.getMessage(), cause); }
          • + requests.clear();

              • End diff –

          I missed the above line, which marks all requests as 'failed'. This code also seem to be based on the assumption that exceptionCaught does not handle any recoverable exception. If it is right, please go ahead. Otherwise, we can fix the bug of TAJO-1569 right now, and we can handle this problem in another jira.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/541#discussion_r28642056 — Diff: tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java — @@ -188,10 +187,12 @@ protected void channelRead0(ChannelHandlerContext ctx, RpcResponse rpcResponse) @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { + /* Current requests will be failed */ for(ProtoCallFuture callback: requests.values()) { callback.setFailed(cause.getMessage(), cause); } + requests.clear(); End diff – I missed the above line, which marks all requests as 'failed'. This code also seem to be based on the assumption that exceptionCaught does not handle any recoverable exception. If it is right, please go ahead. Otherwise, we can fix the bug of TAJO-1569 right now, and we can handle this problem in another jira.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jinossy commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/541#discussion_r28642133

          — Diff: tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java —
          @@ -188,10 +187,12 @@ protected void channelRead0(ChannelHandlerContext ctx, RpcResponse rpcResponse)
          @Override
          public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)
          throws Exception {
          + /* Current requests will be failed */
          for(ProtoCallFuture callback: requests.values())

          { callback.setFailed(cause.getMessage(), cause); }
          • + requests.clear();

              • End diff –

          You are right, exceptionCaught do not received recoverable exception(RemoteCallException) but if we do handle the recoverable exception, It would be better.
          I will add more exception handling in TAJO-1563

          Show
          githubbot ASF GitHub Bot added a comment - Github user jinossy commented on a diff in the pull request: https://github.com/apache/tajo/pull/541#discussion_r28642133 — Diff: tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java — @@ -188,10 +187,12 @@ protected void channelRead0(ChannelHandlerContext ctx, RpcResponse rpcResponse) @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { + /* Current requests will be failed */ for(ProtoCallFuture callback: requests.values()) { callback.setFailed(cause.getMessage(), cause); } + requests.clear(); End diff – You are right, exceptionCaught do not received recoverable exception(RemoteCallException) but if we do handle the recoverable exception, It would be better. I will add more exception handling in TAJO-1563
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on a diff in the pull request:

          https://github.com/apache/tajo/pull/541#discussion_r28642139

          — Diff: tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java —
          @@ -188,10 +187,12 @@ protected void channelRead0(ChannelHandlerContext ctx, RpcResponse rpcResponse)
          @Override
          public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause)
          throws Exception {
          + /* Current requests will be failed */
          for(ProtoCallFuture callback: requests.values())

          { callback.setFailed(cause.getMessage(), cause); }
          • + requests.clear();

              • End diff –

          agreed

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/541#discussion_r28642139 — Diff: tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java — @@ -188,10 +187,12 @@ protected void channelRead0(ChannelHandlerContext ctx, RpcResponse rpcResponse) @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { + /* Current requests will be failed */ for(ProtoCallFuture callback: requests.values()) { callback.setFailed(cause.getMessage(), cause); } + requests.clear(); End diff – agreed
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on the pull request:

          https://github.com/apache/tajo/pull/541#issuecomment-94138694

          +1 Ship it

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/541#issuecomment-94138694 +1 Ship it
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jinossy commented on the pull request:

          https://github.com/apache/tajo/pull/541#issuecomment-94138746

          Thank you for the review.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jinossy commented on the pull request: https://github.com/apache/tajo/pull/541#issuecomment-94138746 Thank you for the review.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jinossy commented on the pull request:

          https://github.com/apache/tajo/pull/541#issuecomment-94140892

          branch-0.10.1 is required TAJO-1497
          I will create new jira task

          Show
          githubbot ASF GitHub Bot added a comment - Github user jinossy commented on the pull request: https://github.com/apache/tajo/pull/541#issuecomment-94140892 branch-0.10.1 is required TAJO-1497 I will create new jira task
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tajo/pull/541

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tajo/pull/541
          Hide
          jhkim Jinho Kim added a comment -

          Committed it to master branch.

          Show
          jhkim Jinho Kim added a comment - Committed it to master branch.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Tajo-master-CODEGEN-build #313 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/313/)
          TAJO-1569: BlockingRpcClient can make other request fail. (jinho) (jhkim: rev 333b6f784a7f237752f5d93b9235b2468aea3471)

          • CHANGES
          • tajo-rpc/tajo-rpc-protobuf/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java
          • tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Tajo-master-CODEGEN-build #313 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/313/ ) TAJO-1569 : BlockingRpcClient can make other request fail. (jinho) (jhkim: rev 333b6f784a7f237752f5d93b9235b2468aea3471) CHANGES tajo-rpc/tajo-rpc-protobuf/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Tajo-master-build #675 (See https://builds.apache.org/job/Tajo-master-build/675/)
          TAJO-1569: BlockingRpcClient can make other request fail. (jinho) (jhkim: rev 333b6f784a7f237752f5d93b9235b2468aea3471)

          • tajo-rpc/tajo-rpc-protobuf/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java
          • CHANGES
          • tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Tajo-master-build #675 (See https://builds.apache.org/job/Tajo-master-build/675/ ) TAJO-1569 : BlockingRpcClient can make other request fail. (jinho) (jhkim: rev 333b6f784a7f237752f5d93b9235b2468aea3471) tajo-rpc/tajo-rpc-protobuf/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java CHANGES tajo-rpc/tajo-rpc-protobuf/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java

            People

            • Assignee:
              jhkim Jinho Kim
              Reporter:
              jhkim Jinho Kim
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development