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 6: (37 comments) http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/server/default_path_handlers.cc@353 PS5, Line 353: for (const auto& entity_types_merge_rule : entity_types_merge_rules) { : vector<string> values; : SplitStringUsing(entity_types_merge_rule, ":", &values); : if (values.size() == 3) { : // Index 0: entity type needed to be merged. : > This leaks table/tablet concepts into the server/ module, which, like util/ Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/hdr_histogram-test.cc File src/kudu/util/hdr_histogram-test.cc: http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/hdr_histogram-test.cc@133 PS5, Line 133: hist.Merge(other); > Should ASSERT_TRUE here. Now Merge return void. http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/hdr_histogram-test.cc@137 PS5, Line 137: ASSERT_EQ(hist.MinValue(), 1); : ASSERT_EQ(hist.MaxValue(), 250); > Maybe just test for 1 and 250 here instead? Would be clearer. Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/hdr_histogram-test.cc@139 PS5, Line 139: ASSERT_NEAR(hist.MeanValue(), (1 + 250) / 2.0, 1e3); > Maybe ASSERT_NEAR would help here (and maybe below too)? Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/hdr_histogram.h File src/kudu/util/hdr_histogram.h: http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/hdr_histogram.h@174 PS5, Line 174: // Merge with another HdrHistogram object, values in each 'counts_' array > Doc. Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/hdr_histogram.cc File src/kudu/util/hdr_histogram.cc: http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/hdr_histogram.cc@348 PS5, Line 348: DCHECK_EQ(num_significant_digits_, other.num_significant_digits()); : DCHECK_EQ(counts_array_length_, other.counts_array_length_); : DCHECK_EQ(bucket_count_, other.bucket_count_); : DCHECK_EQ(sub_bucket_count_, other.sub_bucket_count_); : DCHECK_EQ(sub_bucket_half_count_magnitude_, other.sub_bucket_half_count_magnitude_); : DCHECK_EQ(sub_bucket_half_count_, other.sub_bucket_half_count_); : DCHECK_EQ(sub_bucket_mask_, other.sub_bucket_mask_); : : f > Should these DCHECK/CHECK instead, like L358-363? Under what circumstances Will never happen at runtime now, I'll update it. http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/hdr_histogram.cc@368 PS5, Line 368: histogram_total_count_(histogram_->TotalCount()), : current_bucket_index_(0), : current_sub_bucket_index_(0), : current_value_at_index_(0), : next_bucket_index_(0), : next_sub_bucket_index_(1), : next_value_at_index_(1), : prev_value_iterated_to_(0), : total_count_to_prev_index_(0), : total_count_to_current_index_(0), : total_value_to_current_index_(0), : count_at_this_value_(0), : fresh_sub_bucket_(true) { : } : : bool AbstractHistogramIterator::HasNext() const { : return total_count_to_current_index_ < histogram_total_count_; : } : : Status AbstractHistogramIterator::Next(HistogramIterationValue* value) { : if (histogram_->TotalCount() != histogram_total_count_) { : return Status::IllegalState("Concurrently modified histogram while traversing it"); : } : > Could you share this with IncrementBy rather than copying it? Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@226 PS5, Line 226: #include <cstring> > Nit: prefer <cstring> Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@445 PS5, Line 445: to > setting Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@467 PS5, Line 467: > Not clear why this needs to be RefCountedThreadSafe. I'm using scoped_refptr<MergeAttributes> as value type in merge_attributes_by_entity_type, it's convenient by this way to auto release memory, and use FindPtrOrNull. http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@474 PS5, Line 474: 's ke > merged Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@477 PS5, Line 477: n > Nit: "is in" Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@478 PS5, Line 478: > Nit: replace with "for" Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@480 PS5, Line 480: > Nit "name is" Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@512 PS5, Line 512: // This makes constructor chaining a little less tedious. > Did this change at all, or was it just moved? Just moved, for some compile dependency problems. http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@567 PS5, Line 567: > Convention is to call these EqualTo rather than Equal. Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@573 PS5, Line 573: : type_(std::move(type)), id_(std::move(id)) {} > Doc. Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@579 PS5, Line 579: : struct MetricCollectionEntityHash { : size_t operator()(const MetricCollectionEntity& entity) const { : return std::hash<std::string>()(entity.type_ + entity.id_); : } : }; : : struct MetricCollectionEntityEqual { : bool operator()(const MetricCollectionEntity& first, const MetricCollectionEntity& second) const { : return first.type_ == second.type_ && first.id_ == second.id_; : } : }; > Why does Hash include just id_, but Equal compare both id_ and type_? That Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@627 PS5, Line 627: Status CollectTo(MetricCollection* collections, > Doc. Also not clear why, if this doesn't write JSON, it takes a MetricJsonO Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@681 PS5, Line 681: Get a snapshot of the entity's metrics > Nit: "Get a snapshot of the entity's metrics as well as the entity's attrib Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@683 PS5, Line 683: Status GetMetricsAndAttrs(const MetricFilters& filters, : Metri > Couldn't we just test for this by looking at the size of 'metrics' and 'att Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@685 PS5, Line 685: uteMap* attrs) co > Seems like a poor fit given there's no JSON involved here. Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@711 PS5, Line 711: : virtual Status WriteAsJson(JsonWriter* writer, > You can drop this part of the comment, I don't think it's necessary. Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@923 PS5, Line 923: ); > Where is this used? src/kudu/util/metrics.cc L678 in clone() http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@934 PS5, Line 934: > Call this "unique_values()"? Or doc what it means if it's not obvious. Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@937 PS5, Line 937: DISALLOW_COPY_AND_ASSIGN(StringGauge); > This only exists to support merging, right? May want to use unordered_set i Yes, just for merging. Have used unordered_set instead http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@951 PS5, Line 951: > Use down_cast (gutil/casts) instead of dynamic_cast. Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@1057 PS5, Line 1057: // call DetachToCurrentValue() to make it safe. : down_cast<FunctionGauge<T>*>(m.get())->DetachToCurrentValue(); : return m; > This is pretty icky because it violates the clone() contract (of doing a de Emm, I agree. How about rename 'clone' to 'snapshot'? clone is not a rigid demand. http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@1113 PS5, Line 1113: if (PREDICT_TRUE(this != other.get())) { > Is this why we need to deep copy metrics? Yes http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@1215 PS5, Line 1215: return m; > Nit: indentation. Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.h@1262 PS5, Line 1262: const gscoped_ptr<HdrHistogram> histogram_; > Where is this used? clone() in L1214 http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.cc@72 PS5, Line 72: const MetricCollection& collections, > Shouldn't we also (optionally) write entity attributes? The merged one doesn't have attributes_ property. http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.cc@75 PS5, Line 75: if (collection.second.empty()) { > Why doesn't this replicate the epoch/untouched check that WriteAsJson does? Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.cc@365 PS5, Line 365: InsertOrDie(&table_collection, new_metric->prototype(), new_metric); : } else { > Is there a gutil/map-util.h function that'd be appropriate (and more clear) There is not an appropriate one, however, I'll improve code here. http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.cc@461 PS5, Line 461: > Is this an important use case for you? It adds complexity and there's an an I haven't noticed this problem, I will remove it, and the metrics which have been merged should not be output. http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.cc@671 PS5, Line 671: > Don't need std:: prefixes in these sets and strings. Done http://gerrit.cloudera.org:8080/#/c/13580/5/src/kudu/util/metrics.cc@710 PS5, Line 710: std::lock_guard<simple_spinlock> l(lock_); > Hairy locking: we'll hold two StringGauge locks together at the same time f Done -- 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: 6 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: Thu, 11 Jul 2019 17:19:33 +0000 Gerrit-HasComments: Yes
