Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13580 )
Change subject: [metrics] Merge metrics by the same attribute ...................................................................... Patch Set 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/hdr_histogram.cc File src/kudu/util/hdr_histogram.cc: http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/hdr_histogram.cc@359 PS8, Line 359: Why didn't IncrementBy work here? http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/metrics-test.cc File src/kudu/util/metrics-test.cc: http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/metrics-test.cc@132 PS8, Line 132: TEST_F(MetricsTest, SimpleStringGaugeForMergeTest) { The unordered_set means you can't expect to see a consistent order for merged values. http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/metrics.h@602 PS8, Line 602: MetricsMergeRules Nit: MetricMergeRules for consistency. http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/metrics.h@746 PS8, Line 746: virtual bool Merge(const scoped_refptr<Metric>& other) = 0; You might want to rename this to either MergeTo or MergeFrom to indicate which of the two Metrics is being mutated. Also, if 'other' is being mutated, this function can be const right? http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/metrics.h@927 PS8, Line 927: = std::unordered_set<std::string>()); Surprised {} didn't work here, but OK. http://gerrit.cloudera.org:8080/#/c/13580/6/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/13580/6/src/kudu/util/metrics.cc@682 PS6, Line 682: if (PREDICT_TRUE(unique_values_.empty())) { > Done Oh so you needed in Merge too. Could you rename it to FillUniqueValuesUnlocked then? http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/metrics.cc@312 PS8, Line 312: // as OK, and skip print it. printing http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/metrics.cc@349 PS8, Line 349: collect collecting -- To view, visit http://gerrit.cloudera.org:8080/13580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8db3d082ae847eb1d83b9e4aee57d5e4bf13e1b5 Gerrit-Change-Number: 13580 Gerrit-PatchSet: 8 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Fri, 12 Jul 2019 18:07:33 +0000 Gerrit-HasComments: Yes
