Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14903 )
Change subject: [metrics] Support SUM, MAX and MIN merge types for AtomicGauge ...................................................................... Patch Set 2: (4 comments) Will you be using this in follow-up commits? Which metrics are you thinking of changing? http://gerrit.cloudera.org:8080/#/c/14903/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14903/2//COMMIT_MSG@10 PS2, Line 10: metics metrics http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics-test.cc File src/kudu/util/metrics-test.cc: http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics-test.cc@270 PS2, Line 270: ASSERT_EQ(2, mem_usage_for_merge->value()); Could you add to this a decrement of mem_usage to a value below 2 and show that mem_usage_for_merge drops too? http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics.h@434 PS2, Line 434: enum class MergeType { Add a comment explaining what this enum means and a high-level description of its effects? http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics.h@1130 PS2, Line 1130: down_cast<AtomicGauge<T>*>(other.get())->value() This is repeated across all three cases; do it once and store in a local variable? -- To view, visit http://gerrit.cloudera.org:8080/14903 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81 Gerrit-Change-Number: 14903 Gerrit-PatchSet: 2 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 13 Dec 2019 20:05:40 +0000 Gerrit-HasComments: Yes
