Xiang Yang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19029 )
Change subject: IMPALA-11606: Add the 'mem-tracker.process.bytes-untracked' metric. ...................................................................... Patch Set 9: (10 comments) http://gerrit.cloudera.org:8080/#/c/19029/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19029/8//COMMIT_MSG@9 PS8, Line 9: rd : the > that is not Done http://gerrit.cloudera.org:8080/#/c/19029/8/be/src/runtime/mem-tracker.h File be/src/runtime/mem-tracker.h: http://gerrit.cloudera.org:8080/#/c/19029/8/be/src/runtime/mem-tracker.h@305 PS8, Line 305: ld only apply t > The name is a bit misleading because what it returns is memory tracked by t Done http://gerrit.cloudera.org:8080/#/c/19029/7/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/19029/7/be/src/runtime/mem-tracker.cc@56 PS7, Line 56: MemTracker* const mem_tra > It should be Done http://gerrit.cloudera.org:8080/#/c/19029/8/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: http://gerrit.cloudera.org:8080/#/c/19029/8/be/src/runtime/mem-tracker.cc@49 PS8, Line 49: ScalarMe > No need to inherit from IntGauge (which is a typedef for AtomicMetric<TMetr Done http://gerrit.cloudera.org:8080/#/c/19029/8/be/src/runtime/mem-tracker.cc@272 PS8, Line 272: > nit: "bytes-untracked" fits more with the other metric names. Done http://gerrit.cloudera.org:8080/#/c/19029/8/be/src/runtime/mem-tracker.cc@273 PS8, Line 273: TMet > Writing the type would be more informative. Done http://gerrit.cloudera.org:8080/#/c/19029/8/be/src/runtime/mem-tracker.cc@274 PS8, Line 274: UntrackedMemoryMetric* bytes_untracked = new UntrackedMemoryMetric(this, metric_def); > I'm a bit concerned about the lifetime of this MemTracker and the Untracke Done http://gerrit.cloudera.org:8080/#/c/19029/8/be/src/runtime/mem-tracker.cc@396 PS8, Line 396: { > Do you have chance to test this in a busy cluster? Not sure if crawling the I ran an internal benchmark (which contains 79 internal sqls, executed serially) on a 3 nodes cluster for 3 times, with a script to crawl the metrics every seconds or not, the total executing times are almost the same(whether crawling the metrics or not). Or should I make it a flag to control whether register the metric or not? http://gerrit.cloudera.org:8080/#/c/19029/8/be/src/runtime/mem-tracker.cc@401 PS8, Line 401: } > How does the test fail? I can't figure it out :( http://gerrit.cloudera.org:8080/#/c/19029/8/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/19029/8/common/thrift/metrics.json@2043 PS8, Line 2043: that is > Nit: that is not Done -- To view, visit http://gerrit.cloudera.org:8080/19029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib16e00109d732f759c96c7a967eb1cc32124a03f Gerrit-Change-Number: 19029 Gerrit-PatchSet: 9 Gerrit-Owner: Xiang Yang <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jian Zhang <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Xiang Yang <[email protected]> Gerrit-Reviewer: Xianqing He <[email protected]> Gerrit-Comment-Date: Wed, 15 Feb 2023 02:30:35 +0000 Gerrit-HasComments: Yes
