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 <l...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 09 Mar 2018 20:18:45 +0000 Gerrit-HasComments: Yes
