Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count sub-entities when merge metrics ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.h@711 PS2, Line 711: // See documentation at the top of this file for information on metrics ownership. > Needs docs. Done http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.h@713 PS2, Line 713: public: > Nit: indented too much. Done http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@358 PS2, Line 358: if (!entity_id_ptr) { > What happens if this map lookup fails? Seems like we'll set entity_id to an We should return a NotFound error. Done http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@375 PS2, Line 375: > existing Done http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@377 PS2, Line 377: } > This should be a FindOrDie variant if there's an expectation that the resul Done http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@461 PS2, Line 461: for (const auto& e : entities) { : WARN_NOT_OK(e.second->WriteAsJson(writer, opts), : Substitute("Failed to write entity $0 as JSON", e.second->id())); : } : > An AutoReleasePool might be a more natural choice. Useful util, thanks! Done http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@467 PS2, Line 467: > Drop the prefix. Done http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@479 PS2, Line 479: // metrics, we should keep them around until the next poll. > This effectively adds "dynamic" metric prototype definition to the metrics Thanks for your advice. And you remind me why not define 'sub_entities_count' as a tablet's metric? For tablet, it always be 1, when we merge tablets' metrics to a upper level entity (i.e. table type entity), the GaugePrototype<uint64_t> type will sum up all sub-entities automatically. Done -- To view, visit http://gerrit.cloudera.org:8080/14128 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1 Gerrit-Change-Number: 14128 Gerrit-PatchSet: 3 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 27 Aug 2019 07:32:05 +0000 Gerrit-HasComments: Yes
