helifu 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 usa Done 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 'a Done 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? Done 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 thi Done 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 Done 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? Yes, it is used in the default_path_handlers.cc(L356) while initializing a JsonWriter object. 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? Done http://gerrit.cloudera.org:8080/#/c/13376/5/src/kudu/util/metrics.cc@197 PS5, Line 197: std:: > Don't need std:: prefix. Done 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. Done -- 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 15:02:33 +0000 Gerrit-HasComments: Yes
