You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "helifu (Code Review)" <ge...@cloudera.org> on 2019/05/20 06:36:31 UTC
[kudu-CR] [util] Support for table and tablet filtering in metrics
helifu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13376
Change subject: [util] Support for table and tablet filtering in metrics
......................................................................
[util] Support for table and tablet filtering in metrics
It will take a long time to display the metrics while there are
lots of tablets on the tserver, especially in web browsers. So,
I think it is good to filter table and tablet.
Usage:
http://x.x.x.x:8050/metrics?tables=table_uuid&other_params
http://x.x.x.x:8050/metrics?tablets=tablet_uuid&other_params
Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
4 files changed, 167 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/13376/1
--
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: newchange
Gerrit-Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
Gerrit-Change-Number: 13376
Gerrit-PatchSet: 1
Gerrit-Owner: helifu <hz...@corp.netease.com>
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Fri, 24 May 2019 04:50:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
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 <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Fri, 24 May 2019 15:02:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Support for table and tablet filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13376
to look at the new patch set (#2).
Change subject: [util] Support for table and tablet filtering in metrics
......................................................................
[util] Support for table and tablet filtering in metrics
It will take a long time to display the metrics while there are
lots of tablets on the tserver, especially in web browsers. So,
I think it is good to filter table and tablet.
Usage:
http://x.x.x.x:8050/metrics?tables=table_uuid&other_params
http://x.x.x.x:8050/metrics?tablets=tablet_uuid&other_params
Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
4 files changed, 167 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/13376/2
--
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: newpatchset
Gerrit-Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
Gerrit-Change-Number: 13376
Gerrit-PatchSet: 2
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 3:
(10 comments)
ttr
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/server/default_path_handlers.cc
File src/kudu/server/default_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/server/default_path_handlers.cc@342
PS3, Line 342: // 'type'
: {
: const string* arg = FindOrNull(req.parsed_args, "type");
: if (arg != nullptr) {
: SplitStringUsing(*arg, ",", &opts.entity_types);
: }
: }
Could you decompose this into a helper function to avoid repeating the same few lines of code?
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc
File src/kudu/util/metrics-test.cc:
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc@407
PS3, Line 407: METRIC_DEFINE_entity(tablet);
: METRIC_DEFINE_counter(tablet, rows_inserted, "Rows Inserted", MetricUnit::kRows,
: "Number of rows inserted into this tablet since service start");
: METRIC_DEFINE_counter(tablet, rows_upserted, "Rows Upserted", kudu::MetricUnit::kRows,
: "Number of rows upserted into this tablet since service start");
Could you reuse the entity/counter already defined by this test? (L55, L70, L84, and L98). I think you could assign whatever IDs/attributes to that entity that you want.
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc@419
PS3, Line 419: const auto GenMetrics = [&] () {
GenMetrics and CheckMetrics are only called once each, so why bother wrapping them in lambdas?
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc@443
PS3, Line 443: {
: const string entity_type = "not_exist_type";
: std::ostringstream out;
: MetricJsonOptions opts;
: opts.entity_types.emplace_back(entity_type);
: JsonWriter w(&out, JsonWriter::PRETTY);
: ASSERT_OK(registry_.WriteAsJson(&w, opts));
: ASSERT_EQ("[]", out.str());
: }
Given how often this block is repeated, if we can reduce some of the lines of code via lambda, we should.
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc@559
PS3, Line 559: opts.entity_attrs.emplace_back(entity_attr_key1);
: opts.entity_attrs.emplace_back(entity_attr_val1);
: opts.entity_attrs.emplace_back(entity_attr_key2);
: opts.entity_attrs.emplace_back(entity_attr_val2);
: opts.entity_attrs.emplace_back(entity_attr_key3);
: opts.entity_attrs.emplace_back(entity_attr_val3);
You should be able to do something like:
opts.entity_attrs = { "...", "...", "..." }
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h@445
PS3, Line 445: '*'
Do we still need to support this? Seems like 'empty' is a simpler way to express matching all, and I don't believe we ever exposed '*' in a wire protocol.
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h@452
PS3, Line 452: std::vector<std::string> entity_attrs;
This is more special than the others. Please doc how it works.
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.cc@187
PS3, Line 187: bool MatchMetricInList(const string& metric_name,
This should probably be renamed (and maybe rename some of its variables/comments) to reflect its more generic nature. That is, we're not just using it to match metrics; we're now using it to match arbitrary strings.
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.cc@225
PS3, Line 225: else if (opts.entity_attrs.size() % 2 != 0) {
: return Status::OK();
: }
Why do we exit early if there's an odd number of attribute filters?
Oh, it looks like that's because of the special meaning of entity_attrs: the number of entries should always be even because each pair represents a key and a value. Please doc this and check for it upfront, in MetricRegistry::WriteAsJSON. Should probably return an error or something if it's invalid.
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.cc@249
PS3, Line 249: vector<string> attr_val;
No need for this to be a vector; there's always just one entry. On L254 you can do { attr_val } to get a temporary vector for MatchMetricInList.
--
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: 3
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 22 May 2019 18:27:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
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 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13376/2/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/13376/2/src/kudu/util/metrics.h@409
PS2, Line 409: struct MetricJsonOptions {
> Can you move this functionality into MetricJsonOptions? I don't really see
Done
http://gerrit.cloudera.org:8080/#/c/13376/2/src/kudu/util/metrics.h@666
PS2, Line 666: // For each registered entity, retires orphaned metrics. If an entity has no more
> Since 'opts' is going to gain more advanced filtering capabilities, would b
Done
http://gerrit.cloudera.org:8080/#/c/13376/1/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/13376/1/src/kudu/util/metrics.cc@220
PS1, Line 220: // Quick check the 'attributes'.
> This line of code is a bit strange, except that it skips matching the follo
Ah, i see. It is used to filter entity id. :)
http://gerrit.cloudera.org:8080/#/c/13376/2/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/13376/2/src/kudu/util/metrics.cc@205
PS2, Line 205: } // anonymous namespace
> Can we make this patch more generic to matching entity types/entity ids? Th
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: 3
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Tue, 21 May 2019 11:56:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
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 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13376/6//COMMIT_MSG
Commit Message:
PS6:
> This should be updated to reflect the "type" -> "types" and "id" -> "ids"
ah, i forgot to update it.
http://gerrit.cloudera.org:8080/#/c/13376/6/src/kudu/server/default_path_handlers.cc
File src/kudu/server/default_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/13376/6/src/kudu/server/default_path_handlers.cc@327
PS6, Line 327: bool* value
> It'd be easier to just return this.
Done
http://gerrit.cloudera.org:8080/#/c/13376/6/src/kudu/server/default_path_handlers.cc@333
PS6, Line 333: vector<string>* value
> Likewise, it'd be easier to just return this.
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;
> I see. I don't think that makes sense: the options here are for callers to
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: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Sat, 25 May 2019 07:36:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Support for table and tablet filtering in metrics
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13376 )
Change subject: [util] Support for table and tablet filtering in metrics
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13376/2/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/13376/2/src/kudu/util/metrics.cc@205
PS2, Line 205: bool MatchTableInList(const string& table_id, const vector<string>& table_ids) {
Can we make this patch more generic to matching entity types/entity ids? The kudu/util/ package is meant to be generic/reusable across projects (eg Impala shares this code). Impala isn't currently using our metrics system but would prefer to keep it generic and reusable.
--
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: 2
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Mon, 20 May 2019 16:21:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Support for entity types/ids/attrs/metrics filtering in metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13376 )
Change subject: [util] Support for entity types/ids/attrs/metrics filtering in metrics
......................................................................
[util] Support for entity types/ids/attrs/metrics filtering in metrics
It takes a long time to display the metrics while there are
lots of tablets on the tserver, especially in web browsers.
So, I think it is good to support for entity types/ids/attrs/metrics
filtering in metrics.
Usage:
http://x.x.x.x:8050/metrics?types=type1,type2&ids=id1,id2&attributes=
table_name,table1,table_name,table2&metrics=on_disk_size,rows_inserted,
rows_upserted
Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
Reviewed-on: http://gerrit.cloudera.org:8080/13376
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/diagnostics_log.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 269 insertions(+), 85 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
Gerrit-Change-Number: 13376
Gerrit-PatchSet: 9
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13376
to look at the new patch set (#7).
Change subject: [util] Support for entity type/id/attrs/metrics filtering in metrics
......................................................................
[util] Support for entity type/id/attrs/metrics filtering in metrics
It takes a long time to display the metrics while there are
lots of tablets on the tserver, especially in web browsers.
So, I think it is good to support for entity type/id/attrs/metrics
filtering in metrics.
Usage:
http://x.x.x.x:8050/metrics?types=type1,type2&ids=id1,id2&attributes=
table_name,table1,table_name,table2&metrics=on_disk_size,rows_inserted,
rows_upserted
Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/diagnostics_log.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 269 insertions(+), 85 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/13376/7
--
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: newpatchset
Gerrit-Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
Gerrit-Change-Number: 13376
Gerrit-PatchSet: 7
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [util] Support for table and tablet filtering in metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13376 )
Change subject: [util] Support for table and tablet filtering in metrics
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13376/2/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/13376/2/src/kudu/util/metrics.h@409
PS2, Line 409: struct MetricFilterOptions {
Can you move this functionality into MetricJsonOptions? I don't really see the value of separating the two kinds of options.
http://gerrit.cloudera.org:8080/#/c/13376/2/src/kudu/util/metrics.h@666
PS2, Line 666: const std::vector<std::string>& requested_metrics,
Since 'opts' is going to gain more advanced filtering capabilities, would be nice to move requested_metrics in there too, so that all the filtering is described in one place.
--
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: 2
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Mon, 20 May 2019 17:54:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
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 3:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/server/default_path_handlers.cc
File src/kudu/server/default_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/server/default_path_handlers.cc@342
PS3, Line 342: // 'type'
: {
: const string* arg = FindOrNull(req.parsed_args, "type");
: if (arg != nullptr) {
: SplitStringUsing(*arg, ",", &opts.entity_types);
: }
: }
> Could you decompose this into a helper function to avoid repeating the same
Done
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc
File src/kudu/util/metrics-test.cc:
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc@407
PS3, Line 407: METRIC_DEFINE_entity(tablet);
: METRIC_DEFINE_counter(tablet, rows_inserted, "Rows Inserted", MetricUnit::kRows,
: "Number of rows inserted into this tablet since service start");
: METRIC_DEFINE_counter(tablet, rows_upserted, "Rows Upserted", kudu::MetricUnit::kRows,
: "Number of rows upserted into this tablet since service start");
> Could you reuse the entity/counter already defined by this test? (L55, L70,
Done
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc@419
PS3, Line 419: const auto GenMetrics = [&] () {
> GenMetrics and CheckMetrics are only called once each, so why bother wrappi
Done
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc@443
PS3, Line 443: {
: const string entity_type = "not_exist_type";
: std::ostringstream out;
: MetricJsonOptions opts;
: opts.entity_types.emplace_back(entity_type);
: JsonWriter w(&out, JsonWriter::PRETTY);
: ASSERT_OK(registry_.WriteAsJson(&w, opts));
: ASSERT_EQ("[]", out.str());
: }
> Given how often this block is repeated, if we can reduce some of the lines
Done
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc@559
PS3, Line 559: opts.entity_attrs.emplace_back(entity_attr_key1);
: opts.entity_attrs.emplace_back(entity_attr_val1);
: opts.entity_attrs.emplace_back(entity_attr_key2);
: opts.entity_attrs.emplace_back(entity_attr_val2);
: opts.entity_attrs.emplace_back(entity_attr_key3);
: opts.entity_attrs.emplace_back(entity_attr_val3);
> You should be able to do something like:
Done
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h@445
PS3, Line 445: '*'
> Do we still need to support this? Seems like 'empty' is a simpler way to ex
I have no idea whether we exposed '*' in a wire protocol, but in the code before the change, '*' is supported, please look at L353/356 in default_path_handlers.cc.
Right now after the change, there are 4 cases:
(1) empty/none;
(2) filter=*
(3) filter=a,b,c,d
(4) filter=
case2 is the same as case3, but will match all.
case4 is special in the original code, the metrics of every entity is empty when we call "http://x.x.x.x:8050/metrics?metrics=". That means metrics are omitted. So, i will keep this design if you don't mind.
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h@452
PS3, Line 452: std::vector<std::string> entity_attrs;
> This is more special than the others. Please doc how it works.
Done
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.cc@187
PS3, Line 187: bool MatchMetricInList(const string& metric_name,
> This should probably be renamed (and maybe rename some of its variables/com
Done
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.cc@225
PS3, Line 225: else if (opts.entity_attrs.size() % 2 != 0) {
: return Status::OK();
: }
> Why do we exit early if there's an odd number of attribute filters?
Done
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.cc@249
PS3, Line 249: vector<string> attr_val;
> No need for this to be a vector; there's always just one entry. On L254 you
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: 3
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 23 May 2019 03:11:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Support for entity types/ids/attrs/metrics filtering in metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13376 )
Change subject: [util] Support for entity types/ids/attrs/metrics filtering in metrics
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
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: deleteReviewer
Gerrit-Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
Gerrit-Change-Number: 13376
Gerrit-PatchSet: 8
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13376
to look at the new patch set (#4).
Change subject: [util] Support for entity type/id/attrs/metrics filtering in metrics
......................................................................
[util] Support for entity type/id/attrs/metrics filtering in metrics
It takes a long time to display the metrics while there are
lots of tablets on the tserver, especially in web browsers.
So, I think it is good to support for entity type/id/attrs/metrics
filtering in metrics.
Usage:
http://x.x.x.x:8050/metrics?type=type1,type2&id=id1,id2&attributes=
table_name,table1,table_name,table2&metrics=on_disk_size,rows_inserted,
rows_upserted
Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
---
M src/kudu/master/master.proto
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/diagnostics_log.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
7 files changed, 289 insertions(+), 86 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/13376/4
--
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: newpatchset
Gerrit-Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
Gerrit-Change-Number: 13376
Gerrit-PatchSet: 4
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h@445
PS3, Line 445: '*'
> Hmm, what's the point of all this versatility though? Why have multiple way
Great! That really simplifies the code and test cases indeed. I will submit the patch later.^_^
--
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: 3
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 23 May 2019 06:41:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13376
to look at the new patch set (#6).
Change subject: [util] Support for entity type/id/attrs/metrics filtering in metrics
......................................................................
[util] Support for entity type/id/attrs/metrics filtering in metrics
It takes a long time to display the metrics while there are
lots of tablets on the tserver, especially in web browsers.
So, I think it is good to support for entity type/id/attrs/metrics
filtering in metrics.
Usage:
http://x.x.x.x:8050/metrics?type=type1,type2&id=id1,id2&attributes=
table_name,table1,table_name,table2&metrics=on_disk_size,rows_inserted,
rows_upserted
Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/diagnostics_log.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 273 insertions(+), 87 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/13376/6
--
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: newpatchset
Gerrit-Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
Gerrit-Change-Number: 13376
Gerrit-PatchSet: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13376
to look at the new patch set (#3).
Change subject: [util] Support for entity type/id/attrs/metrics filtering in metrics
......................................................................
[util] Support for entity type/id/attrs/metrics filtering in metrics
It takes a long time to display the metrics while there are
lots of tablets on the tserver, especially in web browsers.
So, I think it is good to support for entity type/id/attrs/metrics
filtering in metrics.
Usage:
http://x.x.x.x:8050/metrics?type=type1,type2&id=id1,id2&attributes=
table_name,table1,table_name,table2&metrics=on_disk_size,rows_inserted,
rows_upserted
Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/diagnostics_log.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 379 insertions(+), 65 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/13376/3
--
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: newpatchset
Gerrit-Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
Gerrit-Change-Number: 13376
Gerrit-PatchSet: 3
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [util] Support for entity types/ids/attrs/metrics filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13376
to look at the new patch set (#8).
Change subject: [util] Support for entity types/ids/attrs/metrics filtering in metrics
......................................................................
[util] Support for entity types/ids/attrs/metrics filtering in metrics
It takes a long time to display the metrics while there are
lots of tablets on the tserver, especially in web browsers.
So, I think it is good to support for entity types/ids/attrs/metrics
filtering in metrics.
Usage:
http://x.x.x.x:8050/metrics?types=type1,type2&ids=id1,id2&attributes=
table_name,table1,table_name,table2&metrics=on_disk_size,rows_inserted,
rows_upserted
Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/diagnostics_log.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 269 insertions(+), 85 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/13376/8
--
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: newpatchset
Gerrit-Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
Gerrit-Change-Number: 13376
Gerrit-PatchSet: 8
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13376
to look at the new patch set (#5).
Change subject: [util] Support for entity type/id/attrs/metrics filtering in metrics
......................................................................
[util] Support for entity type/id/attrs/metrics filtering in metrics
It takes a long time to display the metrics while there are
lots of tablets on the tserver, especially in web browsers.
So, I think it is good to support for entity type/id/attrs/metrics
filtering in metrics.
Usage:
http://x.x.x.x:8050/metrics?type=type1,type2&id=id1,id2&attributes=
table_name,table1,table_name,table2&metrics=on_disk_size,rows_inserted,
rows_upserted
Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/diagnostics_log.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 280 insertions(+), 86 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/13376/5
--
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: newpatchset
Gerrit-Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee
Gerrit-Change-Number: 13376
Gerrit-PatchSet: 5
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
[kudu-CR] [util] Support for entity types/ids/attrs/metrics filtering in metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13376 )
Change subject: [util] Support for entity types/ids/attrs/metrics filtering in metrics
......................................................................
Patch Set 8: Verified+1 Code-Review+2
Overriding Jenkins, failure was a known Java flake.
--
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: 8
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Sun, 26 May 2019 18:10:21 +0000
Gerrit-HasComments: No
[kudu-CR] [util] Support for table and tablet filtering in metrics
Posted by "helifu (Code Review)" <ge...@cloudera.org>.
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13376 )
Change subject: [util] Support for table and tablet filtering in metrics
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13376/1/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/13376/1/src/kudu/util/metrics.cc@220
PS1, Line 220: bool select_all = MatchMetricInList(id(), requested_metrics);
This line of code is a bit strange, except that it skips matching the following metrics. What do you think?
--
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: 1
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Mon, 20 May 2019 06:45:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h@445
PS3, Line 445: '*'
> I have no idea whether we exposed '*' in a wire protocol, but in the code b
Hmm, what's the point of all this versatility though? Why have multiple ways to express the same thing? It just makes for more complicated code/tests. Wildcard was ever exposed in the web UI; it was only part of the internal API. And I'm not convinced that case 4 was an intentional use case; it seems like it may have been completely accidental as it's not specified in the various comments at all.
Basically I think we should keep cases 1 and 3 and drop the rest. It should yield simpler code and fewer things to test.
--
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: 3
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 23 May 2019 06:14:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Support for entity type/id/attrs/metrics filtering in metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13376/6//COMMIT_MSG
Commit Message:
PS6:
This should be updated to reflect the "type" -> "types" and "id" -> "ids" conversion you did.
http://gerrit.cloudera.org:8080/#/c/13376/6/src/kudu/server/default_path_handlers.cc
File src/kudu/server/default_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/13376/6/src/kudu/server/default_path_handlers.cc@327
PS6, Line 327: bool* value
It'd be easier to just return this.
http://gerrit.cloudera.org:8080/#/c/13376/6/src/kudu/server/default_path_handlers.cc@333
PS6, Line 333: vector<string>* value
Likewise, it'd be easier to just return this.
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;
> Yes, it is used in the default_path_handlers.cc(L356) while initializing a
I see. I don't think that makes sense: the options here are for callers to control the behavior for WriteAsJson, and 'compact' isn't used by WriteAsJson; it's just used by the caller itself.
Could you put compact back to the way it used to be, in default-path-handlers.cc only?
--
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: 6
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Fri, 24 May 2019 18:23:25 +0000
Gerrit-HasComments: Yes