Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13376 )
Change subject: [util] Support for entity type/id/attrs/metrics filtering in metrics ...................................................................... Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/server/default_path_handlers.cc@330 PS5, Line 330: const auto parseBool = [&] (const string& key, bool& value) { : string arg = FindWithDefault(req.parsed_args, key, "false"); : value = ParseLeadingBoolValue(arg.c_str(), false); : }; : const auto parseArray = [&] (const string& key, vector<string>& value) { : const string* arg = FindOrNull(req.parsed_args, key); : if (arg != nullptr) { : SplitStringUsing(*arg, ",", &value); : } : }; Could you implement these as static functions instead? It'll make their usage more clear (i.e. they won't capture everything, the names will be more descriptive, etc.). http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/server/default_path_handlers.cc@345 PS5, Line 345: parseArray("type", opts.entity_types); : parseArray("id", opts.entity_ids); Should these be pluralized into 'types' and 'ids' to be consistent with 'attributes' and 'metrics'? http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/util/metrics-test.cc File src/kudu/util/metrics-test.cc: http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/util/metrics-test.cc@350 PS5, Line 350: opts.entity_metrics.emplace_back("*"); Can we get rid of the wildcard cases in these tests? http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/util/metrics-test.cc@427 PS5, Line 427: > > This separation is an artifact of our pre-C++11 days. Now you can write this as '>>' without the parser getting confused. http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/util/metrics-test.cc@489 PS5, Line 489: opts.entity_attrs.emplace_back(attr1); : opts.entity_attrs.emplace_back(not_exist_string); I think it'd be clearer to do opts.entity_attrs = { attr1, not_exist_string } here and below. In fact, let's convert all of these emplace_back() calls into assignment. It's more clear in tests, and it'll be consistent here. http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/util/metrics.h@446 PS5, Line 446: // Whether to compact the json result. : bool compact; Where is this used? http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/util/metrics.cc@192 PS5, Line 192: // Handle wildcard. : if (e == "*") return true; We can remove this now, right? http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/util/metrics.cc@197 PS5, Line 197: std:: Don't need std:: prefix. http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/util/metrics.cc@208 PS5, Line 208: if (!opts.entity_types.empty()) { : if (!MatchNameInList(prototype_->name(), opts.entity_types)) { Combine these two conditions into one if. L214-215 too. -- To view, visit http://gerrit.cloudera.org:8080/13376 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee Gerrit-Change-Number: 13376 Gerrit-PatchSet: 5 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Fri, 24 May 2019 04:50:17 +0000 Gerrit-HasComments: Yes
