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

RpcConnectionPool should check reference counter of connection before close

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.11.0
    • Component/s: RPC
    • Labels:
      None

      Description

      Connections in the pool is shared one and should be closed only when it's not referenced by other threads. Furthermore, current pool implementation locks whole connections for connecting/closing a connection, making bad interferences on other operations (on sane connection).

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user navis opened a pull request:

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

          TAJO-1391 RpcConnectionPool should check reference counter of the connection before close

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

          $ git pull https://github.com/navis/tajo TAJO-1391

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

          https://github.com/apache/tajo/pull/412.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 #412


          commit 6b0d2d52ac68d763b184707eaa3b2b5d1f8eb6b7
          Author: navis.ryu <navis@apache.org>
          Date: 2015-03-11T06:41:22Z

          TAJO-1391 RpcConnectionPool should check reference counter of the connection before close


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user navis opened a pull request: https://github.com/apache/tajo/pull/412 TAJO-1391 RpcConnectionPool should check reference counter of the connection before close You can merge this pull request into a Git repository by running: $ git pull https://github.com/navis/tajo TAJO-1391 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tajo/pull/412.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 #412 commit 6b0d2d52ac68d763b184707eaa3b2b5d1f8eb6b7 Author: navis.ryu <navis@apache.org> Date: 2015-03-11T06:41:22Z TAJO-1391 RpcConnectionPool should check reference counter of the connection before close
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hyunsik commented on the pull request:

          https://github.com/apache/tajo/pull/412#issuecomment-78406823

          @ykrips,

          You may be the best review for this issue. Could you review this patch if you are ok?

          Show
          githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/412#issuecomment-78406823 @ykrips, You may be the best review for this issue. Could you review this patch if you are ok?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ykrips commented on the pull request:

          https://github.com/apache/tajo/pull/412#issuecomment-78418695

          Sure. I will review it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ykrips commented on the pull request: https://github.com/apache/tajo/pull/412#issuecomment-78418695 Sure. I will review it.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tajo/pull/412#discussion_r26302284

          — Diff: tajo-rpc/src/main/java/org/apache/tajo/rpc/AsyncRpcClient.java —
          @@ -113,16 +87,11 @@ public void close() {
          }

          private class ProxyRpcChannel implements RpcChannel {

          • private final ClientChannelInboundHandler handler;
            -
          • public ProxyRpcChannel() {
          • this.handler = getChannel().pipeline()
          • .get(ClientChannelInboundHandler.class);
            + private ClientChannelInboundHandler handler;
          • if (handler == null) { - throw new IllegalArgumentException("Channel does not have " + - "proper handler"); - }

            + private ClientChannelInboundHandler handler() {
            + return handler == null ? handler = getChannel().pipeline()

              • End diff –

          getChannel() function has a possibility to return null when channelfuture is null, so it needs check null value.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ykrips commented on a diff in the pull request: https://github.com/apache/tajo/pull/412#discussion_r26302284 — Diff: tajo-rpc/src/main/java/org/apache/tajo/rpc/AsyncRpcClient.java — @@ -113,16 +87,11 @@ public void close() { } private class ProxyRpcChannel implements RpcChannel { private final ClientChannelInboundHandler handler; - public ProxyRpcChannel() { this.handler = getChannel().pipeline() .get(ClientChannelInboundHandler.class); + private ClientChannelInboundHandler handler; if (handler == null) { - throw new IllegalArgumentException("Channel does not have " + - "proper handler"); - } + private ClientChannelInboundHandler handler() { + return handler == null ? handler = getChannel().pipeline() End diff – getChannel() function has a possibility to return null when channelfuture is null, so it needs check null value.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tajo/pull/412#discussion_r26304267

          — Diff: tajo-rpc/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java —
          @@ -252,8 +257,11 @@ public void testConnectionFailed() throws Exception {

          try {
          int port = server.getListenAddress().getPort() + 1;

          • client = new BlockingRpcClient(DummyProtocol.class,
          • RpcUtils.getConnectAddress(new InetSocketAddress("127.0.0.1", port)), retries);
            + RpcConnectionPool.RpcConnectionKey rpcConnectionKey =
            + new RpcConnectionPool.RpcConnectionKey(
            + RpcUtils.getConnectAddress(new InetSocketAddress("127.0.0.1", port)),
            + DummyProtocol.class, false);
            + client = new BlockingRpcClient(rpcConnectionKey, retries);
              • End diff –

          This code block does not throw ConnectException. how about add acquire connection on this?

          Show
          githubbot ASF GitHub Bot added a comment - Github user ykrips commented on a diff in the pull request: https://github.com/apache/tajo/pull/412#discussion_r26304267 — Diff: tajo-rpc/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java — @@ -252,8 +257,11 @@ public void testConnectionFailed() throws Exception { try { int port = server.getListenAddress().getPort() + 1; client = new BlockingRpcClient(DummyProtocol.class, RpcUtils.getConnectAddress(new InetSocketAddress("127.0.0.1", port)), retries); + RpcConnectionPool.RpcConnectionKey rpcConnectionKey = + new RpcConnectionPool.RpcConnectionKey( + RpcUtils.getConnectAddress(new InetSocketAddress("127.0.0.1", port)), + DummyProtocol.class, false); + client = new BlockingRpcClient(rpcConnectionKey, retries); End diff – This code block does not throw ConnectException. how about add acquire connection on this?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tajo/pull/412#discussion_r26304922

          — Diff: tajo-rpc/src/test/java/org/apache/tajo/rpc/TestAsyncRpc.java —
          @@ -125,8 +125,11 @@ public void setUpRpcServer() throws Exception {
          public void setUpRpcClient() throws Exception {
          retries = 1;

          • client = new AsyncRpcClient(DummyProtocol.class,
          • RpcUtils.getConnectAddress(server.getListenAddress()), retries);
            + RpcConnectionPool.RpcConnectionKey rpcConnectionKey =
            + new RpcConnectionPool.RpcConnectionKey(
            + RpcUtils.getConnectAddress(server.getListenAddress()),
            + DummyProtocol.class, true);
            + client = new AsyncRpcClient(rpcConnectionKey, retries);
              • End diff –

          It needs to acquire connection on this code block, or it can be initialized later on each test functions.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ykrips commented on a diff in the pull request: https://github.com/apache/tajo/pull/412#discussion_r26304922 — Diff: tajo-rpc/src/test/java/org/apache/tajo/rpc/TestAsyncRpc.java — @@ -125,8 +125,11 @@ public void setUpRpcServer() throws Exception { public void setUpRpcClient() throws Exception { retries = 1; client = new AsyncRpcClient(DummyProtocol.class, RpcUtils.getConnectAddress(server.getListenAddress()), retries); + RpcConnectionPool.RpcConnectionKey rpcConnectionKey = + new RpcConnectionPool.RpcConnectionKey( + RpcUtils.getConnectAddress(server.getListenAddress()), + DummyProtocol.class, true); + client = new AsyncRpcClient(rpcConnectionKey, retries); End diff – It needs to acquire connection on this code block, or it can be initialized later on each test functions.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tajo/pull/412#discussion_r26354442

          — Diff: tajo-rpc/src/main/java/org/apache/tajo/rpc/AsyncRpcClient.java —
          @@ -113,16 +87,11 @@ public void close() {
          }

          private class ProxyRpcChannel implements RpcChannel {

          • private final ClientChannelInboundHandler handler;
            -
          • public ProxyRpcChannel() {
          • this.handler = getChannel().pipeline()
          • .get(ClientChannelInboundHandler.class);
            + private ClientChannelInboundHandler handler;
          • if (handler == null) { - throw new IllegalArgumentException("Channel does not have " + - "proper handler"); - }

            + private ClientChannelInboundHandler handler() {
            + return handler == null ? handler = getChannel().pipeline()

              • End diff –

          The code part cannot be reached if this connection is not acquired, asserting the channel is established already. And callMethod() is already regarding getChannel() returns not-null value. I'll add RuntimeException on getChannel() if it can be null.

          Show
          githubbot ASF GitHub Bot added a comment - Github user navis commented on a diff in the pull request: https://github.com/apache/tajo/pull/412#discussion_r26354442 — Diff: tajo-rpc/src/main/java/org/apache/tajo/rpc/AsyncRpcClient.java — @@ -113,16 +87,11 @@ public void close() { } private class ProxyRpcChannel implements RpcChannel { private final ClientChannelInboundHandler handler; - public ProxyRpcChannel() { this.handler = getChannel().pipeline() .get(ClientChannelInboundHandler.class); + private ClientChannelInboundHandler handler; if (handler == null) { - throw new IllegalArgumentException("Channel does not have " + - "proper handler"); - } + private ClientChannelInboundHandler handler() { + return handler == null ? handler = getChannel().pipeline() End diff – The code part cannot be reached if this connection is not acquired, asserting the channel is established already. And callMethod() is already regarding getChannel() returns not-null value. I'll add RuntimeException on getChannel() if it can be null.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tajo/pull/412#discussion_r26354468

          — Diff: tajo-rpc/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java —
          @@ -252,8 +257,11 @@ public void testConnectionFailed() throws Exception {

          try {
          int port = server.getListenAddress().getPort() + 1;

          • client = new BlockingRpcClient(DummyProtocol.class,
          • RpcUtils.getConnectAddress(new InetSocketAddress("127.0.0.1", port)), retries);
            + RpcConnectionPool.RpcConnectionKey rpcConnectionKey =
            + new RpcConnectionPool.RpcConnectionKey(
            + RpcUtils.getConnectAddress(new InetSocketAddress("127.0.0.1", port)),
            + DummyProtocol.class, false);
            + client = new BlockingRpcClient(rpcConnectionKey, retries);
              • End diff –

          My bad. I'll fix that.

          Show
          githubbot ASF GitHub Bot added a comment - Github user navis commented on a diff in the pull request: https://github.com/apache/tajo/pull/412#discussion_r26354468 — Diff: tajo-rpc/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java — @@ -252,8 +257,11 @@ public void testConnectionFailed() throws Exception { try { int port = server.getListenAddress().getPort() + 1; client = new BlockingRpcClient(DummyProtocol.class, RpcUtils.getConnectAddress(new InetSocketAddress("127.0.0.1", port)), retries); + RpcConnectionPool.RpcConnectionKey rpcConnectionKey = + new RpcConnectionPool.RpcConnectionKey( + RpcUtils.getConnectAddress(new InetSocketAddress("127.0.0.1", port)), + DummyProtocol.class, false); + client = new BlockingRpcClient(rpcConnectionKey, retries); End diff – My bad. I'll fix that.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user navis commented on the pull request:

          https://github.com/apache/tajo/pull/412#issuecomment-78762327

          addressed comments and loosen locks further

          Show
          githubbot ASF GitHub Bot added a comment - Github user navis commented on the pull request: https://github.com/apache/tajo/pull/412#issuecomment-78762327 addressed comments and loosen locks further
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ykrips commented on the pull request:

          https://github.com/apache/tajo/pull/412#issuecomment-78837635

          Client does not check the connection status. Can you check this exception?
          ```
          com.google.protobuf.ServiceException: org.apache.tajo.rpc.TajoServiceException: java.nio.channels.ClosedChannelException
          at org.apache.tajo.rpc.BlockingRpcClient$ProxyRpcChannel.callBlockingMethod(BlockingRpcClient.java:115)
          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:94)
          at org.apache.tajo.client.CatalogAdminClientImpl.getAllDatabaseNames(CatalogAdminClientImpl.java:99)
          at org.apache.tajo.client.TajoClientImpl.getAllDatabaseNames(TajoClientImpl.java:186)
          at org.apache.tajo.client.TestTajoClient.testSelectDatabaseToInvalidOne(TestTajoClient.java:145)
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user ykrips commented on the pull request: https://github.com/apache/tajo/pull/412#issuecomment-78837635 Client does not check the connection status. Can you check this exception? ``` com.google.protobuf.ServiceException: org.apache.tajo.rpc.TajoServiceException: java.nio.channels.ClosedChannelException at org.apache.tajo.rpc.BlockingRpcClient$ProxyRpcChannel.callBlockingMethod(BlockingRpcClient.java:115) 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:94) at org.apache.tajo.client.CatalogAdminClientImpl.getAllDatabaseNames(CatalogAdminClientImpl.java:99) at org.apache.tajo.client.TajoClientImpl.getAllDatabaseNames(TajoClientImpl.java:186) at org.apache.tajo.client.TestTajoClient.testSelectDatabaseToInvalidOne(TestTajoClient.java:145) ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user navis commented on the pull request:

          https://github.com/apache/tajo/pull/412#issuecomment-78860785

          I cannot reproduce failure of the test.

          Show
          githubbot ASF GitHub Bot added a comment - Github user navis commented on the pull request: https://github.com/apache/tajo/pull/412#issuecomment-78860785 I cannot reproduce failure of the test.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ykrips commented on the pull request:

          https://github.com/apache/tajo/pull/412#issuecomment-81314231

          That testcase error might be come from insufficient system resources on my laptop. Sorry for giving confusion for this. give +1 to this issue, and will commit it to master branch.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ykrips commented on the pull request: https://github.com/apache/tajo/pull/412#issuecomment-81314231 That testcase error might be come from insufficient system resources on my laptop. Sorry for giving confusion for this. give +1 to this issue, and will commit it to master branch.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dongjoon-hyun commented on the pull request:

          https://github.com/apache/tajo/pull/412#issuecomment-81315870

          +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dongjoon-hyun commented on the pull request: https://github.com/apache/tajo/pull/412#issuecomment-81315870 +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tajo/pull/412
          Hide
          ykrips Jihun Kang added a comment -

          committed.

          Show
          ykrips Jihun Kang added a comment - committed.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Tajo-master-build #617 (See https://builds.apache.org/job/Tajo-master-build/617/)
          TAJO-1391: RpcConnectionPool should check reference counter of connection before close (jihun: rev 0dc7d68071dcf7c9d01dde8ed7598ca422e4c50c)

          • tajo-rpc/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java
          • tajo-rpc/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java
          • tajo-client/src/main/java/org/apache/tajo/client/SessionConnection.java
          • tajo-rpc/src/test/java/org/apache/tajo/rpc/TestAsyncRpc.java
          • tajo-rpc/src/main/java/org/apache/tajo/rpc/NettyClientBase.java
          • tajo-rpc/src/main/java/org/apache/tajo/rpc/AsyncRpcClient.java
          • tajo-rpc/src/main/java/org/apache/tajo/rpc/RpcConnectionPool.java
          • CHANGES
          • tajo-rpc/src/main/java/org/apache/tajo/rpc/RpcUtils.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Tajo-master-build #617 (See https://builds.apache.org/job/Tajo-master-build/617/ ) TAJO-1391 : RpcConnectionPool should check reference counter of connection before close (jihun: rev 0dc7d68071dcf7c9d01dde8ed7598ca422e4c50c) tajo-rpc/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java tajo-rpc/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java tajo-client/src/main/java/org/apache/tajo/client/SessionConnection.java tajo-rpc/src/test/java/org/apache/tajo/rpc/TestAsyncRpc.java tajo-rpc/src/main/java/org/apache/tajo/rpc/NettyClientBase.java tajo-rpc/src/main/java/org/apache/tajo/rpc/AsyncRpcClient.java tajo-rpc/src/main/java/org/apache/tajo/rpc/RpcConnectionPool.java CHANGES tajo-rpc/src/main/java/org/apache/tajo/rpc/RpcUtils.java
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Tajo-master-CODEGEN-build #254 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/254/)
          TAJO-1391: RpcConnectionPool should check reference counter of connection before close (jihun: rev 0dc7d68071dcf7c9d01dde8ed7598ca422e4c50c)

          • tajo-rpc/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java
          • tajo-rpc/src/test/java/org/apache/tajo/rpc/TestAsyncRpc.java
          • tajo-rpc/src/main/java/org/apache/tajo/rpc/RpcConnectionPool.java
          • tajo-client/src/main/java/org/apache/tajo/client/SessionConnection.java
          • CHANGES
          • tajo-rpc/src/main/java/org/apache/tajo/rpc/AsyncRpcClient.java
          • tajo-rpc/src/main/java/org/apache/tajo/rpc/RpcUtils.java
          • tajo-rpc/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java
          • tajo-rpc/src/main/java/org/apache/tajo/rpc/NettyClientBase.java
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Tajo-master-CODEGEN-build #254 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/254/ ) TAJO-1391 : RpcConnectionPool should check reference counter of connection before close (jihun: rev 0dc7d68071dcf7c9d01dde8ed7598ca422e4c50c) tajo-rpc/src/test/java/org/apache/tajo/rpc/TestBlockingRpc.java tajo-rpc/src/test/java/org/apache/tajo/rpc/TestAsyncRpc.java tajo-rpc/src/main/java/org/apache/tajo/rpc/RpcConnectionPool.java tajo-client/src/main/java/org/apache/tajo/client/SessionConnection.java CHANGES tajo-rpc/src/main/java/org/apache/tajo/rpc/AsyncRpcClient.java tajo-rpc/src/main/java/org/apache/tajo/rpc/RpcUtils.java tajo-rpc/src/main/java/org/apache/tajo/rpc/BlockingRpcClient.java tajo-rpc/src/main/java/org/apache/tajo/rpc/NettyClientBase.java

            People

            • Assignee:
              navis Navis
              Reporter:
              navis Navis
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development