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/08/23 07:59:41 UTC
[kudu-CR] [metrics] Add a metric to count sub-entities when merge metrics
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14128
Change subject: [metrics] Add a metric to count sub-entities when merge metrics
......................................................................
[metrics] Add a metric to count sub-entities when merge metrics
When metrics entity merged to a new entity, we don't know how many sub
entities it merged from.
This patch add a new metric named 'sub_entities_count' to the new merged
entity to count sub entities, we can use this information to check whether
a table is balance by thirdparty monitor system.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
2 files changed, 47 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/1
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#12) to the change originally created by Yingchun Lai. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
[metrics] Add a metric to count merged entities when merge metrics
When metric entities merged to a new entity, we don't know how many
entities it's merged from.
This patch adds a new metric 'merged_entities_count_of_<entity>' to
entity and set value to 1, then the new merged entity will have the
same metric with value merged.
In Kudu, when we want to check whether tables are balanced or not on
tservers, and how many replicas distributed on each tserver, though
we can use CLI tool like 'kudu cluster rebalance' to check per-table
balance status of a cluster, but we have to fetch balance info table
by table (by add --tables=xx flag), and also we have to "run command"
in thirdparty monitor system and this tool seems not fast enough
because it will call functions like RetrieveAllTablets.
Now we can use this new feature to check whether a table is balanced
or not by thirdparty monitor system efficiently. For example, when
we fetch metrics from tserver by:
http://<host>:<port>/metrics?merge_rules=tablet|table|table_name
we can get merged metrics of table entity from each tserver as
mentioned in commit fe6e5cc0c9c1573de174d1ce7838b449373ae36e, and now
we can get an extra size type gauge metric
"merged_entities_count_of_tablet" that indicate how many replicas this
tserver hold of a table, then compare this metric of each tserver and
judge whether this table is balanced or not.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M build-support/iwyu.py
M src/kudu/master/catalog_manager.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/tablet.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
7 files changed, 53 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/12
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14128
to look at the new patch set (#10).
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
[metrics] Add a metric to count merged entities when merge metrics
When metric entities merged to a new entity, we don't know how many
entities it's merged from.
This patch adds a new metric 'merged_entities_count_of_<entity>' to
entity and set value to 1, then the new merged entity will have the
same metric with value merged.
In Kudu, when we want to check whether tables are balanced or not on
tservers, and how many replicas distributed on each tserver, though
we can use CLI tool like 'kudu cluster rebalance' to check per-table
balance status of a cluster, but we have to fetch balance info table
by table (by add --tables=xx flag), and also we have to "run command"
in thirdparty monitor system and this tool seems not fast enough
because it will call functions like RetrieveAllTablets.
Now we can use this new feature to check whether a table is balanced
or not by thirdparty monitor system efficiently. For example, when
we fetch metrics from tserver by:
http://<host>:<port>/metrics?merge_rules=tablet|table|table_name
we can get merged metrics of table entity from each tserver as
mentioned in commit fe6e5cc0c9c1573de174d1ce7838b449373ae36e, and now
we can get an extra size type gauge metric
"merged_entities_count_of_tablet" that indicate how many replicas this
tserver hold of a table, then compare this metric of each tserver and
judge whether this table is balanced or not.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M src/kudu/master/catalog_manager.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/tablet.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 48 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/10
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14128/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14128/7//COMMIT_MSG@14
PS7, Line 14: In Kudu, when we want to check whether tables are balanced or not on
: tservers, and how many replicas distributed
> Could you add more details on this? I think that might be a good how-to ex
Done
http://gerrit.cloudera.org:8080/#/c/14128/7/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/14128/7/src/kudu/tablet/tablet.cc@234
PS7, Line 234: InstantiateHidden(metric_entity_, 1);
: }
> Would it make sense to combine these two calls into a new method, something
InstantiateHidden() would be better I think, because it will appear either be merged or updated.
Done
http://gerrit.cloudera.org:8080/#/c/14128/7/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/7/src/kudu/util/metrics.h@265
PS7, Line 265: "Count of entities merged together when entities are " \
: "merged by common attribute value.");
:
> Reword as "Count of entities merged together when entities are merged by co
Done
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 06 Sep 2019 03:07:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 8: Code-Review+1
(4 comments)
I removed -1 vote due to strange IWYU issue. This looks good to me.
Maybe, Adar has some feedback as well.
http://gerrit.cloudera.org:8080/#/c/14128/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14128/7//COMMIT_MSG@14
PS7, Line 14: In Kudu, when we want to check whether tables are balanced or not on
: tservers, and how many replicas distributed
> Done
Great, thanks a lot!
http://gerrit.cloudera.org:8080/#/c/14128/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14128/8//COMMIT_MSG@10
PS8, Line 10: it
nit: it's
http://gerrit.cloudera.org:8080/#/c/14128/8//COMMIT_MSG@11
PS8, Line 11: add
nit: adds
http://gerrit.cloudera.org:8080/#/c/14128/8/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/8/src/kudu/util/metrics.h@785
PS8, Line 785: template<typename T>
: friend class GaugePrototype;
: FRIEND_TEST(MetricsTest, TestDumpOnlyChanged);
It seems IWYU incorrectly processes this. I think we can ignore that. Or, if it looks too messy to you, I'm OK keeping InvalidEpoch() public, but it's all up to you :)
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 06 Sep 2019 06:48:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (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/14128
to look at the new patch set (#5).
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
[metrics] Add a metric to count merged entities when merge metrics
When metric entities merged to a new entity, we don't know how many
entities it merged from.
This patch add a new metric 'merged_<entity>_entities_count' to entity
and set value to 1, then the new merged entity will have the same
metric with value merged.
We can use this information to check whether a table is balanced or
not by thirdparty monitor system efficiently. Though we can use CLI
tool like 'kudu cluster rebalance' to check per-table balance status
of a cluster, but we have to fetch balance info table by table (by add
--tables=xx flag), and also we have to "run command" in thirdparty
monitor system and this tool seems not fast enough because it will
call functions like RetrieveAllTablets.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M src/kudu/master/catalog_manager.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/tablet.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 48 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/5
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14128/7/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/7/src/kudu/util/metrics.h@265
PS7, Line 265: "When enabled metrics merge by some entity attribute, " \
: "and this is a merged entity, this metric identify " \
: "how many entities it merged from."
Reword as "Count of entities merged together when entities are merged by common attribute value".
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 05 Sep 2019 18:51:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14128/11/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/11/src/kudu/util/metrics.h@256
PS11, Line 256: // IWYU pragma: no_forward_declare kudu::MetricPrototype
: // IWYU pragma: no_forward_declare kudu::MetricPrototype::CtorArgs
: // IWYU pragma: no_forward_declare kudu::MetricUnit
> Ah, that's nice. But it seems this is not enough to help out IWYU with its
I've tried to make InvalidateEpoch() public again, but it didn't work, IWYU still report this message. :(
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 17:28:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (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/14128
to look at the new patch set (#4).
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
[metrics] Add a metric to count merged entities when merge metrics
When metric entities merged to a new entity, we don't know how many
entities it merged from.
This patch add a new metric named 'merged_entities_count' to the new merged
entity to count entities. For Kudu, we can use this information to
check whether a table is balanced or not by thirdparty monitor system.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M src/kudu/tablet/tablet.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
4 files changed, 30 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/4
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14128/3//COMMIT_MSG
Commit Message:
PS3:
> In discussing this, are you sure we want to introduce the concept of a "sub
I agree, I will do these changes.
http://gerrit.cloudera.org:8080/#/c/14128/3//COMMIT_MSG@13
PS3, Line 13: check whether a table is balanced or not by thirdparty monitor system.
> I saw this in my first pass but forgot to comment: how is this an effective
When we use rebalancer to check per-table balance status of a cluster, we have to fetch balance info table by table (by add --tables=xx flag), and also we have to "run command" in monitor system and this tool seems not fast enough because it will call function like RetrieveAllTablets.
When table level metrics "merged_entities_count" are included in metrics from tserver, different tserver may have different values of this metric for a table, we can check it and jugde whether it's balance or not.
http://gerrit.cloudera.org:8080/#/c/14128/3/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/14128/3/src/kudu/tablet/tablet.cc@163
PS3, Line 163: METRIC_DEFINE_gauge_size(tablet, merged_entities_count,
> Would it be possible to prevent the count from being displayed when regular
I've added a function "InvalidateEpoch()" to class Metric, which will set "m_epoch_" to -1. So if a metric is initialized and InvalidateEpoch()ed, but never changed, it's invisible.
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@479
PS2, Line 479: // metrics, we should keep them around until the next poll.
> Is tablet->table the only kind of merge allowed? Or is it possible to merge
It's not the only kind, we may perform more merge types.
Done
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 03 Sep 2019 16:09:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count sub-entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has restored this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count sub-entities when merge metrics
......................................................................
Restored
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: restore
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [metrics] Add a metric to count sub-entities when merge metrics
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/14128
to look at the new patch set (#2).
Change subject: [metrics] Add a metric to count sub-entities when merge metrics
......................................................................
[metrics] Add a metric to count sub-entities when merge metrics
When metrics entity merged to a new entity, we don't know how many sub
entities it merged from.
This patch add a new metric named 'sub_entities_count' to the new merged
entity to count sub entities. For Kudu, we can use this information to
check whether a table is balanced or not by thirdparty monitor system.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
3 files changed, 96 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/2
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 10:
Looks good but IWYU is still complaining. What happens if you add its recommendations?
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 00:58:58 +0000
Gerrit-HasComments: No
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14128/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14128/3//COMMIT_MSG@13
PS3, Line 13: check whether a table is balanced or not by thirdparty monitor system.
> When we use rebalancer to check per-table balance status of a cluster, we h
Thanks, that's useful to know. Maybe you can find a way to incorporate this explanation into your commit message?
http://gerrit.cloudera.org:8080/#/c/14128/4/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/4/src/kudu/util/metrics.h@737
PS4, Line 737: // Invalidate 'm_epoch_', then this metric is invisible until its value changed.
Nit: "Invalidate 'm_epoch_', causing this metric to be invisible until its value changes."
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@479
PS2, Line 479: // metrics, we should keep them around until the next poll.
> It's not the only kind, we may perform more merge types.
Hmm, I don't see this change done: I still see just the one static metric prototype for merging tablet entities.
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 03 Sep 2019 20:20:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14128/5/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14128/5/src/kudu/master/catalog_manager.cc@276
PS5, Line 276: METRIC_DEFINE_gauge_size(table, merged_table_entities_count,
: "Entities Count Merged From",
: kudu::MetricUnit::kEntries,
: "When enabled metrics merge by some entity attribute, "
: "and this is a merged entity, this metric identify "
: "how many entities it merged from.");
Instead of adding METRIC_DEFINE_gauge_size to every place where METRIC_DEFINE_entity is used, could you modify the macro definition for METRIC_DEFINE_entity to also call METRIC_DEFINE_gauge_size?
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 04 Sep 2019 05:56:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count sub-entities when merge metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count sub-entities when merge metrics
......................................................................
Patch Set 2:
(8 comments)
Didn't make it all the way through; wanted to see if we could avoid the dynamic metric prototype usage.
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.h@711
PS2, Line 711: class MergedEntityPrototypes {
Needs docs.
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.h@713
PS2, Line 713: static MergedEntityPrototypes* GetInstance() {
Nit: indented too much.
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@358
PS2, Line 358: entity_id = attrs[merge_rule->attribute_to_merge_by];
What happens if this map lookup fails? Seems like we'll set entity_id to an empty string; do we instead want to return an error?
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@375
PS2, Line 375: exist
existing
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@377
PS2, Line 377: FindPtrOrNull(entity_collection, entries_count_prototype).get());
This should be a FindOrDie variant if there's an expectation that the result always exists (and the code currently assumes that).
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@461
PS2, Line 461: MergedEntityPrototypes::~MergedEntityPrototypes() {
: for (const auto& prototype : prototypes_) {
: delete[] prototype.second.get()->entity_type();
: }
: }
An AutoReleasePool might be a more natural choice.
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@467
PS2, Line 467: std::
Drop the prefix.
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@479
PS2, Line 479: MetricPrototype::CtorArgs(entity_type_tmp,
This effectively adds "dynamic" metric prototype definition to the metrics library. Note that all prototypes are statically defined today (there's a comment near the top of metrics.h about this), and that has advantages: apart from the odd lifecycle semantics of dynamic prototypes, it also means it's really easy to generate a full schema up front using the prototypes and send that schema to monitoring tools.
In this particular case, "entity_type_tmp" will always be one of several entity types (i.e. "server", "tablet", and "table"), right? So perhaps we could statically define it too, by adjusting METRIC_DEFINE_entity() to also define a gauge prototype for the merged entity count?
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
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: Mon, 26 Aug 2019 22:44:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
[metrics] Add a metric to count merged entities when merge metrics
When metric entities merged to a new entity, we don't know how many
entities it's merged from.
This patch adds a new metric 'merged_entities_count_of_<entity>' to
entity and set value to 1, then the new merged entity will have the
same metric with value merged.
In Kudu, when we want to check whether tables are balanced or not on
tservers, and how many replicas distributed on each tserver, though
we can use CLI tool like 'kudu cluster rebalance' to check per-table
balance status of a cluster, but we have to fetch balance info table
by table (by add --tables=xx flag), and also we have to "run command"
in thirdparty monitor system and this tool seems not fast enough
because it will call functions like RetrieveAllTablets.
Now we can use this new feature to check whether a table is balanced
or not by thirdparty monitor system efficiently. For example, when
we fetch metrics from tserver by:
http://<host>:<port>/metrics?merge_rules=tablet|table|table_name
we can get merged metrics of table entity from each tserver as
mentioned in commit fe6e5cc0c9c1573de174d1ce7838b449373ae36e, and now
we can get an extra size type gauge metric
"merged_entities_count_of_tablet" that indicate how many replicas this
tserver hold of a table, then compare this metric of each tserver and
judge whether this table is balanced or not.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Reviewed-on: http://gerrit.cloudera.org:8080/14128
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M build-support/iwyu.py
M src/kudu/master/catalog_manager.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/tablet.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
7 files changed, 49 insertions(+), 11 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 14
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count sub-entities when merge metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count sub-entities when merge metrics
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14128/3//COMMIT_MSG
Commit Message:
PS3:
In discussing this, are you sure we want to introduce the concept of a "sub-entity"? It doesn't seem necessary to me: when talking about merging entities, when we're done merging, we publish a metric that describes the number of merged entities, a merged entity count, or something along those lines. "Sub-entity" muddies the waters: does this refer to a different kind of entity that is somehow subordinate to a regular entity?
If you agree, change 'sub_entities_count' to 'merged_entities_count' and adjust the rest of the commit message to reflect "merged" entities rather than "sub-entities". Plus whatever references exist in the code itself.
http://gerrit.cloudera.org:8080/#/c/14128/3//COMMIT_MSG@13
PS3, Line 13: check whether a table is balanced or not by thirdparty monitor system.
I saw this in my first pass but forgot to comment: how is this an effective proxy for table balance? Why not leverage the rebalancer?
http://gerrit.cloudera.org:8080/#/c/14128/3/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/14128/3/src/kudu/tablet/tablet.cc@163
PS3, Line 163: METRIC_DEFINE_gauge_size(tablet, sub_entities_count,
Would it be possible to prevent the count from being displayed when regular tablet metrics are emitted, and instead only show it after they're merged?
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@479
PS2, Line 479: // metrics, we should keep them around until the next poll.
> Thanks for your advice.
Is tablet->table the only kind of merge allowed? Or is it possible to merge table entities together? Or server entities?
Why not future-proof this by defining a "merged entity count" metric prototype whenever we define a new metric entity prototype?
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 27 Aug 2019 23:15:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 8: Verified+1
Overriding strange IWYU issue with friends declaration.
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 06 Sep 2019 06:49:49 +0000
Gerrit-HasComments: No
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 11: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14128/11/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/11/src/kudu/util/metrics.h@256
PS11, Line 256: // IWYU pragma: no_forward_declare kudu::MetricPrototype
: // IWYU pragma: no_forward_declare kudu::MetricPrototype::CtorArgs
: // IWYU pragma: no_forward_declare kudu::MetricUnit
Ah, that's nice. But it seems this is not enough to help out IWYU with its glitch.
It seems there only way around is to make InvalidateEpoch() public. As I understand, Adar is advocating for resolving it this way, and I'm OK with that as well.
Sorry to see IWYU gives so much trouble with making some methods of template classes non-public :(
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 17:11:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 10:
> Patch Set 10:
>
> > Looks good but IWYU is still complaining. What happens if you add
> > its recommendations?
>
> I went that route: compilation fails if doing so. I think those forward declarations confuse IWYU. The forward declarations are necessary if trying to keep InvalidateEpoch() as a non-public method: I recommended doing so.
The IWYU message seems caused by L262~L266, you can check that by patch set 7, however, I'm not sure how to fix it. :(
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 06:43:05 +0000
Gerrit-HasComments: No
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#13) to the change originally created by Yingchun Lai. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
[metrics] Add a metric to count merged entities when merge metrics
When metric entities merged to a new entity, we don't know how many
entities it's merged from.
This patch adds a new metric 'merged_entities_count_of_<entity>' to
entity and set value to 1, then the new merged entity will have the
same metric with value merged.
In Kudu, when we want to check whether tables are balanced or not on
tservers, and how many replicas distributed on each tserver, though
we can use CLI tool like 'kudu cluster rebalance' to check per-table
balance status of a cluster, but we have to fetch balance info table
by table (by add --tables=xx flag), and also we have to "run command"
in thirdparty monitor system and this tool seems not fast enough
because it will call functions like RetrieveAllTablets.
Now we can use this new feature to check whether a table is balanced
or not by thirdparty monitor system efficiently. For example, when
we fetch metrics from tserver by:
http://<host>:<port>/metrics?merge_rules=tablet|table|table_name
we can get merged metrics of table entity from each tserver as
mentioned in commit fe6e5cc0c9c1573de174d1ce7838b449373ae36e, and now
we can get an extra size type gauge metric
"merged_entities_count_of_tablet" that indicate how many replicas this
tserver hold of a table, then compare this metric of each tserver and
judge whether this table is balanced or not.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M build-support/iwyu.py
M src/kudu/master/catalog_manager.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/tablet.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
7 files changed, 49 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/13
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 13
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 20:44:36 +0000
Gerrit-HasComments: No
[kudu-CR] [metrics] Add a metric to count sub-entities when merge metrics
Posted by "Yingchun Lai (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/14128
to look at the new patch set (#3).
Change subject: [metrics] Add a metric to count sub-entities when merge metrics
......................................................................
[metrics] Add a metric to count sub-entities when merge metrics
When metrics entity merged to a new entity, we don't know how many sub
entities it merged from.
This patch add a new metric named 'sub_entities_count' to the new merged
entity to count sub entities. For Kudu, we can use this information to
check whether a table is balanced or not by thirdparty monitor system.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M src/kudu/tablet/tablet.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
3 files changed, 17 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/3
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 10: Verified+1
Overriding strange IWYU issue with friends declaration.
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 02:04:50 +0000
Gerrit-HasComments: No
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14128/5/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/14128/5/src/kudu/master/catalog_manager.cc@276
PS5, Line 276:
: using base::subtle::NoBarrier_CompareAndSwap;
: using base::subtle::NoBarrier_Load;
: using boost::make_optional;
: using boost::none;
: using boost::optional;
> Instead of adding METRIC_DEFINE_gauge_size to every place where METRIC_DEFI
Done
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 05 Sep 2019 11:32:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (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/14128
to look at the new patch set (#7).
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
[metrics] Add a metric to count merged entities when merge metrics
When metric entities merged to a new entity, we don't know how many
entities it merged from.
This patch add a new metric 'merged_<entity>_entities_count' to entity
and set value to 1, then the new merged entity will have the same
metric with value merged.
We can use this information to check whether a table is balanced or
not by thirdparty monitor system efficiently. Though we can use CLI
tool like 'kudu cluster rebalance' to check per-table balance status
of a cluster, but we have to fetch balance info table by table (by add
--tables=xx flag), and also we have to "run command" in thirdparty
monitor system and this tool seems not fast enough because it will
call functions like RetrieveAllTablets.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M src/kudu/master/catalog_manager.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/tablet.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 39 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/7
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14128/12/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/12/src/kudu/util/metrics.h@255
PS12, Line 255:
: // IWYU pragma: no_forward_declare kudu::MetricPrototype
: // IWYU pragma: no_forward_declare kudu::MetricPrototype::CtorArgs
: // IWYU pragma: no_forward_declare kudu::MetricUnit
> Should we remove this?
Yep, that makes sense: most likely the list of necessary IWYU pragmas will be different for next version of the tool.
Done.
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 19:14:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14128
to look at the new patch set (#9).
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
[metrics] Add a metric to count merged entities when merge metrics
When metric entities merged to a new entity, we don't know how many
entities it's merged from.
This patch adds a new metric 'merged_entities_count_of_<entity>' to
entity and set value to 1, then the new merged entity will have the
same metric with value merged.
In Kudu, when we want to check whether tables are balanced or not on
tservers, and how many replicas distributed on each tserver, though
we can use CLI tool like 'kudu cluster rebalance' to check per-table
balance status of a cluster, but we have to fetch balance info table
by table (by add --tables=xx flag), and also we have to "run command"
in thirdparty monitor system and this tool seems not fast enough
because it will call functions like RetrieveAllTablets.
Now we can use this new feature to check whether a table is balanced
or not by thirdparty monitor system efficiently. For example, when
we fetch metrics from tserver by:
http://<host>:<port>/metrics?merge_rules=tablet|table|table_name
we can get merged metrics of table entity from each tserver as
mentioned in commit fe6e5cc0c9c1573de174d1ce7838b449373ae36e, and now
we can get an extra size type gauge metric
"merged_entities_count_of_tablet" that indicate how many replicas this
tserver hold of a table, then compare this metric of each tserver and
judge whether this table is balanced or not.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M src/kudu/master/catalog_manager.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/tablet.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 48 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/9
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14128
to look at the new patch set (#8).
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
[metrics] Add a metric to count merged entities when merge metrics
When metric entities merged to a new entity, we don't know how many
entities it merged from.
This patch add a new metric 'merged_entities_count_of_<entity>' to
entity and set value to 1, then the new merged entity will have the
same metric with value merged.
In Kudu, when we want to check whether tables are balanced or not on
tservers, and how many replicas distributed on each tserver, though
we can use CLI tool like 'kudu cluster rebalance' to check per-table
balance status of a cluster, but we have to fetch balance info table
by table (by add --tables=xx flag), and also we have to "run command"
in thirdparty monitor system and this tool seems not fast enough
because it will call functions like RetrieveAllTablets.
Now we can use this new feature to check whether a table is balanced
or not by thirdparty monitor system efficiently. For example, when
we fetch metrics from tserver by:
http://<host>:<port>/metrics?merge_rules=tablet|table|table_name
we can get merged metrics of table entity from each tserver as
mentioned in commit fe6e5cc0c9c1573de174d1ce7838b449373ae36e, and now
we can get an extra size type gauge metric
"merged_entities_count_of_tablet" that indicate how many replicas this
tserver hold of a table, then compare this metric of each tserver and
judge whether this table is balanced or not.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M src/kudu/master/catalog_manager.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/tablet.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 48 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/8
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 8
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14128/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14128/9//COMMIT_MSG@11
PS9, Line 11: merged_entities_count_of_<entity>
Could we just name it 'merged_entities_count'? The entity type is implied from the owning entity, and each instance of this metric prototype will be scoped to a different entity prototype, so the fact that they all have the same metric name shouldn't matter, right?
http://gerrit.cloudera.org:8080/#/c/14128/9/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/9/src/kudu/util/metrics.h@905
PS9, Line 905:
: // Instantiate a "manual" gauge and hidden it, it will appear
: // when it update, or in the new merged entity.
"Instantiate a "manual" gauge and hide it. It will appear when its value is updated, or when its entity is merged."
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 06 Sep 2019 17:14:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14128/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14128/9//COMMIT_MSG@11
PS9, Line 11: merged_entities_count_of_<entity>
> Could we just name it 'merged_entities_count'? The entity type is implied f
Metrics defined by METRIC_DEFINE_gauge_size(name, merged_entities_count,..) is named METRIC_merged_entities_count, and METRIC_DEFINE_entity(name) is called in global scope, so there will be multiple definition of METRIC_merged_entities_count if we do it like this.
http://gerrit.cloudera.org:8080/#/c/14128/9/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/9/src/kudu/util/metrics.h@905
PS9, Line 905:
: // Instantiate a "manual" gauge and hidden it, it will appear
: // when it update, or in the new merged entity.
> "Instantiate a "manual" gauge and hide it. It will appear when its value is
Done
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Sat, 07 Sep 2019 01:45:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (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/14128
to look at the new patch set (#6).
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
[metrics] Add a metric to count merged entities when merge metrics
When metric entities merged to a new entity, we don't know how many
entities it merged from.
This patch add a new metric 'merged_<entity>_entities_count' to entity
and set value to 1, then the new merged entity will have the same
metric with value merged.
We can use this information to check whether a table is balanced or
not by thirdparty monitor system efficiently. Though we can use CLI
tool like 'kudu cluster rebalance' to check per-table balance status
of a cluster, but we have to fetch balance info table by table (by add
--tables=xx flag), and also we have to "run command" in thirdparty
monitor system and this tool seems not fast enough because it will
call functions like RetrieveAllTablets.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M src/kudu/master/catalog_manager.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/tablet.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 39 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/6
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 6
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14128/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14128/7//COMMIT_MSG@14
PS7, Line 14: We can use this information to check whether a table is balanced or
: not by thirdparty monitor system efficiently
Could you add more details on this? I think that might be a good how-to example for people who might be interested in using this newly introduced functionality (maybe, even with other metrics).
http://gerrit.cloudera.org:8080/#/c/14128/7/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/14128/7/src/kudu/tablet/tablet.cc@234
PS7, Line 234: Instantiate(metric_entity_, 1)
: ->InvalidateEpoch();
Would it make sense to combine these two calls into a new method, something like InstantiateHidden() / InstantiateAppearOnMerge() / InstantiateAppearOnUpdate() or alike?
If Metric::InvalidateEpoch() is not used anywhere else, it may be possible to keep it a private.
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Thu, 05 Sep 2019 23:56:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14128/11/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/11/src/kudu/util/metrics.h@256
PS11, Line 256: // IWYU pragma: no_forward_declare kudu::MetricPrototype
: // IWYU pragma: no_forward_declare kudu::MetricPrototype::CtorArgs
: // IWYU pragma: no_forward_declare kudu::MetricUnit
> I've tried to make InvalidateEpoch() public again, but it didn't work, IWYU
Whoops, indeed. This IWYU's glitch is annoying, probably it will be better with newer IWYU's version. For now, let's add this file into the IWYU's suppression list.
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 19:00:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count sub-entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count sub-entities when merge metrics
......................................................................
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.h@711
PS2, Line 711: // See documentation at the top of this file for information on metrics ownership.
> Needs docs.
Done
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.h@713
PS2, Line 713: public:
> Nit: indented too much.
Done
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@358
PS2, Line 358: if (!entity_id_ptr) {
> What happens if this map lookup fails? Seems like we'll set entity_id to an
We should return a NotFound error.
Done
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@375
PS2, Line 375:
> existing
Done
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@377
PS2, Line 377: }
> This should be a FindOrDie variant if there's an expectation that the resul
Done
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@461
PS2, Line 461: for (const auto& e : entities) {
: WARN_NOT_OK(e.second->WriteAsJson(writer, opts),
: Substitute("Failed to write entity $0 as JSON", e.second->id()));
: }
:
> An AutoReleasePool might be a more natural choice.
Useful util, thanks!
Done
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@467
PS2, Line 467:
> Drop the prefix.
Done
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@479
PS2, Line 479: // metrics, we should keep them around until the next poll.
> This effectively adds "dynamic" metric prototype definition to the metrics
Thanks for your advice.
And you remind me why not define 'sub_entities_count' as a tablet's metric? For tablet, it always be 1, when we merge tablets' metrics to a upper level entity (i.e. table type entity), the GaugePrototype<uint64_t> type will sum up all sub-entities automatically.
Done
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 27 Aug 2019 07:32:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14128
to look at the new patch set (#11).
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
[metrics] Add a metric to count merged entities when merge metrics
When metric entities merged to a new entity, we don't know how many
entities it's merged from.
This patch adds a new metric 'merged_entities_count_of_<entity>' to
entity and set value to 1, then the new merged entity will have the
same metric with value merged.
In Kudu, when we want to check whether tables are balanced or not on
tservers, and how many replicas distributed on each tserver, though
we can use CLI tool like 'kudu cluster rebalance' to check per-table
balance status of a cluster, but we have to fetch balance info table
by table (by add --tables=xx flag), and also we have to "run command"
in thirdparty monitor system and this tool seems not fast enough
because it will call functions like RetrieveAllTablets.
Now we can use this new feature to check whether a table is balanced
or not by thirdparty monitor system efficiently. For example, when
we fetch metrics from tserver by:
http://<host>:<port>/metrics?merge_rules=tablet|table|table_name
we can get merged metrics of table entity from each tserver as
mentioned in commit fe6e5cc0c9c1573de174d1ce7838b449373ae36e, and now
we can get an extra size type gauge metric
"merged_entities_count_of_tablet" that indicate how many replicas this
tserver hold of a table, then compare this metric of each tserver and
judge whether this table is balanced or not.
Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
---
M src/kudu/master/catalog_manager.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/tablet.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
6 files changed, 52 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/14128/11
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 11
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 14:
Thanks Adar and Alexey!
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 14
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 11 Sep 2019 01:57:57 +0000
Gerrit-HasComments: No
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 12: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14128/12/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/12/src/kudu/util/metrics.h@255
PS12, Line 255:
: // IWYU pragma: no_forward_declare kudu::MetricPrototype
: // IWYU pragma: no_forward_declare kudu::MetricPrototype::CtorArgs
: // IWYU pragma: no_forward_declare kudu::MetricUnit
Should we remove this?
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 12
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 19:01:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 10:
> Looks good but IWYU is still complaining. What happens if you add
> its recommendations?
I went that route: compilation fails if doing so. I think those forward declarations confuse IWYU. The forward declarations are necessary if trying to keep InvalidateEpoch() as a non-public method: I recommended doing so.
Last time I manually overrode -1 from Jenkins due to IWYU failure. I can do it this time as well. An alternative route is to make InvalidateEpoch() public, as it was before.
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 02:04:21 +0000
Gerrit-HasComments: No
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14128/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14128/8//COMMIT_MSG@10
PS8, Line 10: it
> nit: it's
Done
http://gerrit.cloudera.org:8080/#/c/14128/8//COMMIT_MSG@11
PS8, Line 11: add
> nit: adds
Done
http://gerrit.cloudera.org:8080/#/c/14128/8/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/8/src/kudu/util/metrics.h@785
PS8, Line 785: template<typename T>
: friend class GaugePrototype;
: FRIEND_TEST(MetricsTest, TestDumpOnlyChanged);
> It seems IWYU incorrectly processes this. I think we can ignore that. Or,
The IWYU message seems caused by L262~L266, but I'm not sure how to fix it.
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 9
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Fri, 06 Sep 2019 07:13:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 10:
> Last time I manually overrode -1 from Jenkins due to IWYU failure. I can do it this time as well.
That just passes the pain along: the next person to touch metrics.h will see the exact same failure.
> An alternative route is to make InvalidateEpoch() public, as it was before.
Let's do this instead.
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 10
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Tue, 10 Sep 2019 02:41:14 +0000
Gerrit-HasComments: No
[kudu-CR] [metrics] Add a metric to count sub-entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has abandoned this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count sub-entities when merge metrics
......................................................................
Abandoned
need fix memory problem
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 14:
> Thanks Adar and Alexey!
Thank you for your contribution!
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 14
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 11 Sep 2019 19:05:58 +0000
Gerrit-HasComments: No
[kudu-CR] [metrics] Add a metric to count merged entities when merge metrics
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14128 )
Change subject: [metrics] Add a metric to count merged entities when merge metrics
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14128/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14128/3//COMMIT_MSG@13
PS3, Line 13: check whether a table is balanced or not by thirdparty monitor system.
> Thanks, that's useful to know. Maybe you can find a way to incorporate this
Done
http://gerrit.cloudera.org:8080/#/c/14128/4/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/14128/4/src/kudu/util/metrics.h@737
PS4, Line 737: // Invalidate 'm_epoch_', then this metric is invisible until its value changed.
> Nit: "Invalidate 'm_epoch_', causing this metric to be invisible until its
Done
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:
http://gerrit.cloudera.org:8080/#/c/14128/2/src/kudu/util/metrics.cc@479
PS2, Line 479: // metrics, we should keep them around until the next poll.
> Hmm, I don't see this change done: I still see just the one static metric p
Done
--
To view, visit http://gerrit.cloudera.org:8080/14128
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I652b457a2df73414f95f5d1d5efaa003cc262bd1
Gerrit-Change-Number: 14128
Gerrit-PatchSet: 4
Gerrit-Owner: Yingchun Lai <40...@qq.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <40...@qq.com>
Gerrit-Comment-Date: Wed, 04 Sep 2019 05:33:19 +0000
Gerrit-HasComments: Yes