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 10: (8 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@357 PS5, Line 357: > We should be weary of table names changing and merge table IDs instead. Has been changed in the latest patch set. http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/13580/8/src/kudu/server/default_path_handlers.cc@347 PS8, Line 347: ; > nit: extra semicolon Done http://gerrit.cloudera.org:8080/#/c/13580/9/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/13580/9/src/kudu/server/default_path_handlers.cc@355 PS9, Line 355: SplitStringUsing(merge_rule, "|", &values); > Do you intend on using this to merge table IDs or table names? If table nam Table name could contains many special characters, even if ':' or ',', and ',' is used to split several merge rules.However I change it to use '|' which is barely occured in table names. And here, users can define their own rules, both table name and table ID is OK, because both of them are in attributes of a tablet entry. Done http://gerrit.cloudera.org:8080/#/c/13580/9/src/kudu/util/metrics-test.cc File src/kudu/util/metrics-test.cc: http://gerrit.cloudera.org:8080/#/c/13580/9/src/kudu/util/metrics-test.cc@143 PS9, Line 143: ASSERT > nit: move the = up to the previous line. Same elsewhere. Done http://gerrit.cloudera.org:8080/#/c/13580/9/src/kudu/util/metrics-test.cc@231 PS9, Line 231: expl > nit: one space in front of public and private: Done http://gerrit.cloudera.org:8080/#/c/13580/9/src/kudu/util/metrics-test.cc@462 PS9, Line 462: void CheckMergeOutput(const std::ostringstream& output, : std::map<std::string, int> expect_counters) { > nit: so it's easier to look back on this, could you comment what this is do I think we will get r-value in this case. Done http://gerrit.cloudera.org:8080/#/c/13580/9/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13580/9/src/kudu/util/metrics.h@595 PS9, Line 595: > nit: maybe MergedMetrics? I'm finding it a little daunting to have "merging Done http://gerrit.cloudera.org:8080/#/c/13580/9/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/13580/9/src/kudu/util/metrics.cc@355 PS9, Line 355: :: > nit: remove If not add '::', gcc can't find the FindOrNull function in gutil/map-util.h, but find the one of 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: 10 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: Mon, 12 Aug 2019 15:10:49 +0000 Gerrit-HasComments: Yes
