You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2019/12/13 11:35:52 UTC
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14903
Change subject: [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
......................................................................
[metrics] Support SUM, MAX and MIN merge types for AtomicGauge
Now AtomicGauge type metric only support SUM semantic when merge,
some type of metics like timestamp, may need MAX or MIN semantic when
merge.
Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
---
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.h
2 files changed, 75 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/03/14903/1
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14903 )
Change subject: [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
......................................................................
Patch Set 2:
(4 comments)
Will you be using this in follow-up commits? Which metrics are you thinking of changing?
http://gerrit.cloudera.org:8080/#/c/14903/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14903/2//COMMIT_MSG@10
PS2, Line 10: metics
metrics
http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics-test.cc
File src/kudu/util/metrics-test.cc:
http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics-test.cc@270
PS2, Line 270: ASSERT_EQ(2, mem_usage_for_merge->value());
Could you add to this a decrement of mem_usage to a value below 2 and show that mem_usage_for_merge drops too?
http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics.h@434
PS2, Line 434: enum class MergeType {
Add a comment explaining what this enum means and a high-level description of its effects?
http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics.h@1130
PS2, Line 1130: down_cast<AtomicGauge<T>*>(other.get())->value()
This is repeated across all three cases; do it once and store in a local variable?
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 13 Dec 2019 20:05:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for metrics
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14903 )
Change subject: [metrics] Support SUM, MAX and MIN merge types for metrics
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14903/4/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14903/4/src/kudu/util/metrics.h@1147
PS4, Line 1147: LOG(FATAL) << "Unknown AtomicGauge type: " << prototype()->name();
nit: maybe also include the 'type_' value, in case this ever triggers and we need to debug. Same with L1306
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 17 Dec 2019 20:22:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for metrics
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14903 )
Change subject: [metrics] Support SUM, MAX and MIN merge types for metrics
......................................................................
[metrics] Support SUM, MAX and MIN merge types for metrics
Now metrics only support SUM semantic when merge, some type of
metrics like timestamp, may need MAX or MIN semantic when merge.
Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Reviewed-on: http://gerrit.cloudera.org:8080/14903
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.h
2 files changed, 109 insertions(+), 26 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, but someone else must approve
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14903 )
Change subject: [metrics] Support SUM, MAX and MIN merge types for metrics
......................................................................
Patch Set 4: Code-Review+2
We can merge this, though I think your follow-up patch ends up not needing MAX or MIN merging semantics, right?
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 17 Dec 2019 21:42:24 +0000
Gerrit-HasComments: No
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14903 )
Change subject: [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
......................................................................
Patch Set 2:
> Patch Set 2:
>
> (4 comments)
>
> Will you be using this in follow-up commits? Which metrics are you thinking of changing?
Yes. I want to add a metric like 'last_consult_time' to tablets, represent the last write or scan timestamp of a tablet. We can judge whether a tablet is 'hot' or 'cold' by this metric, and it need a 'MAX' type when merge into table level metric.
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sat, 14 Dec 2019 00:43:23 +0000
Gerrit-HasComments: No
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14903
to look at the new patch set (#2).
Change subject: [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
......................................................................
[metrics] Support SUM, MAX and MIN merge types for AtomicGauge
Now AtomicGauge type metric only support SUM semantic when merge,
some type of metics like timestamp, may need MAX or MIN semantic when
merge.
Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
---
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.h
2 files changed, 76 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/03/14903/2
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14903
to look at the new patch set (#3).
Change subject: [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
......................................................................
[metrics] Support SUM, MAX and MIN merge types for AtomicGauge
Now AtomicGauge type metric only support SUM semantic when merge,
some type of metrics like timestamp, may need MAX or MIN semantic when
merge.
Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
---
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.h
2 files changed, 83 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/03/14903/3
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14903 )
Change subject: [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
......................................................................
Patch Set 3:
(4 comments)
> Patch Set 2:
>
> +1 to all Adar's comments. I'm also curious which metrics you intend on updating the merge policies for.
A metric like 'last_consult_time' of a tablet.
http://gerrit.cloudera.org:8080/#/c/14903/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14903/2//COMMIT_MSG@10
PS2, Line 10: metric
> metrics
Done
http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics-test.cc
File src/kudu/util/metrics-test.cc:
http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics-test.cc@270
PS2, Line 270: ASSERT_EQ(1, start_time_for_merge->value());
> Could you add to this a decrement of mem_usage to a value below 2 and show
Done
http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics.h@434
PS2, Line 434: // Type of behavior when two metrics merge together, it only take effect on the result
> Add a comment explaining what this enum means and a high-level description
Done
http://gerrit.cloudera.org:8080/#/c/14903/2/src/kudu/util/metrics.h@1130
PS2, Line 1130: NeededInMerge(other)) {
> This is repeated across all three cases; do it once and store in a local va
Done
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sat, 14 Dec 2019 06:33:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14903
to look at the new patch set (#4).
Change subject: [metrics] Support SUM, MAX and MIN merge types for metrics
......................................................................
[metrics] Support SUM, MAX and MIN merge types for metrics
Now metrics only support SUM semantic when merge, some type of
metrics like timestamp, may need MAX or MIN semantic when merge.
Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
---
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.h
2 files changed, 109 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/03/14903/4
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14903 )
Change subject: [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
......................................................................
Patch Set 2:
+1 to all Adar's comments. I'm also curious which metrics you intend on updating the merge policies for.
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 13 Dec 2019 23:31:18 +0000
Gerrit-HasComments: No
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14903 )
Change subject: [metrics] Support SUM, MAX and MIN merge types for AtomicGauge
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14903/3/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14903/3/src/kudu/util/metrics.h@436
PS3, Line 436: // NOTE: Now only AtomicGauge support it.
I don't think this note is necessary; it's obvious from inspection that only AtomicGauge uses the MergeType argument, and this comment will become stale and will need to be updated if that changes.
http://gerrit.cloudera.org:8080/#/c/14903/3/src/kudu/util/metrics.h@442
PS3, Line 442: miniimum
minimum
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Mon, 16 Dec 2019 21:15:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Support SUM, MAX and MIN merge types for metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14903 )
Change subject: [metrics] Support SUM, MAX and MIN merge types for metrics
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14903/4/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14903/4/src/kudu/util/metrics.h@1147
PS4, Line 1147: LOG(FATAL) << "Unknown AtomicGauge type: " << prototype()->name();
> nit: maybe also include the 'type_' value, in case this ever triggers and w
‘type_’ is an enum class value, which is not convenient to logging, and this line is unreachable.
http://gerrit.cloudera.org:8080/#/c/14903/3/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14903/3/src/kudu/util/metrics.h@436
PS3, Line 436: enum class MergeType {
> I don't think this note is necessary; it's obvious from inspection that onl
Done
http://gerrit.cloudera.org:8080/#/c/14903/3/src/kudu/util/metrics.h@442
PS3, Line 442:
> minimum
Done
--
To view, visit http://gerrit.cloudera.org:8080/14903
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1acad40c2b73cfeb16e0dc4ad5ffba4f19167c81
Gerrit-Change-Number: 14903
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 18 Dec 2019 02:23:48 +0000
Gerrit-HasComments: Yes