You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/02/13 06:13:03 UTC
[kudu-CR] [clock] update on Clock interface
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15215
Change subject: [clock] update on Clock interface
......................................................................
[clock] update on Clock interface
This patch re-factors Clock-related classes:
* removed Clock::RegisterMetrics() method
* HybridClock constructor requires metric entity
* LogicalClock constructor accepts metric entity as optional
second parameter
* LogicalClock constructor is now public
* LogicalClock::CreateStartingAt() is gone
I also did other minor re-factoring, partially due to warnings reported
by ClangTidy on the code I touched.
The motivation for this change is to prepare for follow-up changelists
addressing KUDU-3048 (adding clock metrics for better observability).
Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877
---
M src/kudu/clock/clock.h
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/hybrid_clock.h
M src/kudu/clock/logical_clock-test.cc
M src/kudu/clock/logical_clock.cc
M src/kudu/clock/logical_clock.h
M src/kudu/consensus/consensus_peers-test.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/log-test-base.h
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/consensus/time_manager-test.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tools/tool_action_perf.cc
25 files changed, 335 insertions(+), 314 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/15215/1
--
To view, visit http://gerrit.cloudera.org:8080/15215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877
Gerrit-Change-Number: 15215
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [clock] update on Clock interface
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15215 )
Change subject: [clock] update on Clock interface
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15215/1/src/kudu/clock/logical_clock.h
File src/kudu/clock/logical_clock.h:
http://gerrit.cloudera.org:8080/#/c/15215/1/src/kudu/clock/logical_clock.h@51
PS1, Line 51: const scoped_refptr<MetricEntity>& metric_entity = {});
> Because with the newly introduced metrics HybridClock updates them very oft
OK, I guess I don't feel strongly enough to warrant a ton of LogicalClock-specific work.
--
To view, visit http://gerrit.cloudera.org:8080/15215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877
Gerrit-Change-Number: 15215
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Feb 2020 05:34:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] [clock] update on Clock interface
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15215 )
Change subject: [clock] update on Clock interface
......................................................................
Patch Set 1:
> > You added several new metrics. What is their purpose ? Should be
> be
> > documented ?
>
> This patch doesn't add any new metrics.
The patch with a couple of new metrics is: https://gerrit.cloudera.org/#/c/15212/
I'm going to re-base it on top of https://gerrit.cloudera.org/#/c/15216/ , but feel free to leave feedback there as well.
In short, the new metrics are documented by their instantiation macro (i.e. METRIC_DEFINE_gauge_<type>(), METRIC_DEFINE_histogram(), etc.), but in addition to that there is extra information on those in description of KUDU-3048. I'm going to add more color to KUDU-3048 in that regard as well (like documenting how the new metrics can be used).
--
To view, visit http://gerrit.cloudera.org:8080/15215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877
Gerrit-Change-Number: 15215
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Feb 2020 16:49:55 +0000
Gerrit-HasComments: No
[kudu-CR] [clock] update on Clock interface
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Volodymyr Verovkin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15215 )
Change subject: [clock] update on Clock interface
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/15215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877
Gerrit-Change-Number: 15215
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Feb 2020 19:22:44 +0000
Gerrit-HasComments: No
[kudu-CR] [clock] update on Clock interface
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15215 )
Change subject: [clock] update on Clock interface
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15215/1/src/kudu/clock/logical_clock.h
File src/kudu/clock/logical_clock.h:
http://gerrit.cloudera.org:8080/#/c/15215/1/src/kudu/clock/logical_clock.h@51
PS1, Line 51: const scoped_refptr<MetricEntity>& metric_entity = {});
> OK, I guess I don't feel strongly enough to warrant a ton of LogicalClock-s
Thank you for the feedback and fast review.
If needed, we can address the unification of the constructors for LogicalClock() and HybridClock() in a separate changelist.
--
To view, visit http://gerrit.cloudera.org:8080/15215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877
Gerrit-Change-Number: 15215
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Feb 2020 17:45:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [clock] update on Clock interface
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15215 )
Change subject: [clock] update on Clock interface
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15215/1/src/kudu/clock/logical_clock.h
File src/kudu/clock/logical_clock.h:
http://gerrit.cloudera.org:8080/#/c/15215/1/src/kudu/clock/logical_clock.h@51
PS1, Line 51: const scoped_refptr<MetricEntity>& metric_entity = {});
> Why is it optional in the LogicalClock but required in the HybridClock? Cer
Because with the newly introduced metrics HybridClock updates them very often, and doing additional 'if (metric)' seems prohibitive. LogicalClock doesn't update any metrics: it only reports on some if requested via the metrics registry.
Yes, there are many more tests which don't set up an entity since they don't need to extract metrics from LogicalClock.
I'm not sure it's worth updating those pursuing the noble goal of semantics unification, but let me know if you feel strongly otherwise.
--
To view, visit http://gerrit.cloudera.org:8080/15215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877
Gerrit-Change-Number: 15215
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Feb 2020 04:16:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [clock] update on Clock interface
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15215 )
Change subject: [clock] update on Clock interface
......................................................................
Patch Set 1:
> You added several new metrics. What is their purpose ? Should be be
> documented ?
This patch doesn't add any new metrics.
--
To view, visit http://gerrit.cloudera.org:8080/15215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877
Gerrit-Change-Number: 15215
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Feb 2020 16:38:58 +0000
Gerrit-HasComments: No
[kudu-CR] [clock] update on Clock interface
Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Volodymyr Verovkin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15215 )
Change subject: [clock] update on Clock interface
......................................................................
Patch Set 1:
You added several new metrics. What is their purpose ? Should be be documented ?
--
To view, visit http://gerrit.cloudera.org:8080/15215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877
Gerrit-Change-Number: 15215
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Feb 2020 08:32:33 +0000
Gerrit-HasComments: No
[kudu-CR] [clock] update on Clock interface
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15215 )
Change subject: [clock] update on Clock interface
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15215/1/src/kudu/clock/logical_clock.h
File src/kudu/clock/logical_clock.h:
http://gerrit.cloudera.org:8080/#/c/15215/1/src/kudu/clock/logical_clock.h@51
PS1, Line 51: const scoped_refptr<MetricEntity>& metric_entity = {});
Why is it optional in the LogicalClock but required in the HybridClock? Certainly in production we'll always have the server entity; are there many more LogicalClock tests that don't set up an entity?
FWIW, I think it'd be clearer if both clock types behaved the same way: either both required an entity, or it was optional for both. As is, it'll be hard to remember that there are different semantics for different clock types.
--
To view, visit http://gerrit.cloudera.org:8080/15215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877
Gerrit-Change-Number: 15215
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Feb 2020 01:49:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [clock] update on Clock interface
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15215 )
Change subject: [clock] update on Clock interface
......................................................................
[clock] update on Clock interface
This patch re-factors Clock-related classes:
* removed Clock::RegisterMetrics() method
* HybridClock constructor requires metric entity
* LogicalClock constructor accepts metric entity as optional
second parameter
* LogicalClock constructor is now public
* LogicalClock::CreateStartingAt() is gone
I also did other minor re-factoring, partially due to warnings reported
by ClangTidy on the code I touched.
The motivation for this change is to prepare for follow-up changelists
addressing KUDU-3048 (adding clock metrics for better observability).
Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877
Reviewed-on: http://gerrit.cloudera.org:8080/15215
Tested-by: Kudu Jenkins
Reviewed-by: Volodymyr Verovkin <ve...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/clock/clock.h
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/hybrid_clock.h
M src/kudu/clock/logical_clock-test.cc
M src/kudu/clock/logical_clock.cc
M src/kudu/clock/logical_clock.h
M src/kudu/consensus/consensus_peers-test.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/log-test-base.h
M src/kudu/consensus/log_cache-test.cc
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/consensus/time_manager-test.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tools/tool_action_perf.cc
25 files changed, 335 insertions(+), 314 deletions(-)
Approvals:
Kudu Jenkins: Verified
Volodymyr Verovkin: Looks good to me, but someone else must approve
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15215
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877
Gerrit-Change-Number: 15215
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>