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 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/server/default_path_handlers.cc@347 PS4, Line 347: opts.entity_types = ParseArray(req.parsed_args, "types"); > If the point of this is to be pretty generic, would it make sense to use so Done http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@444 PS4, Line 444: // Whether to include un > Is this/Should this be affected by 'include_entity_attributes'? I add a new url parameter as comments in https://gerrit.cloudera.org/c/13580/4/src/kudu/server/default_path_handlers.cc#347, and this variable will be discarded. http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@442 PS4, Line 442: bool include_entity_attributes; : : // Whether to include unmerged entities if merge option is enabled. We can : // reduce output size by set 'include_unmerged_entities_if_merged' to false : // and enable merge option. See 'merge_attributes_by_entity_type' for merge option : // details. : // Default: false : bool include_unmerge > What happens if these are both true or both false? I think: `merge_by_attribute` has beend removed and `include_origin` has been renamed to `include_unmerged_entities_if_merged`. And when merge option is disabled, original entities will be output on force. http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@449 PS4, Line 449: include_unmerg > nit: How about naming 'include_origin' to 'include_unmerged_entities'? "ori Done http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@451 PS4, Line 451: // A set of substrings to filter entity against, where empty matches all. : // : // entity type. : std::vector<std::string> entity_types; : // entity id. : std::vector<std::string> entity_ids; : // entity attributes. : // > How about encapsulating these into a struct like: I think the latter one would be better, I've modified it like that. http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@673 PS4, Line 673: AttributeMap attributes); > Can you add docs to this, including that this will only ever return either Done http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@699 PS4, Line 699: > Could you add docs for this and why it might be useful to clone? Done http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@727 PS4, Line 727: > nit: Could you add docs for this too, including what the return value is? 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: 5 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, 22 Jun 2019 12:39:18 +0000 Gerrit-HasComments: Yes
