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 <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 09 Mar 2018 03:31:30 +0000 Gerrit-HasComments: Yes
