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 <l...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-Comment-Date: Mon, 19 Feb 2018 22:34:15 +0000
Gerrit-HasComments: Yes

Reply via email to