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 <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Sat, 10 Mar 2018 02:21:14 +0000 Gerrit-HasComments: Yes
