[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 5: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 13 Mar 2018 02:01:14 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..

IMPALA-6576: Add metrics for data stream service memory usage

This change adds metrics for the data stream service memory usage. Both
current and peak usage are exposed.

It adds a test to test_krpc_metrics.py to make sure that the expected
metrics are present and that the peak usage shows a non-zero value after
running a query.

Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Reviewed-on: http://gerrit.cloudera.org:8080/9562
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/runtime/exec-env.cc
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M be/src/util/memory-metrics.cc
M be/src/util/memory-metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_krpc_metrics.py
7 files changed, 104 insertions(+), 17 deletions(-)

Approvals:
  Tim Armstrong: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2087/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 12 Mar 2018 22:27:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 12 Mar 2018 22:24:37 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 5:

I wasn't happy with the metric naming, I think "current" captures the idea 
better than "total".


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 12 Mar 2018 22:23:08 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Lars Volker (Code Review)]

Hello Michael Ho, Sailesh Mukil, Tim Armstrong, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9562

to look at the new patch set (#5).

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..

IMPALA-6576: Add metrics for data stream service memory usage

This change adds metrics for the data stream service memory usage. Both
current and peak usage are exposed.

It adds a test to test_krpc_metrics.py to make sure that the expected
metrics are present and that the peak usage shows a non-zero value after
running a query.

Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
---
M be/src/runtime/exec-env.cc
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M be/src/util/memory-metrics.cc
M be/src/util/memory-metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_krpc_metrics.py
7 files changed, 104 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/9562/5
--
To view, visit http://gerrit.cloudera.org:8080/9562
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 4:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2083/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 4
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 12 Mar 2018 20:25:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 4
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 12 Mar 2018 20:03:33 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 4: Code-Review+1

Thanks for the reviews. PS4 addresses Tim's comment. Carrying +1 from Michael 
and Sailesh.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 4
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 12 Mar 2018 19:39:28 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/service/data-stream-service.cc@58
PS3, Line 58: InitMetrics
I think InitMetrics() is misleading - the name is too similar to other 
functions that do some kind of global initialization. Maybe something like 
MemTrackerMetric::CreateMetrics() so it sounds more like a constructor/factory 
method.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 12 Mar 2018 19:07:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/service/data-stream-service.cc@58
PS3, Line 58: DataStreamService
> If we're going to do a CM change anyway, then I suggest staying consistent 
Lars pointed out to me that non KRPC RPC metrics are also camel cased. So, 
since that's the case, lets leave this as it is. It's not important enough to 
have to change everything at this point.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 12 Mar 2018 18:58:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 3: Code-Review+1

(1 comment)

Feel free to upgrade to a +2 if no one else is reviewing it aside from Michael 
and I.

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/service/data-stream-service.cc@58
PS3, Line 58: DataStreamService
> Yes, we seem to be a bit inconsistent in the naming convention for the RPC
If we're going to do a CM change anyway, then I suggest staying consistent with 
the other metrics and change both this and the one from IMPALA-6269.

If the CM change is to be deferred for later, then sticking with this is fine. 
But the preference is the former.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 12 Mar 2018 18:48:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/service/data-stream-service.cc@58
PS3, Line 58: DataStreamService
> I noticed that, too, but the one we added in IMPALA-6269 is camel case. htt
Yes, we seem to be a bit inconsistent in the naming convention for the RPC 
related metrics and other metrics. My suggestion would be to stay consistent 
with other RPC related metrics for now.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 12 Mar 2018 18:09:43 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/service/data-stream-service.cc@58
PS3, Line 58: treamService");
> nit: The naming conventions for all the other metrics seems to be like this
I noticed that, too, but the one we added in IMPALA-6269 is camel case. 
https://gerrit.cloudera.org/#/c/9292/

Looking at /metrics the other service names are camel case, too, e.g. 
rpc-method.backend.ImpalaInternalService.UpdateFilter.call_duration.

I'm open to changing this to data-stream-service, but then we should also 
change the one from IMPALA-6269. That will require to change it in CM, too, 
which we can do in the change to expose the ones from this current change.

Let me know which way you prefer.


http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/util/memory-metrics.h
File be/src/util/memory-metrics.h:

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/util/memory-metrics.h@203
PS3, Line 203:   static BufferPoolMetric* LIMIT;
 :   static BufferPoolMetric* SYSTEM_ALLOCATED;
 :   static BufferPoolMetric* RESERVED;
 :   static BufferPoolMetric* UNUSED_RESERVATION_BYTES;
 :   static BufferPoolMetric* NUM_FREE_BUFFERS;
 :   static BufferPoolMetric* FREE_BUFFER_BYTES;
 :   static BufferPoolMetric* CLEAN_PAGES_LIMIT;
 :   static BufferPoolMetric* NUM_CLEAN_PAGES;
 :   static BufferPoolMetric* CLEAN_PAGE_BYTES;
> Do we need to make the metrics for MemTrackerMetric, globally visible like
I didn't see why. Once they are registered we don't need to access them 
anymore. The ones here are globally unique, whereas the MemTrackerMetrics could 
be instantiated for multiple MemTrackers.


http://gerrit.cloudera.org:8080/#/c/9562/2/tests/custom_cluster/test_krpc_metrics.py
File tests/custom_cluster/test_krpc_metrics.py:

http://gerrit.cloudera.org:8080/#/c/9562/2/tests/custom_cluster/test_krpc_metrics.py@93
PS2, Line 93: metric_name = 
'rpc.impala.DataStreamService.rpcs_queue_overflow'
: before = self.get_metric(metric_name)
: assert before == 0
:
: self.client.execute(self.TEST_QUERY)
: after = self.get_metric(metric_name)
: assert before < after
> How about making this a helper function? And then taking metric_name as a p
For this change here, making it a helper won't benefit us, since the other 
metrics are gauges that don't necessarily increase monotonically. I think it's 
better to defer refactoring to a future change when we can benefit from it. 
That way we'll know which interface to pick.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 10 Mar 2018 02:21:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Sailesh Mukil (Code Review)]

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

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/service/data-stream-service.cc@58
PS3, Line 58: DataStreamService
nit: The naming conventions for all the other metrics seems to be like this:
"data-stream-service"

Let's be consistent with that.


http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/util/memory-metrics.h
File be/src/util/memory-metrics.h:

http://gerrit.cloudera.org:8080/#/c/9562/3/be/src/util/memory-metrics.h@203
PS3, Line 203:   static BufferPoolMetric* LIMIT;
 :   static BufferPoolMetric* SYSTEM_ALLOCATED;
 :   static BufferPoolMetric* RESERVED;
 :   static BufferPoolMetric* UNUSED_RESERVATION_BYTES;
 :   static BufferPoolMetric* NUM_FREE_BUFFERS;
 :   static BufferPoolMetric* FREE_BUFFER_BYTES;
 :   static BufferPoolMetric* CLEAN_PAGES_LIMIT;
 :   static BufferPoolMetric* NUM_CLEAN_PAGES;
 :   static BufferPoolMetric* CLEAN_PAGE_BYTES;
Do we need to make the metrics for MemTrackerMetric, globally visible like this?


http://gerrit.cloudera.org:8080/#/c/9562/2/tests/custom_cluster/test_krpc_metrics.py
File tests/custom_cluster/test_krpc_metrics.py:

http://gerrit.cloudera.org:8080/#/c/9562/2/tests/custom_cluster/test_krpc_metrics.py@93
PS2, Line 93: metric_name = 
'rpc.impala.DataStreamService.rpcs_queue_overflow'
: before = self.get_metric(metric_name)
: assert before == 0
:
: self.client.execute(self.TEST_QUERY)
: after = self.get_metric(metric_name)
: assert before < after
How about making this a helper function? And then taking metric_name as a 
parameter? We'll be adding more metrics in the future, so having this helper 
function would be nice.

The 'before' check may have to go though, since it may not be 0 for anything 
after the first query is run.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 09 Mar 2018 21:02:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 2:

(6 comments)

Thanks for the review. Please see my inline comments and PS3.

http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/service/data-stream-service.h
File be/src/service/data-stream-service.h:

http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/service/data-stream-service.h@43
PS2, Line 43: metrics
> nit: May be easier to follow to call it metrics_group ?
Done


http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.h
File be/src/util/memory-metrics.h:

http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.h@247
PS2, Line 247: each type
> What does "each type" mean ? May want to elaborate exactly what this functi
Done


http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.h@254
PS2, Line 254: class
> why not just enum MemTrackerMetricType ?
I mostly used the new C++11 enum class for consistency, since the rest of the 
file does so (see L218 above). They also provide stronger type guarantees.


http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.h@262
PS2, Line 262: MemTrackerMetricType type_
> Can this be const ?
Yes, done.


http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.h@263
PS2, Line 263: MemTracker* mem_tracker_
> What's the story with the lifecycle management of the metrics wrt mem_track
They both are owned transitively by the exec env singleton which lives until 
the end of the process. If we were ever to destruct the exec env instance, the 
memtracker would get destructed before the webserver and before the metrics so 
there could be an issue. However, we seem to ignore this in general, e.g. 
rendering the /memz page during shutdown has the same issues. I updated the 
comment to make the requirement explicit.


http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.cc
File be/src/util/memory-metrics.cc:

http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.cc@305
PS2, Line 305: void MemTrackerMetric::InitMetrics(MetricGroup* metrics, 
MemTracker* mem_tracker, const string& name) {
> nit: long line
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 09 Mar 2018 20:18:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Lars Volker (Code Review)]

Hello Michael Ho, Tim Armstrong,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9562

to look at the new patch set (#3).

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..

IMPALA-6576: Add metrics for data stream service memory usage

This change adds metrics for the data stream service memory usage. Both
current and peak usage are exposed.

It adds a test to test_krpc_metrics.py to make sure that the expected
metrics are present and that the peak usage shows a non-zero value after
running a query.

Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
---
M be/src/runtime/exec-env.cc
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M be/src/util/memory-metrics.cc
M be/src/util/memory-metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_krpc_metrics.py
7 files changed, 104 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/9562/3
--
To view, visit http://gerrit.cloudera.org:8080/9562
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9562 )

Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..


Patch Set 2:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/service/data-stream-service.h
File be/src/service/data-stream-service.h:

http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/service/data-stream-service.h@43
PS2, Line 43: metrics
nit: May be easier to follow to call it metrics_group ?


http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.h
File be/src/util/memory-metrics.h:

http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.h@247
PS2, Line 247: each type
What does "each type" mean ? May want to elaborate exactly what this function 
accomplishes.

Registers two new metrics tracking the peak and current usages of 'mem_tracker' 
in the metrics group 'metrics'.


http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.h@254
PS2, Line 254: class
why not just enum MemTrackerMetricType ?


http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.h@262
PS2, Line 262: MemTrackerMetricType type_
Can this be const ?


http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.h@263
PS2, Line 263: MemTracker* mem_tracker_
What's the story with the lifecycle management of the metrics wrt mem_tracker_ 
? How do we make sure that the metrics doesn't outlive the MemTracker itself ?


http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.cc
File be/src/util/memory-metrics.cc:

http://gerrit.cloudera.org:8080/#/c/9562/2/be/src/util/memory-metrics.cc@305
PS2, Line 305: void MemTrackerMetric::InitMetrics(MetricGroup* metrics, 
MemTracker* mem_tracker, const string& name) {
nit: long line



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 09 Mar 2018 03:31:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6576: Add metrics for data stream service memory usage

[Lars Volker (Code Review)]

Lars Volker has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9562


Change subject: IMPALA-6576: Add metrics for data stream service memory usage
..

IMPALA-6576: Add metrics for data stream service memory usage

This change adds metrics for the data stream service memory usage. Both
current and peak usage are exposed.

It adds a test to test_krpc_metrics.py to make sure that the expected
metrics are present and that the peak usage shows a non-zero value after
running a query.

Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
---
M be/src/runtime/exec-env.cc
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M be/src/util/memory-metrics.cc
M be/src/util/memory-metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_krpc_metrics.py
7 files changed, 101 insertions(+), 17 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/9562/2
--
To view, visit http://gerrit.cloudera.org:8080/9562
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5033b8dfda0b23d4230535ba13c3e050a35d01a3
Gerrit-Change-Number: 9562
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker