You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Lars Volker (Code Review)" <ge...@cloudera.org> on 2018/02/13 05:03:59 UTC
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Lars Volker has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9292
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change is based on a change by Sailesh Mukil.
TODO: testing, once IMPALA-6508 has been submitted.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
14 files changed, 370 insertions(+), 100 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/4
--
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: newchange
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 4
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 20:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1969/
--
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: 20
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Feb 2018 02:47:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 19: Code-Review+2
(1 comment)
PS20 addresses the remaining comment. Carrying Michael's +2.
http://gerrit.cloudera.org:8080/#/c/9292/19/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/9292/19/be/src/runtime/exec-env.cc@159
PS19, Line 159: rpc_metrics_ = metrics_->GetOrCreateChildGroup("rpc");
> nit: The way rpc_metrics_ is initialized in PS18 should be fine too.
Moved it back to the initializer list.
--
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: 19
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Feb 2018 02:47:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 18:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1961/
--
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: 18
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Feb 2018 19:07:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#15).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change contains tests to check that the metrics show up in /rpcz
and /metrics and that they update as expected when executing queries.
This change is based on a change by Sailesh Mukil.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
16 files changed, 506 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/15
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 15
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9292/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9292/6//COMMIT_MSG@9
PS6, Line 9: /rpcz debug web page
If possible, could you post a link to a screenshot of a /rpcz page with this change?
It'll be easier to review the HTML and JS part with a visual.
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h@36
PS6, Line 36: // RPC metrics that are shared across all service pools. They need to be passed into the
: // service pool constructor and will aggregate values across all pools.
: struct RpcMetrics {
: // Number of RPCs that were rejected due to the queue being full. Not owned.
: IntCounter* rpcs_queue_overflow = nullptr;
: };
Do we need this now since we only have one service pool? My take is that we can add it as necessary, since we already have a counter to track the same thing within the ServicePool.
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@225
PS5, Line 225:
The RPC handler can run asynchronously, so we can't accurately track the num_in_handlers from here. So, it'd be better to remove this.
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@238
PS5, Line 238:
Wouldn't microseconds be too granular for the webpages?
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@256
PS5, Line 256: document->GetAllocator());
: value->AddMember("incoming_que
We'll have to remove this.
--
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: 6
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 23:48:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 21: Code-Review+2
Carrying Michael's +2.
--
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: 21
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Feb 2018 06:37:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 5:
PS5 exposes metrics for rejected RPCs on /metrics.
--
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: 5
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Feb 2018 23:37:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#20).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change contains tests to check that the metrics show up in /rpcz
and /metrics and that they update as expected when executing queries.
This change is based on a change by Sailesh Mukil.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
19 files changed, 548 insertions(+), 125 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/20
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 20
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#14).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change contains tests to check that the metrics show up in /rpcz
and /metrics and that they update as expected when executing queries.
This change is based on a change by Sailesh Mukil.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
16 files changed, 503 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/14
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 14
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@225
PS5, Line 225:
> Removed it. Can you clarify what asynchronously means here? Are you concern
One example is that in the KrpcDataStreamRecvr, we can defer responding to RPCs for early senders. So the RPC is technically not handled until the RPC is responded to. However in this case, the 'num_in_handlers_' wouldn't reflect the number of RPCs being handled correctly.
--
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: 6
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 00:43:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 5:
(6 comments)
Some initial comments. Doing another pass.
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.h@48
PS5, Line 48: public:
Please document the input arguments.
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@236
PS5, Line 236: DCHECK(
DCHECK_EQ
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h@100
PS5, Line 100: MetricGroup* metrics,
Can you please add a comment for 'metrics' and 'use_tls' (not your change) ?
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h@127
PS5, Line 127: GeneratedServiceIf
Why this change ? Is it because of something that's defined in GeneratedServiceIf but not in the base class ServiceIf ?
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h@156
PS5, Line 156: void ToJson(rapidjson::Document* document);
A quick comment on what's included in the output would help.
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.cc@85
PS5, Line 85: rpcs_queue_overflow
This is a shared counter across all RPC services, right ? If so, please add a comment about that.
--
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: 5
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 23:37:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 6:
PS 6 rebases the change and removed the client timeout metrics (as discussed between Michael and Lars in person). Our current client implementation will never time out and thus this value would always be 0.
--
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: 6
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 23:36:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 17: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/rpc/impala-service-pool.cc@254
PS15, Line 254: const string KrpcHistogramToString(const kudu::Histogram* histogram) {
> Currently we're exposing only what is required to fulfill ToJson()'s contra
Please at least add a TODO and mention the JIRA.
http://gerrit.cloudera.org:8080/#/c/9292/17/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/17/be/src/rpc/impala-service-pool.cc@281
PS17, Line 281:
nit: unnecessary blank line
http://gerrit.cloudera.org:8080/#/c/9292/17/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9292/17/be/src/rpc/rpc-mgr.cc@165
PS17, Line 165: if (acceptor_pool_.get()) acceptor_pool_.reset();
Can probably move this to be after line 167 instead given how the order in which messenger_, service_pool and acceptor_pool_ are initialized. The if-stmt can probably be skipped too. In other words, just do acceptor_pool_.reset() after line 167.
--
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: 17
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Feb 2018 18:47:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#17).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change contains tests to check that the metrics show up in /rpcz
and /metrics and that they update as expected when executing queries.
This change is based on a change by Sailesh Mukil.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
16 files changed, 516 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/17
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 17
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 20: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1969/
--
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: 20
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Feb 2018 06:29:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 10:
Please see the tests in PS10.
--
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: 10
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 22:05:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 19 Feb 2018 06:32:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#8).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change is based on a change by Sailesh Mukil.
TODO: testing, once IMPALA-6508 has been submitted.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
15 files changed, 387 insertions(+), 106 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/8
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 8
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#18).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change contains tests to check that the metrics show up in /rpcz
and /metrics and that they update as expected when executing queries.
This change is based on a change by Sailesh Mukil.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
16 files changed, 516 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/18
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 18
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 18: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1961/
--
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: 18
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Feb 2018 22:15:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker 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:
(4 comments)
Thanks for the review. Please see PS15 and my inline 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 pat
Good point, filed KUDU-2313 and left a comment 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 f
That's true. Non-generated services don't necessarily use a map to lookup methods. When adding the required getter to Kudu it felt far easier to only add it to the generated services interface than require a change to all services. I agree that we should revisit this if we need a custom service. An alternative then would be to register the methods ourselves, since we already have to type them out in code.
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 t
I factored the name into a parameter to make the code more generic. The next service might require a different query to exercise it though and not knowing which one exactly we're going to add, I'd like to defer factoring the method into a loop to a future change.
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?
I couldn't find a way to make it look nicer using colspan but putting in another layer of tables seems to have improved it.
--
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 <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Feb 2018 01:42:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 18: Code-Review+2
(3 comments)
Thanks for the reviews. PS18 addresses the final comments. Carrying Michael's +2.
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/rpc/impala-service-pool.cc@254
PS15, Line 254: // TODO: Switch to structured JSON (IMPALA-6545).
> Please at least add a TODO and mention the JIRA.
Done
http://gerrit.cloudera.org:8080/#/c/9292/17/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/17/be/src/rpc/impala-service-pool.cc@281
PS17, Line 281:
> nit: unnecessary blank line
Done
http://gerrit.cloudera.org:8080/#/c/9292/17/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9292/17/be/src/rpc/rpc-mgr.cc@165
PS17, Line 165: if (messenger_.get() == nullptr) return;
> Can probably move this to be after line 167 instead given how the order in
Done
--
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: 18
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Feb 2018 19:07:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 21:
PS21 puts a NOLINT statement back that is needed to please clang-tidy.
--
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: 21
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Feb 2018 06:37:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 15: Code-Review+2
(5 comments)
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@76
PS11, Line 76:
> No, since it is not added to any metric groups and thus also not displayed
Is there a JIRA for it ? Will it be done as part of IMPALA-6540 ?
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/rpc/impala-service-pool.cc@254
PS15, Line 254: const string KrpcHistogramToString(const kudu::Histogram* histogram) {
It may be more extensible to not return a formatted string. Instead, we can consider returning just a list of numbers and let the caller present them in a more flexible way. As the code stands now, it would require the caller to parse the string to extract the numbers.
If you think this makes sense, please add a TODO here.
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/rpc/rpc-mgr.cc@164
PS15, Line 164: void RpcMgr::Shutdown() {
We can probably give up ownership of the acceptor_pool_ before calling messenger_->Shutdown().
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
> I think this would break compatibility with previous versions of Impala, fo
Makes sense.
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/util/histogram-metric.h
File be/src/util/histogram-metric.h:
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/util/histogram-metric.h@96
PS15, Line 96: type
nit: long line
--
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: 15
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Feb 2018 02:17:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 21:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1971/
--
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: 21
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Feb 2018 06:38:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 17:
(4 comments)
Please see my inline comments and PS17. It also removes the now obsolete queue size and replaces it with memory usage values.
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@76
PS11, Line 76:
> Is there a JIRA for it ? Will it be done as part of IMPALA-6540 ?
I don't think this would be part of IMPALA-6540, and I filed IMPALA-6541 for it. This particular metric here should be moved to KRPC's internal code (KUDU-2313).
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/rpc/impala-service-pool.cc@254
PS15, Line 254: const string KrpcHistogramToString(const kudu::Histogram* histogram) {
> It may be more extensible to not return a formatted string. Instead, we can
Currently we're exposing only what is required to fulfill ToJson()'s contract. Once we want to wrap Kudu's Histograms and register them inside our metric groups, we will have to alter this abstraction (but I don't know how to do that yet). I created IMPALA-6541 to track this effort.
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/rpc/rpc-mgr.cc@164
PS15, Line 164: void RpcMgr::Shutdown() {
> We can probably give up ownership of the acceptor_pool_ before calling mess
Good point, done.
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/util/histogram-metric.h
File be/src/util/histogram-metric.h:
http://gerrit.cloudera.org:8080/#/c/9292/15/be/src/util/histogram-metric.h@96
PS15, Line 96:
> nit: long line
Done
--
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: 17
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Feb 2018 07:12:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change contains tests to check that the metrics show up in /rpcz
and /metrics and that they update as expected when executing queries.
This change is based on a change by Sailesh Mukil.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Reviewed-on: http://gerrit.cloudera.org:8080/9292
Reviewed-by: Lars Volker <lv...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
19 files changed, 547 insertions(+), 124 deletions(-)
Approvals:
Lars Volker: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 22
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 7:
(10 comments)
Thanks for the reviews. Please see my inline comments and PS7.
http://gerrit.cloudera.org:8080/#/c/9292/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9292/6//COMMIT_MSG@9
PS6, Line 9: /rpcz debug web page
> If possible, could you post a link to a screenshot of a /rpcz page with thi
Done
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h@36
PS6, Line 36: // A pool of threads that handle new incoming RPC calls.
: // Also includes a queue that calls get pushed onto for handling by the pool.
: class ImpalaServicePool : public kudu::rpc::RpcService {
: public:
: ImpalaServicePool(MemTracker* mem_tracker,
:
> My preference would be to expose the queue overflow metrics per service. In
Done
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.h@48
PS5, Line 48:
> Please document the input arguments.
I reverted this change.
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@225
PS5, Line 225: // Release the InboundCal
> My understanding is that this only tracks the number of threads running the
Thanks for the explanations. The total count of RPC calls is already exposed in the histograms so we should be good.
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.h@168
PS6, Line 168: }
> Please see comments elsewhere. This may not be needed.
Done
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h@100
PS5, Line 100: ficates, and encrypti
> Can you please add a comment for 'metrics' and 'use_tls' (not your change)
Done
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.cc@85
PS5, Line 85: TITY_server.Instant
> This is a shared counter across all RPC services, right ? If so, please add
Removed, N/A.
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.cc@82
PS6, Line 82:
> As discussed in person, it may be useful to have this in ExecEnv instead. F
I moved this to the ExecEnv. Moving the other RPC metrics into the same group seems to be a compatibility breaking change since various external tooling might depend on them being in impala-server. We should track moving them in a separate Jira.
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.cc@198
PS6, Line 198: document->AddMember("num_inbound_calls_in_flight", num_inbound_calls_in_flight,
: document->GetAllocator());
:
> How often is this run ? I assume this only acquires the shared lock so shou
This runs every time the /rpcz page gets rendered. Yes, QueueInboundCall() only takes a shared lock as well.
http://gerrit.cloudera.org:8080/#/c/9292/6/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/9292/6/common/thrift/metrics.json@1639
PS6, Line 1639: {
: "description": "Service $0: Total number of incoming RPCs that were rejected due to overflow of the service queue.",
: "contexts": [
: "IMPALAD"
: ],
: "label": "Service $0 Incoming RPC Queue Overflows",
: "units": "UNIT",
: "kind": "COUNTER",
: "key": "rpc.$0.rpcs_queue_overflow"
: }
> Not needed anymore ?
Done
--
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: 7
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 03:41:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#11).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change contains tests to check that the metrics show up in /rpcz
and /metrics and that they update as expected when executing queries.
This change is based on a change by Sailesh Mukil.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
16 files changed, 503 insertions(+), 106 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/11
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 11
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h@36
PS6, Line 36: // RPC metrics that are shared across all service pools. They need to be passed into the
: // service pool constructor and will aggregate values across all pools.
: struct RpcMetrics {
: // Number of RPCs that were rejected due to the queue being full. Not owned.
: IntCounter* rpcs_queue_overflow = nullptr;
: };
> Happy to remove it, what do others think?
My preference would be to expose the queue overflow metrics per service. In the end, we probably won't have many different RPC services (most likely 2). One for data stream related RPC and one for control command related (e.g. status report, starting fragment execution).
While having an aggregate count across all services is still a reasonable proxy, exposing them per service makes it easier to identify the problematic one and it shouldn't be too bad if we don't plan to have a lot of services.
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@225
PS5, Line 225:
> Removed it. Can you clarify what asynchronously means here? Are you concern
My understanding is that this only tracks the number of threads running the RPC handler. If I understand Sailesh correctly, what he meant is that some payloads may be handled in deferred manner as the row batch queue fills up in the receiver.
In some sense, this may be useful if all the threads are stuck in handlers for a long time. However, one really cannot tell between whether the threads are stuck or they are always busy. May be a more meaningful metrics is the aggregate number of RPC calls handled so at least we know if the service thread is stuck or not.
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.h@168
PS6, Line 168: RpcMetrics rpc_metrics_;
Please see comments elsewhere. This may not be needed.
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.cc@82
PS6, Line 82: MetricGroup* rpc_group = metrics->GetOrCreateChildGroup("rpc");
As discussed in person, it may be useful to have this in ExecEnv instead. For KRPC milestone 1, we still have a mix of KRPC and Thrift RPCs so it may be useful to move those existing Thrift RPC related metrics under this new group too.
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/rpc-mgr.cc@198
PS6, Line 198: // Add messenger metrics.
: DumpRunningRpcsResponsePB response;
: messenger_->DumpRunningRpcs(DumpRunningRpcsRequestPB(), &response);
How often is this run ? I assume this only acquires the shared lock so shouldn't block enqueuing incoming RPCs, right ?
http://gerrit.cloudera.org:8080/#/c/9292/6/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/9292/6/common/thrift/metrics.json@1639
PS6, Line 1639: {
: "description": "Total number of incoming RPCs that timed out in the incoming service queue.",
: "contexts": [
: "IMPALAD"
: ],
: "label": "rpcs-timed-out-in-queue",
: "units": "UNIT",
: "kind": "COUNTER",
: "key": "rpc.rpcs-timed-out-in-queue"
: },
Not needed anymore ?
--
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: 6
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 01:05:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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:
(4 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@85
PS11, Line 85: // Number of RPCs that were rejected due to the queue being full.
Owned by the rpc metrics group singleton.
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@264
PS11, Line 264:
nit: indent
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@52
PS11, Line 52: -datastream_service_queue_depth=1
This flag is being deprecated here https://gerrit.cloudera.org/#/c/9282/
http://gerrit.cloudera.org:8080/#/c/9292/11/tests/custom_cluster/test_krpc_metrics.py@70
PS11, Line 70: -datastream_service_queue_depth=1
Same here
--
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 <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 19 Feb 2018 00:32:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#5).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page.
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change is based on a change by Sailesh Mukil.
TODO: testing, once IMPALA-6508 has been submitted.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
15 files changed, 424 insertions(+), 106 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/5
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 4:
This is the change to expose the KRPC metrics.
--
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: 4
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Feb 2018 05:04:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#7).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change is based on a change by Sailesh Mukil.
TODO: testing, once IMPALA-6508 has been submitted.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
15 files changed, 387 insertions(+), 106 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/7
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 7
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#10).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change is based on a change by Sailesh Mukil.
TODO: testing, once IMPALA-6508 has been submitted.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
16 files changed, 503 insertions(+), 106 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/10
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 10
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 19: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9292/19/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/9292/19/be/src/runtime/exec-env.cc@159
PS19, Line 159: rpc_metrics_ = metrics_->GetOrCreateChildGroup("rpc");
nit: The way rpc_metrics_ is initialized in PS18 should be fine too.
--
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: 19
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Feb 2018 02:40:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#12).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change contains tests to check that the metrics show up in /rpcz
and /metrics and that they update as expected when executing queries.
This change is based on a change by Sailesh Mukil.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
16 files changed, 505 insertions(+), 107 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/12
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 12
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Feb 2018 00:48:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#13).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change contains tests to check that the metrics show up in /rpcz
and /metrics and that they update as expected when executing queries.
This change is based on a change by Sailesh Mukil.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
16 files changed, 501 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/13
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 13
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 13:
(6 comments)
PS13 contains the rebase. PS14 should address all left-over comments.
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: for (const auto& method : method_infos) {
: const string& method_name = method.first;
: string payload_size_name = Substitute("
> These lines can be combined into:
Done
http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/impala-service-pool.cc@76
PS11, Line 76:
> Does this need to be defined in metrics.json too ?
No, since it is not added to any metric groups and thus also not displayed on /metrics. I think we can add it there, but it would be inconsistent with the KRPC metrics, so I think it would be better to add them to /metrics in a separate change that also figures out how to add KRPC metrics there to begin with.
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@52
PS11, Line 52: -datastream_service_queue_depth=1
> I will address all other comments, then rebase the change, and then address
Replaced with the new flag.
http://gerrit.cloudera.org:8080/#/c/9292/11/tests/custom_cluster/test_krpc_metrics.py@60
PS11, Line 60: [0]
> The idea was that there needs to be at least one service, but you're right,
Done.
http://gerrit.cloudera.org:8080/#/c/9292/11/tests/custom_cluster/test_krpc_metrics.py@70
PS11, Line 70: -datastream_service_queue_depth=1
> See above.
Replaced with the new flag.
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
Done
--
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: 13
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 19 Feb 2018 23:37:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#16).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change contains tests to check that the metrics show up in /rpcz
and /metrics and that they update as expected when executing queries.
This change is based on a change by Sailesh Mukil.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
16 files changed, 507 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/16
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 16
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 6:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:
http://gerrit.cloudera.org:8080/#/c/9292/6/be/src/rpc/impala-service-pool.h@36
PS6, Line 36: // RPC metrics that are shared across all service pools. They need to be passed into the
: // service pool constructor and will aggregate values across all pools.
: struct RpcMetrics {
: // Number of RPCs that were rejected due to the queue being full. Not owned.
: IntCounter* rpcs_queue_overflow = nullptr;
: };
> Do we need this now since we only have one service pool? My take is that we
Happy to remove it, what do others think?
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@225
PS5, Line 225:
> The RPC handler can run asynchronously, so we can't accurately track the nu
Removed it. Can you clarify what asynchronously means here? Are you concerned that requests could get queued up in the service?
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@236
PS5, Line 236: return
> DCHECK_EQ
Done
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@238
PS5, Line 238:
> Wouldn't microseconds be too granular for the webpages?
This tells the printer how to interpret values in the histogram. Then they will be formatted to human readable strings.
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/impala-service-pool.cc@256
PS5, Line 256: document->GetAllocator());
: value->AddMember("incoming_que
> We'll have to remove this.
Done
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h@127
PS5, Line 127: GeneratedServiceIf
> Why this change ? Is it because of something that's defined in GeneratedSer
We need to access GeneratedServiceIf::methods_by_name(), which is not in ServiceIf.
http://gerrit.cloudera.org:8080/#/c/9292/5/be/src/rpc/rpc-mgr.h@156
PS5, Line 156: void ToJson(rapidjson::Document* document);
> A quick comment on what's included in the output would help.
Done
--
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: 6
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 00:22:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker 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:
(11 comments)
Thanks for the review. Please see PS11 and my inline comments. I will rebase the change next and then address comments related to the deprecated startup options.
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.
Done
http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/impala-service-pool.h@85
PS11, Line 85: // Number of RPCs that were rejected due to the queue being full.
> Owned by the rpc metrics group singleton.
I added to the comment that this is not owned, it feels more future proof to not include the actual owner and I think we omit it in other places, too. Let me know if you would like to include the owning entity here.
http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/impala-service-pool.h@86
PS11, Line 86: IntCounter* rpcs_queue_overflow_;
> = nullptr
Done
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@246
PS11, Line 246: Value service_name_val(service_name().c_str(), document->GetAllocator());
> nit: indent wrong
Done
http://gerrit.cloudera.org:8080/#/c/9292/11/be/src/rpc/impala-service-pool.cc@264
PS11, Line 264:
> nit: indent
Done
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 ?
Done
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 relat
I think this would break compatibility with previous versions of Impala, for example users might have tools that rely on the metric group hierarchy as it is. I created IMPALA-6540 for this.
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: ///
Done
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@52
PS11, Line 52: -datastream_service_queue_depth=1
> This flag is being deprecated here https://gerrit.cloudera.org/#/c/9282/
I will address all other comments, then rebase the change, and then address this one.
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 DataStr
The idea was that there needs to be at least one service, but you're right, this depends on the datastream service specific startup flag so I'll change this together with the flag after the rebase.
http://gerrit.cloudera.org:8080/#/c/9292/11/tests/custom_cluster/test_krpc_metrics.py@70
PS11, Line 70: -datastream_service_queue_depth=1
> Same here
See 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 <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 19 Feb 2018 22:34:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 21: Verified+1
--
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: 21
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Feb 2018 10:19:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#6).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page.
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change is based on a change by Sailesh Mukil.
TODO: testing, once IMPALA-6508 has been submitted.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
15 files changed, 415 insertions(+), 107 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/6
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 20: Code-Review+2
Carrying +2 to the right PS.
--
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: 20
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Feb 2018 04:40:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#19).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change contains tests to check that the metrics show up in /rpcz
and /metrics and that they update as expected when executing queries.
This change is based on a change by Sailesh Mukil.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
19 files changed, 550 insertions(+), 125 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/19
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 19
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Sailesh Mukil, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9292
to look at the new patch set (#21).
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
IMPALA-6269: Expose KRPC metrics on debug webpage
This change exposes KRPC metrics on the /rpcz debug web page.
This change also exposes metrics for rejected RPCs on the /metrics debug
web page. See here for an example: https://git.io/vAczm
This change also fixes a bug in PrettyPrinter::GetByteUnit(), which
previously did not work for unsigned values due to an implicit cast.
This change contains tests to check that the metrics show up in /rpcz
and /metrics and that they update as expected when executing queries.
This change is based on a change by Sailesh Mukil.
Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr-kerberized-test.cc
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/rpc/rpc-trace.cc
M be/src/rpc/rpc-trace.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/service/impalad-main.cc
M be/src/util/histogram-metric.h
M be/src/util/pretty-printer.h
M common/thrift/Metrics.thrift
M common/thrift/metrics.json
A tests/custom_cluster/test_krpc_metrics.py
M tests/webserver/test_web_pages.py
M www/rpcz.tmpl
19 files changed, 547 insertions(+), 124 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/92/9292/21
--
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: newpatchset
Gerrit-Change-Id: I7af7c1a84a5be82c979ca4ef1edf35167493be3f
Gerrit-Change-Number: 9292
Gerrit-PatchSet: 21
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 15: Code-Review+1
(2 comments)
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
> That's true. Non-generated services don't necessarily use a map to lookup m
Ok thanks for the explanation. We can address it when it becomes necessary. This looks good for now.
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:
> I factored the name into a parameter to make the code more generic. The nex
Makes sense, the query can be a parameter too then (like a 2-D array). But lets defer that for the future.
--
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: 15
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Feb 2018 01:48:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9292/7/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:
http://gerrit.cloudera.org:8080/#/c/9292/7/be/src/rpc/impala-service-pool.cc@55
PS7, Line 55: RPC_QUEUE_OVERFLOW_METRIC_KEY = "rpc.$0.rpcs_queue_overflow";
Looks reasonable to me.
http://gerrit.cloudera.org:8080/#/c/9292/7/be/src/rpc/impala-service-pool.cc@72
PS7, Line 72: service_->methods_by_name();
nit: indent
http://gerrit.cloudera.org:8080/#/c/9292/7/be/src/rpc/impala-service-pool.cc@74
PS7, Line 74:
nit: space
http://gerrit.cloudera.org:8080/#/c/9292/7/be/src/rpc/impala-service-pool.cc@77
PS7, Line 77:
nit: indent
--
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: 7
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 09:17:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6269: Expose KRPC metrics on debug webpage
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9292 )
Change subject: IMPALA-6269: Expose KRPC metrics on debug webpage
......................................................................
Patch Set 7:
Thanks for the review, I address the comments in PS8. I will rebase the change next to pull in the --test_krpc flag.
--
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: 7
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 18:33:39 +0000
Gerrit-HasComments: No