Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/12956 )
Change subject: IMPALA-8375: Add metrics for spill disk usage ...................................................................... Patch Set 4: (7 comments) Addressed review comments on patch #3. 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: are recycled from a file. We can add a new metric in future for keeping > Can you add a brief "Testing:" section to describe the automated tests you Done 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: Status status = file->Remove(); > nit: unnecessary parentheses Done 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: AddMetricDef("hwm_gauge", TMetricKind::GAUGE, TUnit::NONE); > I think if you cast this to the base class IntGauge the behaviour will chan Addressed this by making the new class AtomicHighWaterMarkGauge inherit directly from ScalarMetric<>. 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: /// for maintaining the current value and HWM, they could be out of sync for a short > I'm wondering if it's better to have this implemented as two metrics - one Done http://gerrit.cloudera.org:8080/#/c/12956/3/be/src/util/metrics.h@263 PS3, Line 263: : ScalarMetric<int64_t, TMetricKind::GAUGE>(metric_def), > Let's mark it is an override so it's clear that it's overriding something f Done http://gerrit.cloudera.org:8080/#/c/12956/3/be/src/util/metrics.h@269 PS3, Line 269: ~AtomicHighWaterMarkGauge() {} > I think we have a problem here - SetValue() is not a virtual function so we Addressed this by making the new class AtomicHighWaterMarkGauge inherit directly from ScalarMetric<>. http://gerrit.cloudera.org:8080/#/c/12956/3/be/src/util/metrics.h@276 PS3, Line 276: current_value_->SetValue(value); > Same issue here with it being non-virtual. Addressed this by making the new class AtomicHighWaterMarkGauge inherit directly from ScalarMetric<>. -- 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: 4 Gerrit-Owner: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: 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: Sun, 14 Apr 2019 23:56:46 +0000 Gerrit-HasComments: Yes
