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

Clients calling remote services returning BoolProto ignores false values

    Details

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

      Description

      Some services like QueryMasterProtocolService returns false sometimes. But clients are just ignoring them.

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user navis opened a pull request:

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

        TAJO-1409 Clients calling remote services returning BoolProto ignores false values

        Now server throws exception to client instead of returning false.

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

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

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

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


        commit 94ff284d751cd6cbdeede432b118812e9201cca3
        Author: navis.ryu <navis@apache.org>
        Date: 2015-03-18T06:19:33Z

        TAJO-1409 Clients calling remote services returning BoolProto ignores false values


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user navis opened a pull request: https://github.com/apache/tajo/pull/430 TAJO-1409 Clients calling remote services returning BoolProto ignores false values Now server throws exception to client instead of returning false. You can merge this pull request into a Git repository by running: $ git pull https://github.com/navis/tajo TAJO-1409 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tajo/pull/430.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 #430 commit 94ff284d751cd6cbdeede432b118812e9201cca3 Author: navis.ryu <navis@apache.org> Date: 2015-03-18T06:19:33Z TAJO-1409 Clients calling remote services returning BoolProto ignores false values
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user navis commented on the pull request:

        https://github.com/apache/tajo/pull/430#issuecomment-83847432

        rebased to master

        Show
        githubbot ASF GitHub Bot added a comment - Github user navis commented on the pull request: https://github.com/apache/tajo/pull/430#issuecomment-83847432 rebased to master
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user navis commented on the pull request:

        https://github.com/apache/tajo/pull/430#issuecomment-85253949

        Rebased to master

        Show
        githubbot ASF GitHub Bot added a comment - Github user navis commented on the pull request: https://github.com/apache/tajo/pull/430#issuecomment-85253949 Rebased to master
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/tajo/pull/430#discussion_r28201248

        — Diff: tajo-core/src/test/java/org/apache/tajo/querymaster/TestKillQuery.java —
        @@ -63,7 +63,7 @@
        private static TajoTestingCluster cluster;
        private static TajoConf conf;
        private static TajoClient client;

        • private static String queryStr = "select t1.l_orderkey, t1.l_partkey, t2.c_custkey " +
          + private static String queryStr = "select t1.l_orderkey, t1.l_partkey, t2.c_custkey, sleep(1) " +
            • End diff –

        The problem of a test failure was fixed in https://issues.apache.org/jira/browse/TAJO-1138 and https://issues.apache.org/jira/browse/TAJO-1440.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/430#discussion_r28201248 — Diff: tajo-core/src/test/java/org/apache/tajo/querymaster/TestKillQuery.java — @@ -63,7 +63,7 @@ private static TajoTestingCluster cluster; private static TajoConf conf; private static TajoClient client; private static String queryStr = "select t1.l_orderkey, t1.l_partkey, t2.c_custkey " + + private static String queryStr = "select t1.l_orderkey, t1.l_partkey, t2.c_custkey, sleep(1) " + End diff – The problem of a test failure was fixed in https://issues.apache.org/jira/browse/TAJO-1138 and https://issues.apache.org/jira/browse/TAJO-1440 .
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/tajo/pull/430#discussion_r28201388

        — Diff: tajo-core/src/main/java/org/apache/tajo/querymaster/QueryMasterManagerService.java —
        @@ -168,7 +170,7 @@ public void statusUpdate(RpcController controller, TajoWorkerProtocol.TaskStatus
        done.run(TajoWorker.TRUE_PROTO);
        — End diff –

        This line is also meaningless.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/430#discussion_r28201388 — Diff: tajo-core/src/main/java/org/apache/tajo/querymaster/QueryMasterManagerService.java — @@ -168,7 +170,7 @@ public void statusUpdate(RpcController controller, TajoWorkerProtocol.TaskStatus done.run(TajoWorker.TRUE_PROTO); — End diff – This line is also meaningless.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/430#issuecomment-92003200

        +1
        Thanks @navis. Your patch looks good to me.
        I left trivial comments. If you don't mind, I'll reflect them before commit.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/430#issuecomment-92003200 +1 Thanks @navis. Your patch looks good to me. I left trivial comments. If you don't mind, I'll reflect them before commit.
        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/430#discussion_r28392283

        — Diff: tajo-core/src/test/java/org/apache/tajo/querymaster/TestKillQuery.java —
        @@ -63,7 +63,7 @@
        private static TajoTestingCluster cluster;
        private static TajoConf conf;
        private static TajoClient client;

        • private static String queryStr = "select t1.l_orderkey, t1.l_partkey, t2.c_custkey " +
          + private static String queryStr = "select t1.l_orderkey, t1.l_partkey, t2.c_custkey, sleep(1) " +
            • End diff –

        good. I'll remove this line

        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/430#discussion_r28392283 — Diff: tajo-core/src/test/java/org/apache/tajo/querymaster/TestKillQuery.java — @@ -63,7 +63,7 @@ private static TajoTestingCluster cluster; private static TajoConf conf; private static TajoClient client; private static String queryStr = "select t1.l_orderkey, t1.l_partkey, t2.c_custkey " + + private static String queryStr = "select t1.l_orderkey, t1.l_partkey, t2.c_custkey, sleep(1) " + End diff – good. I'll remove this line
        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/430#discussion_r28392403

        — Diff: tajo-core/src/main/java/org/apache/tajo/querymaster/QueryMasterManagerService.java —
        @@ -168,7 +170,7 @@ public void statusUpdate(RpcController controller, TajoWorkerProtocol.TaskStatus
        done.run(TajoWorker.TRUE_PROTO);
        — End diff –

        I cannot understand this. Is it?

        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/430#discussion_r28392403 — Diff: tajo-core/src/main/java/org/apache/tajo/querymaster/QueryMasterManagerService.java — @@ -168,7 +170,7 @@ public void statusUpdate(RpcController controller, TajoWorkerProtocol.TaskStatus done.run(TajoWorker.TRUE_PROTO); — End diff – I cannot understand this. Is it?
        Hide
        githubbot ASF GitHub Bot added a comment -

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

        https://github.com/apache/tajo/pull/430#discussion_r28409251

        — Diff: tajo-core/src/main/java/org/apache/tajo/querymaster/QueryMasterManagerService.java —
        @@ -168,7 +170,7 @@ public void statusUpdate(RpcController controller, TajoWorkerProtocol.TaskStatus
        done.run(TajoWorker.TRUE_PROTO);
        — End diff –

        I mean, we can assume that the remote call is successfully finished if the InvocationFailure does not occur.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/430#discussion_r28409251 — Diff: tajo-core/src/main/java/org/apache/tajo/querymaster/QueryMasterManagerService.java — @@ -168,7 +170,7 @@ public void statusUpdate(RpcController controller, TajoWorkerProtocol.TaskStatus done.run(TajoWorker.TRUE_PROTO); — End diff – I mean, we can assume that the remote call is successfully finished if the InvocationFailure does not occur.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jinossy commented on the pull request:

        https://github.com/apache/tajo/pull/430#issuecomment-93348821

        @jihoonson
        done.run must be called once. If throwing exception is allowed, I think that we need to handle all case error instead of InvocationFailure

        https://issues.apache.org/jira/browse/TAJO-292?focusedCommentId=13838973&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13838973

        @navis
        Thank you for your contribution. the patch is very useful but It seems to need failure test
        Could you add unit test ?

        Show
        githubbot ASF GitHub Bot added a comment - Github user jinossy commented on the pull request: https://github.com/apache/tajo/pull/430#issuecomment-93348821 @jihoonson done.run must be called once. If throwing exception is allowed, I think that we need to handle all case error instead of InvocationFailure https://issues.apache.org/jira/browse/TAJO-292?focusedCommentId=13838973&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13838973 @navis Thank you for your contribution. the patch is very useful but It seems to need failure test Could you add unit test ?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user navis commented on the pull request:

        https://github.com/apache/tajo/pull/430#issuecomment-93379829

        @jinossy you are right. Because clients are calling this APIs with null controller, exception made in server side is not rethrown to client. Seemed need some more works.

        Show
        githubbot ASF GitHub Bot added a comment - Github user navis commented on the pull request: https://github.com/apache/tajo/pull/430#issuecomment-93379829 @jinossy you are right. Because clients are calling this APIs with null controller, exception made in server side is not rethrown to client. Seemed need some more works.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user navis commented on the pull request:

        https://github.com/apache/tajo/pull/430#issuecomment-93400842

        @jinossy I have a question.
        ````
        NettyClientBase client = getQueryMasterConnection();
        try

        { QueryMasterProtocol.QueryMasterProtocolService.Interface stub = client.getStub(); stub.doneExecutionBlock(null, reporter, NullCallback.get()); }

        finally

        { connPool.releaseConnection(client); }

        ````
        thread calling stub.doneExecutionBlock() is exiting when it ends flushing packet to wire. Is it possible connPool.releaseConnection(client) to close the connection prematurely? Should we release connection when callback is called (with whatever from server)?

        Show
        githubbot ASF GitHub Bot added a comment - Github user navis commented on the pull request: https://github.com/apache/tajo/pull/430#issuecomment-93400842 @jinossy I have a question. ```` NettyClientBase client = getQueryMasterConnection(); try { QueryMasterProtocol.QueryMasterProtocolService.Interface stub = client.getStub(); stub.doneExecutionBlock(null, reporter, NullCallback.get()); } finally { connPool.releaseConnection(client); } ```` thread calling stub.doneExecutionBlock() is exiting when it ends flushing packet to wire. Is it possible connPool.releaseConnection(client) to close the connection prematurely? Should we release connection when callback is called (with whatever from server)?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jinossy commented on the pull request:

        https://github.com/apache/tajo/pull/430#issuecomment-93418802

        @navis
        We don’t need release. Actually, releaseConnection is exception cleanup for reconnection.
        some kind of codes is my fault. I've remove the legacy code in TAJO-1497

        Show
        githubbot ASF GitHub Bot added a comment - Github user jinossy commented on the pull request: https://github.com/apache/tajo/pull/430#issuecomment-93418802 @navis We don’t need release. Actually, releaseConnection is exception cleanup for reconnection. some kind of codes is my fault. I've remove the legacy code in TAJO-1497
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/430#issuecomment-93610381

        Thanks @jinossy for addressing missing points.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/430#issuecomment-93610381 Thanks @jinossy for addressing missing points.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jinossy commented on the pull request:

        https://github.com/apache/tajo/pull/430#issuecomment-93941591

        @navis
        I will fix network hangs by TAJO-1563

        Show
        githubbot ASF GitHub Bot added a comment - Github user jinossy commented on the pull request: https://github.com/apache/tajo/pull/430#issuecomment-93941591 @navis I will fix network hangs by TAJO-1563
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jinossy commented on the pull request:

        https://github.com/apache/tajo/pull/430#issuecomment-97008145

        If you set an error message in controller, Async CallFuture.get() can throw exception.

        like this example :
        ```
        @Override
        public void done(RpcController controller, TajoWorkerProtocol.TaskCompletionReport report,
        RpcCallback<PrimitiveProtos.BoolProto> done) {
        try {
        QueryMasterTask queryMasterTask = queryMaster.getQueryMasterTask(
        new QueryId(report.getId().getTaskId().getExecutionBlockId().getQueryId()));
        if (queryMasterTask != null)

        { queryMasterTask.getEventHandler().handle(new TaskCompletionEvent(report)); }

        done.run(TajoWorker.TRUE_PROTO);
        } catch (Exception e)

        { LOG.error(e.getMessage(), e); controller.setFailed(e.getMessage()); done.run(TajoWorker.FALSE_PROTO); }

        }
        ```

        Show
        githubbot ASF GitHub Bot added a comment - Github user jinossy commented on the pull request: https://github.com/apache/tajo/pull/430#issuecomment-97008145 If you set an error message in controller, Async CallFuture.get() can throw exception. like this example : ``` @Override public void done(RpcController controller, TajoWorkerProtocol.TaskCompletionReport report, RpcCallback<PrimitiveProtos.BoolProto> done) { try { QueryMasterTask queryMasterTask = queryMaster.getQueryMasterTask( new QueryId(report.getId().getTaskId().getExecutionBlockId().getQueryId())); if (queryMasterTask != null) { queryMasterTask.getEventHandler().handle(new TaskCompletionEvent(report)); } done.run(TajoWorker.TRUE_PROTO); } catch (Exception e) { LOG.error(e.getMessage(), e); controller.setFailed(e.getMessage()); done.run(TajoWorker.FALSE_PROTO); } } ```
        Hide
        jhkim Jinho Kim added a comment -

        Solved by TAJO-1563, TAJO-1584

        Show
        jhkim Jinho Kim added a comment - Solved by TAJO-1563 , TAJO-1584
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jinossy commented on the pull request:

        https://github.com/apache/tajo/pull/430#issuecomment-99730546

        Solved by TAJO-1563, TAJO-1584
        please close this PR.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jinossy commented on the pull request: https://github.com/apache/tajo/pull/430#issuecomment-99730546 Solved by TAJO-1563 , TAJO-1584 please close this PR.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

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

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development