Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13580 )
Change subject: [metrics] Merge metrics by the same attribute ...................................................................... Patch Set 9: (9 comments) Mostly cosmetic feedback from me. http://gerrit.cloudera.org:8080/#/c/13580/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13580/9//COMMIT_MSG@11 PS9, Line 11: if they do not care about the original metrics details. Could you also give a small example of how this is useful? E.g. by specifying a metrics URL that leverages the new behavior? 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. 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 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 names, we may want to delimit the strings by some other character (maybe ","?), since "impala::" is a pretty common prefix for existing tables. If table ID, this seems fine. 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: = nit: move the = up to the previous line. Same elsewhere. http://gerrit.cloudera.org:8080/#/c/13580/9/src/kudu/util/metrics-test.cc@231 PS9, Line 231: public nit: one space in front of public and private: http://gerrit.cloudera.org:8080/#/c/13580/9/src/kudu/util/metrics-test.cc@462 PS9, Line 462: void CheckCollectOutput(const std::ostringstream& out, : std::map<std::string, int>&& expect_counters) { nit: so it's easier to look back on this, could you comment what this is doing? Also, if we pass by value, would we get the r-value if using an initializer list? 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: CollectMetrics nit: maybe MergedMetrics? I'm finding it a little daunting to have "merging" and "aggregating" and "collecting" since they all seem quite similar, and I'd prefer if we could standardize the terminology. Same with MetricCollectionEntity -- maybe MetricMergingEntity? Or MergedMetricEntity? 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 -- 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: 9 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, 09 Aug 2019 00:33:47 +0000 Gerrit-HasComments: Yes
