Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8472 )

Change subject: IMPALA-4671: (part-2) Replace kudu::ServicePool with one that 
uses Impala threads
......................................................................


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8472/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8472/4//COMMIT_MSG@33
PS4, Line 33: This patch however does not display instrumentation on the 
Webpages yet.
            : In a future patch, we will add that through the ImpalaServicePool 
and the
            : RpcMgr.
> can you file a JIRA for that (and you could reference it here).
Done


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.h@65
PS4, Line 65:   // TODO: Display these metrics in the debug webpage. IMPALA-6269
            :   // Number of RPCs that timed out while waiting in the service 
queue.
            :   AtomicInt32 rpcs_timed_out_in_queue_;
            :   // Number of RPCs that were rejected due to the queue being 
full.
            :   AtomicInt32 rpcs_queue_overflow_;
> these aren't used by anything yet, right? let's add a TODO with a JIRA numb
Done


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.h@75
PS4, Line 75:
> comment on what that synchronizes
Done. Since the lock theoretically seems unnecessary, I've left a TODO to 
consider removing it later.


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/rpc/impala-service-pool.cc@40
PS4, Line 40:
            : METRIC_DEFINE_histogram(server, impala_unused,
            :     "RPC Queue Time",
            :     kudu::MetricUnit::kMic
> include names.h instead
Done


http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

http://gerrit.cloudera.org:8080/#/c/8472/4/be/src/service/impalad-main.cc@58
PS4, Line 58:
> why do we have that now?
It's not needed. Removed.



--
To view, visit http://gerrit.cloudera.org:8080/8472
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3e0fa9ae22c4803b0cea736eb35e333e2671448
Gerrit-Change-Number: 8472
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Dec 2017 21:35:20 +0000
Gerrit-HasComments: Yes

Reply via email to