Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19029 )
Change subject: IMPALA-11606: add 'untracked memory' metric ...................................................................... Patch Set 8: (8 comments) Thanks. I have some suggestions, but I'm not very familiar with this area so it would be good if someone else could also have a look. 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: that : not that is not 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: UntrackedMemory The name is a bit misleading because what it returns is memory tracked by this MemTracker but not its children. The comment should explain this. 'MemoryUntrackedByChildren()' may be a better name. I know it's intended for process level MemTrackers where "not tracked by children" more or less means "untracked" but this function can be called on other kinds of MemTrackers, too, where the name is misleading. 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* mem_tracker_; > Hi jian, unfortunately I can't add the const modifier because it cannot co It should be MemTracker* const mem_tracker_; That is, the pointer cannot be changed but the pointed-to value can. 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: IntGauge No need to inherit from IntGauge (which is a typedef for AtomicMetric<TMetricKind::GAUGE>, see https://github.com/apache/impala/blob/1899b2e34b3fd184bcd3101fb5da8ae80479ef25/be/src/util/metrics-fwd.h#L66). AtomicMetric adds the member "AtomicInt64 value_;" to ScalarMetric, which we don't use here, as well as the member functions "SetValue()" and "Increment()" which are not appropriate here. We should inherit from ScalarMetric<int64_t, TMetricKind::GAUGE> instead. http://gerrit.cloudera.org:8080/#/c/19029/8/be/src/runtime/mem-tracker.cc@273 PS8, Line 273: auto Writing the type would be more informative. http://gerrit.cloudera.org:8080/#/c/19029/8/be/src/runtime/mem-tracker.cc@274 PS8, Line 274: metrics->RegisterMetric(bytes_untracked); I'm a bit concerned about the lifetime of this MemTracker and the UntrackedMemoryMetric instance, which holds a pointer to 'this'. I think we only call this function when this MemTracker and the MetricGroup object, which will have ownership of the UntrackedMemoryMetric object, have ~equal lifetimes (for example as members of ExecEnv), but if I'm wrong or the code changes in the future, it could lead to a dangling pointer. I'm not sure there is a good way to check that as I don't think the order of destroying MemTracker and MetricGroup (together with the UntrackedMemoryMetric object) is fixed. Unless we enforce an order, we'd have to have both objects notify the other when they are destroyed, checking whether the other is still alive. That would be complicated. We could at least add a comment to RegisterMetrics() in the header saying that this function should only be called for process level MemTrackers. Please correct me if we actually call it for other MemTrackers, I may be wrong. http://gerrit.cloudera.org:8080/#/c/19029/8/be/src/runtime/mem-tracker.cc@401 PS8, Line 401: // TODO: uncomment next line will cause the 'TestAdmissionController::() How does the test fail? 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 not Nit: that is not -- 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: 8 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: Xiang Yang <[email protected]> Gerrit-Reviewer: Xianqing He <[email protected]> Gerrit-Comment-Date: Tue, 08 Nov 2022 14:40:58 +0000 Gerrit-HasComments: Yes
