You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yifan Zhang (Code Review)" <ge...@cloudera.org> on 2019/05/30 09:18:44 UTC
[kudu-CR] metrics-test: fix wrong metric name and improve metric filtering test
Yifan Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13471
Change subject: metrics-test: fix wrong metric name and improve metric filtering test
......................................................................
metrics-test: fix wrong metric name and improve metric filtering test
This patch fixes wrong metric name in MetricsTest.JsonPrintTest
and add another metric to test whether metric filtering works.
Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
---
M src/kudu/util/metrics-test.cc
1 file changed, 11 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/13471/1
--
To view, visit http://gerrit.cloudera.org:8080/13471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
Gerrit-Change-Number: 13471
Gerrit-PatchSet: 1
Gerrit-Owner: Yifan Zhang <ch...@163.com>
[kudu-CR] metrics-test: fix wrong metric name and improve metric filtering test
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13471
to look at the new patch set (#2).
Change subject: metrics-test: fix wrong metric name and improve metric filtering test
......................................................................
metrics-test: fix wrong metric name and improve metric filtering test
This patch fixes wrong metric name in MetricsTest.JsonPrintTest,
it should match as a case-insensitive substring of "test_counter"
instead of "My Test Counter".
Besides, this patch also adds another metric test_gauge to test
whether metric filtering works.
Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
---
M src/kudu/util/metrics-test.cc
1 file changed, 26 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/13471/2
--
To view, visit http://gerrit.cloudera.org:8080/13471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
Gerrit-Change-Number: 13471
Gerrit-PatchSet: 2
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] metrics-test: fix wrong metric name and improve metric filtering test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13471 )
Change subject: metrics-test: fix wrong metric name and improve metric filtering test
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
Gerrit-Change-Number: 13471
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] metrics-test: fix wrong metric name and improve metric filtering test
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13471 )
Change subject: metrics-test: fix wrong metric name and improve metric filtering test
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13471/2/src/kudu/util/metrics-test.cc
File src/kudu/util/metrics-test.cc:
http://gerrit.cloudera.org:8080/#/c/13471/2/src/kudu/util/metrics-test.cc@204
PS2, Line 204: // Ma
> Add a 'using' for this.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
Gerrit-Change-Number: 13471
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Sat, 01 Jun 2019 16:31:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] metrics-test: fix wrong metric name and improve metric filtering test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13471 )
Change subject: metrics-test: fix wrong metric name and improve metric filtering test
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13471/1/src/kudu/util/metrics-test.cc
File src/kudu/util/metrics-test.cc:
http://gerrit.cloudera.org:8080/#/c/13471/1/src/kudu/util/metrics-test.cc@219
PS1, Line 219: const rapidjson::Value* attributes;
> The metric name of test_counter is wrong, it is "test_counter" instead of "
Ack
http://gerrit.cloudera.org:8080/#/c/13471/1/src/kudu/util/metrics-test.cc@238
PS1, Line 238: out.str("");
> It should match "test_counter".
Ah, I see what you mean: we were trying to match the metric _label_, but matching happens on _names_.
http://gerrit.cloudera.org:8080/#/c/13471/2/src/kudu/util/metrics-test.cc
File src/kudu/util/metrics-test.cc:
http://gerrit.cloudera.org:8080/#/c/13471/2/src/kudu/util/metrics-test.cc@204
PS2, Line 204: std::
Add a 'using' for this.
--
To view, visit http://gerrit.cloudera.org:8080/13471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
Gerrit-Change-Number: 13471
Gerrit-PatchSet: 2
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Fri, 31 May 2019 20:10:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] metrics-test: fix wrong metric name and improve metric filtering test
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13471
to look at the new patch set (#3).
Change subject: metrics-test: fix wrong metric name and improve metric filtering test
......................................................................
metrics-test: fix wrong metric name and improve metric filtering test
This patch fixes wrong metric name in MetricsTest.JsonPrintTest,
it should match as a case-insensitive substring of "test_counter"
instead of "My Test Counter".
Besides, this patch also adds another metric test_gauge to test
whether metric filtering works.
Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
---
M src/kudu/util/metrics-test.cc
1 file changed, 27 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/13471/3
--
To view, visit http://gerrit.cloudera.org:8080/13471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
Gerrit-Change-Number: 13471
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] metrics-test: fix wrong metric name and improve metric filtering test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13471 )
Change subject: metrics-test: fix wrong metric name and improve metric filtering test
......................................................................
Patch Set 3: Verified+1 Code-Review+2
Overriding Jenkins, failure was unrelated.
--
To view, visit http://gerrit.cloudera.org:8080/13471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
Gerrit-Change-Number: 13471
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Sun, 02 Jun 2019 05:34:48 +0000
Gerrit-HasComments: No
[kudu-CR] metrics-test: fix wrong metric name and improve metric filtering test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13471 )
Change subject: metrics-test: fix wrong metric name and improve metric filtering test
......................................................................
metrics-test: fix wrong metric name and improve metric filtering test
This patch fixes wrong metric name in MetricsTest.JsonPrintTest,
it should match as a case-insensitive substring of "test_counter"
instead of "My Test Counter".
Besides, this patch also adds another metric test_gauge to test
whether metric filtering works.
Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
Reviewed-on: http://gerrit.cloudera.org:8080/13471
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/util/metrics-test.cc
1 file changed, 27 insertions(+), 9 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/13471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
Gerrit-Change-Number: 13471
Gerrit-PatchSet: 4
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
[kudu-CR] metrics-test: fix wrong metric name and improve metric filtering test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13471 )
Change subject: metrics-test: fix wrong metric name and improve metric filtering test
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13471/1//COMMIT_MSG
Commit Message:
PS1:
The reversal of arguments in ASSERT_STR_CONTAINS within metrics-test makes sense, but is this fixing something else too? Am I missing something?
http://gerrit.cloudera.org:8080/#/c/13471/1/src/kudu/util/metrics-test.cc
File src/kudu/util/metrics-test.cc:
http://gerrit.cloudera.org:8080/#/c/13471/1/src/kudu/util/metrics-test.cc@204
PS1, Line 204: ASSERT_EQ("test_counter", metric_name);
Does WriteAsJson guarantee some deterministic order to the output? Otherwise how do we know that metrics[1] is the counter and not the gauge?
http://gerrit.cloudera.org:8080/#/c/13471/1/src/kudu/util/metrics-test.cc@219
PS1, Line 219: opts.entity_metrics.emplace_back("test_count");
Likewise, how will this match given the substring?
http://gerrit.cloudera.org:8080/#/c/13471/1/src/kudu/util/metrics-test.cc@238
PS1, Line 238: opts.entity_metrics.emplace_back("teST_coUNteR");
Given the underscore, how can this match as a case-insensitive substring of "My Test Counter"?
--
To view, visit http://gerrit.cloudera.org:8080/13471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
Gerrit-Change-Number: 13471
Gerrit-PatchSet: 1
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 30 May 2019 23:48:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] metrics-test: fix wrong metric name and improve metric filtering test
Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13471 )
Change subject: metrics-test: fix wrong metric name and improve metric filtering test
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13471/1//COMMIT_MSG
Commit Message:
PS1:
> The reversal of arguments in ASSERT_STR_CONTAINS within metrics-test makes
The string used to test metric name filter is wrong, it should be "test_counter" but not "My Test Counter".
http://gerrit.cloudera.org:8080/#/c/13471/1/src/kudu/util/metrics-test.cc
File src/kudu/util/metrics-test.cc:
http://gerrit.cloudera.org:8080/#/c/13471/1/src/kudu/util/metrics-test.cc@204
PS1, Line 204: std::unordered_map<string, int64_t> metric_map;
> Does WriteAsJson guarantee some deterministic order to the output? Otherwis
Done
http://gerrit.cloudera.org:8080/#/c/13471/1/src/kudu/util/metrics-test.cc@219
PS1, Line 219: const rapidjson::Value* attributes;
> Likewise, how will this match given the substring?
The metric name of test_counter is wrong, it is "test_counter" instead of "my test counter". if
use "test count" to filter metrics, we only got
a empty string, that is why the order of arguments in ASSERT_STR_CONTAINS is wrong and the test could still pass.
http://gerrit.cloudera.org:8080/#/c/13471/1/src/kudu/util/metrics-test.cc@238
PS1, Line 238: out.str("");
> Given the underscore, how can this match as a case-insensitive substring of
It should match "test_counter".
--
To view, visit http://gerrit.cloudera.org:8080/13471
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iec50aa2364d5d5f98a89d4aa7a3b213a39afd569
Gerrit-Change-Number: 13471
Gerrit-PatchSet: 2
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Comment-Date: Fri, 31 May 2019 11:32:27 +0000
Gerrit-HasComments: Yes