Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage ...................................................................... Patch Set 11: (10 comments) http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/impala-service-pool.h File be/src/rpc/impala-service-pool.h: http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/impala-service-pool.h@83 PS11, Line 83: map Will an unordered_map suffice ? It seems to be more performant. http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/impala-service-pool.h@86 PS11, Line 86: IntCounter* rpcs_queue_overflow_; = nullptr http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/impala-service-pool.cc@71 PS11, Line 71: const kudu::rpc::GeneratedServiceIf::MethodInfoMap& method_infos = : service_->methods_by_name(); : for (const auto& method : method_infos) { These lines can be combined into: for (const auto& method : service_->methods_by_name()) { http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/impala-service-pool.cc@76 PS11, Line 76: new HistogramMetric( Does this need to be defined in metrics.json too ? http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/impala-service-pool.cc@246 PS11, Line 246: Value service_name_val(service_name().c_str(), document->GetAllocator()); nit: indent wrong http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/rpc-mgr.h@183 PS11, Line 183: Not owned. shared_ptr<> means co-ownership no ? http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/rpc-trace.cc File be/src/rpc/rpc-trace.cc: http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/rpc-trace.cc@147 PS11, Line 147: MetricGroup* metrics Should callers now be passing the exec_env_->rpc_metrics() so all RPC related metrics are under the newly created "RPC" metric group ? http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/util/histogram-metric.h File be/src/util/histogram-metric.h: http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/util/histogram-metric.h@96 PS11, Line 96: // nit: /// http://gerrit.cloudera.org:8080/#/c/9292/11/tests/custom_cluster/test_krpc_metrics.py File tests/custom_cluster/test_krpc_metrics.py: http://gerrit.cloudera.org:8080/#/c/9292/11/tests/custom_cluster/test_krpc_metrics.py@60 PS11, Line 60: [0] This may not be necessary now but it'd be better to not assume that DataStreamService is always the only entry in rpcz['services']. May make sense to look it the entry in question by matching against 'service_name'. http://gerrit.cloudera.org:8080/#/c/9292/11/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/9292/11/tests/webserver/test_web_pages.py@250 PS11, Line 250: rpcz['services'][0]['rpc_method_metrics'] same comment as above -- 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: 11 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: Mon, 19 Feb 2018 06:32:40 +0000 Gerrit-HasComments: Yes
