Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage ...................................................................... Patch Set 14: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/9292/8/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/9292/8/be/src/rpc/impala-service-pool.cc@76 PS8, Line 76: } : : ImpalaServicePool::~ImpalaServic Maybe this is something to consider moving into RpcMethodInfo as a Kudu patch. In that case, we can get it like we get the handler latency in L273. Not for now, but if you agree, we can add a TODO here. http://gerrit.cloudera.org:8080/#/c/9292/8/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9292/8/be/src/rpc/rpc-mgr.cc@128 PS8, Line 128: rviceIf* service_p This might stop us from working with different types of ServiceIfs in the future if we need to. But since we don't have any other types of ServiceIfs, it's okay for now. Something to keep a note of though. http://gerrit.cloudera.org:8080/#/c/9292/14/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/9292/14/tests/webserver/test_web_pages.py@251 PS14, Line 251: 'impala.DataStreamService' Why not keep this in a local array? Then we can add more services to this test as we add them to the code base. Then we can remove all references to 'DataStreamService' and use the array entry. http://gerrit.cloudera.org:8080/#/c/9292/8/www/rpcz.tmpl File www/rpcz.tmpl: http://gerrit.cloudera.org:8080/#/c/9292/8/www/rpcz.tmpl@42 PS8, Line 42: <tr> : <th>Queue Size</th> : <th>Idle threads</th> : <th>Maximum Queue Size</th> : <th>RPCs rejected due to queue overflow</th> : </tr> : <tr> : <td id="{{service_name}}_queue_size">{{queue_size}}</td> : <td id="{{service_name}}_idle_threads">{{idle_threads}}</td> : <td id="{{service_name}}_service_max_queue_size">{{service_max_queue_size}}</td> : </td> : <td id="{{service_name}}_rpcs_queue_overflow">{{rpcs_queue_overflow}}</td> : </tr> Would specifying a colspan for these guys align them better? -- To view, visit http://gerrit.cloudera.org:8080/9292 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f Gerrit-Change-Number: 9292 Gerrit-PatchSet: 14 Gerrit-Owner: Lars Volker <l...@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: Tue, 20 Feb 2018 00:48:29 +0000 Gerrit-HasComments: Yes
