Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12956 )
Change subject: IMPALA-8375: Add metrics for spill disk usage ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/12956/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12956/3//COMMIT_MSG@20 PS3, Line 20: Can you add a brief "Testing:" section to describe the automated tests you added and any manual tests you ran (e.g. looked at the metrics page. I find that helpful for reviewing. http://gerrit.cloudera.org:8080/#/c/12956/3/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: http://gerrit.cloudera.org:8080/#/c/12956/3/be/src/runtime/tmp-file-mgr.cc@285 PS3, Line 285: -1 * (scratch_space_bytes_used_counter_->value())); nit: unnecessary parentheses http://gerrit.cloudera.org:8080/#/c/12956/3/be/src/util/metrics-test.cc File be/src/util/metrics-test.cc: http://gerrit.cloudera.org:8080/#/c/12956/3/be/src/util/metrics-test.cc@112 PS3, Line 112: IntHWMGauge* int_hwm_gauge = metrics.AddHWMGauge("gauge", 0); I think if you cast this to the base class IntGauge the behaviour will change because of the non-virtual methods. http://gerrit.cloudera.org:8080/#/c/12956/3/be/src/util/metrics.h File be/src/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/12956/3/be/src/util/metrics.h@255 PS3, Line 255: class AtomicHighWaterMarkMetric : public IntGauge { I'm wondering if it's better to have this implemented as two metrics - one which is the underlying metric and other which is the high water mark. I think if the HWM is useful then the other metric is also useful to expose. E.g. this could be a wrapper around an AtomicMetric similar to NegatedGauge below. http://gerrit.cloudera.org:8080/#/c/12956/3/be/src/util/metrics.h@263 PS3, Line 263: int64_t GetValue() { return hwm_value_.Load(); } Let's mark it is an override so it's clear that it's overriding something from the base class. http://gerrit.cloudera.org:8080/#/c/12956/3/be/src/util/metrics.h@269 PS3, Line 269: void SetValue(const int64_t& value) { I think we have a problem here - SetValue() is not a virtual function so we can easily call the wrong implementation if we have a pointer to IntGauge rather than AtomicHighWaterMarkMetric. http://gerrit.cloudera.org:8080/#/c/12956/3/be/src/util/metrics.h@276 PS3, Line 276: int64_t Increment(int64_t delta) { Same issue here with it being non-virtual. I think if we implemented this as a subclass of ScalarMetric<> wrapping an AtomicMetric, we would avoid this issue since it wouldn't be in the same hierarchy as AtomicMetric. -- To view, visit http://gerrit.cloudera.org:8080/12956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia1b3dd604c7234a8d8af34d70ca731544a46d298 Gerrit-Change-Number: 12956 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 08 Apr 2019 23:50:25 +0000 Gerrit-HasComments: Yes
