Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/13580 )
Change subject: [metrics] Merge metrics by the same attribute ...................................................................... Patch Set 8: (9 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? 1. IncrementBy(int64_t value, int64_t count) need value and count, we should traverse bucket and sub-bucket, and get value by ValueFromIndex(int bucket_index, int sub_bucket_index), right? 2. max, min, count, sum values update will go through many times NoBarrier_AtomicIncrement and comparation v.s. once 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 merg Done 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. Done 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 wh Renamed it to MergeFrom, certainly, this function is not const. 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. Done 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())) { > Oh so you needed in Merge too. Could you rename it to FillUniqueValuesUnloc Done 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 Done http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/metrics.cc@349 PS8, Line 349: collect > collecting Done http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/util/metrics.cc@355 PS8, Line 355: :: > Nit: shouldn't need this. If not add '::', gcc can't find the FindOrNull function in gutil/map-util.h, but the member function of MetricEntity. -- 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: Sat, 13 Jul 2019 15:26:36 +0000 Gerrit-HasComments: Yes
