You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2017/08/19 02:16:50 UTC
[kudu-CR] rpc: periodic timers
Hello David Ribeiro Alves, Mike Percy, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7733
to review the following change.
Change subject: rpc: periodic timers
......................................................................
rpc: periodic timers
This patch introduces a generic periodic timer class. How does it work?
1. A timer is constructed with a user-provided task functor.
2. After Start() is called, the timer will run the functor repeatedly on a
fixed (and optionally, jittered) period.
3. When Reset() is called, it'll reset the period.
4. After Stop() is called, it won't run the functor anymore.
The implementation is based on Messenger::ScheduleOnReactor but it could
just as easily build on libev directly. I chose to use the Messenger so that
I wouldn't have to implement timer thread pooling. It's also just a generic
version of the Raft heartbeat logic found in Peer (see commit 1070e76).
In follow-on patches I will use this class to replace the bespoke Peer logic
as well as for Raft failure detection. If you're curious, it's actually
because of failure detection that both Start() and Reset() optionally accept
deltas to override the default period; the snoozing code likes to provide
heavily customized backoff.
Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/periodic-test.cc
A src/kudu/rpc/periodic.cc
A src/kudu/rpc/periodic.h
4 files changed, 459 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/7733/1
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] rpc: periodic timers
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic-test.cc
File src/kudu/rpc/periodic-test.cc:
Line 88: TEST_P(PeriodicTimerTest, TestReset) {
> When I looped 1000 times with TSAN all I got were data races on access to c
yea, of course it will be timing-dependent. But perhaps just running the whole thing slower (eg with period_ms_ being 500ms instead of 50) would mean that the same absolute time slownesses would be much less likely to cause a failure.
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic.h
File src/kudu/rpc/periodic.h:
PS2, Line 59: periodic_period_jitter_pct
> I think the interface would be cleaner as
ok, I'm alright with a gflag on the argument that they're easier to change than constants, so long as it's experimental
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: periodic timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7733/1/src/kudu/rpc/periodic-test.cc
File src/kudu/rpc/periodic-test.cc:
Line 70: SleepFor(MonoDelta::FromMilliseconds(period_ms_ * 2));
> warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o
Done
Line 76: SleepFor(MonoDelta::FromMilliseconds(period_ms_ * 2));
> warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o
Done
Line 96: if (now - start_time > MonoDelta::FromMilliseconds(period_ms_ * 5)) {
> warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o
Done
Line 107: timer_->Reset(MonoDelta::FromMilliseconds(period_ms_ * 5));
> warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o
Done
Line 120: timer_->Start(MonoDelta::FromMilliseconds(period_ms_ * 5));
> warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o
Done
http://gerrit.cloudera.org:8080/#/c/7733/1/src/kudu/rpc/periodic.cc
File src/kudu/rpc/periodic.cc:
Line 76: Reset(next_task_delta);
> warning: parameter 'next_task_delta' is passed by value and only copied onc
Done
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: periodic timers
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] rpc: periodic timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7733
to look at the new patch set (#4).
Change subject: rpc: periodic timers
......................................................................
rpc: periodic timers
This patch introduces a generic periodic timer class. How does it work?
1. A timer is constructed with a user-provided task functor.
2. After Start() is called, the timer will run the functor repeatedly on a
fixed (and optionally, jittered) period.
3. When Reset() is called, it'll reset the period.
4. After Stop() is called, it won't run the functor anymore.
The implementation is based on Messenger::ScheduleOnReactor but it could
just as easily build on libev directly. I chose to use the Messenger so that
I wouldn't have to implement timer thread pooling. It's also just a generic
version of the Raft heartbeat logic found in Peer (see commit 1070e76).
In follow-on patches I will use this class to replace the bespoke Peer logic
as well as for Raft failure detection. If you're curious, it's actually
because of failure detection that both Start() and Reset() optionally accept
deltas to override the default period; the snoozing code likes to provide
heavily customized backoff.
Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/periodic-test.cc
A src/kudu/rpc/periodic.cc
A src/kudu/rpc/periodic.h
4 files changed, 465 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/7733/4
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] rpc: periodic timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7733
to look at the new patch set (#2).
Change subject: rpc: periodic timers
......................................................................
rpc: periodic timers
This patch introduces a generic periodic timer class. How does it work?
1. A timer is constructed with a user-provided task functor.
2. After Start() is called, the timer will run the functor repeatedly on a
fixed (and optionally, jittered) period.
3. When Reset() is called, it'll reset the period.
4. After Stop() is called, it won't run the functor anymore.
The implementation is based on Messenger::ScheduleOnReactor but it could
just as easily build on libev directly. I chose to use the Messenger so that
I wouldn't have to implement timer thread pooling. It's also just a generic
version of the Raft heartbeat logic found in Peer (see commit 1070e76).
In follow-on patches I will use this class to replace the bespoke Peer logic
as well as for Raft failure detection. If you're curious, it's actually
because of failure detection that both Start() and Reset() optionally accept
deltas to override the default period; the snoozing code likes to provide
heavily customized backoff.
Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/periodic-test.cc
A src/kudu/rpc/periodic.cc
A src/kudu/rpc/periodic.h
4 files changed, 459 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/7733/2
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] rpc: periodic timers
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
LGTM modulo the typo
http://gerrit.cloudera.org:8080/#/c/7733/4/src/kudu/rpc/periodic.h
File src/kudu/rpc/periodic.h:
PS4, Line 68: -
+
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: periodic timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 5: Verified+1
Unrelated test failure, I filed KUDU-2109 for the issue.
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] rpc: periodic timers
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 5:
unrelated flake. retriggered
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] rpc: periodic timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7733
to look at the new patch set (#5).
Change subject: rpc: periodic timers
......................................................................
rpc: periodic timers
This patch introduces a generic periodic timer class. How does it work?
1. A timer is constructed with a user-provided task functor.
2. After Start() is called, the timer will run the functor repeatedly on a
fixed (and optionally, jittered) period.
3. When Reset() is called, it'll reset the period.
4. After Stop() is called, it won't run the functor anymore.
The implementation is based on Messenger::ScheduleOnReactor but it could
just as easily build on libev directly. I chose to use the Messenger so that
I wouldn't have to implement timer thread pooling. It's also just a generic
version of the Raft heartbeat logic found in Peer (see commit 1070e76).
In follow-on patches I will use this class to replace the bespoke Peer logic
as well as for Raft failure detection. If you're curious, it's actually
because of failure detection that both Start() and Reset() optionally accept
deltas to override the default period; the snoozing code likes to provide
heavily customized backoff.
Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/periodic-test.cc
A src/kudu/rpc/periodic.cc
A src/kudu/rpc/periodic.h
4 files changed, 465 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/7733/5
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] rpc: periodic timers
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic.cc
File src/kudu/rpc/periodic.cc:
Line 38: DEFINE_double(periodic_period_jitter_pct, 0.25,
I don't think exposing this is a good idea. It's not clear what it's actually controlling. If we wanted to make this configurable, I'd expect it to be done on a per-use basis (e.g. heartbeat_period_jitter).
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: periodic timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic-test.cc
File src/kudu/rpc/periodic-test.cc:
Line 88: TEST_P(PeriodicTimerTest, TestReset) {
> yea, of course it will be timing-dependent. But perhaps just running the wh
OK, I've changed the period from 20ms to 200ms to stave off potential TSAN flakiness. I looped the latest version 1000 times and got no failures.
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic.cc
File src/kudu/rpc/periodic.cc:
Line 38: DEFINE_double(periodic_period_jitter_pct, 0.25,
> See my reply to the other comment about what I would do to make this go awa
I did end up renaming the flag (to remove 'period'), but your general point still stands, and in the latest version of this patch the flag is gone.
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic.h
File src/kudu/rpc/periodic.h:
PS2, Line 59: periodic_period_jitter_pct
> ok, I'm alright with a gflag on the argument that they're easier to change
Dan's approach is compelling. I've rewritten the code in that way.
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: periodic timers
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] rpc: periodic timers
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic-test.cc
File src/kudu/rpc/periodic-test.cc:
Line 77: ASSERT_GT(counter_, 0);
is this going to be flaky? maybe an AssertEventually is more robust?
Line 88: TEST_P(PeriodicTimerTest, TestReset) {
have you looped this with tsan? I'm worried it will be flaky if this main thread gets descheduled for more than 20ms. maybe need a much larger period to avoid flakiness?
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic.cc
File src/kudu/rpc/periodic.cc:
PS2, Line 54: messenger, functor
I'm surprised clang-tidy dind't complain about these, I'd have expected passing them through with std::move
PS2, Line 133: // We must schedule the next callback with the minimal period as the delay
: // so that if Reset() is called with a low value, the scheduled callback
: // can still honor it.
:
I don't quite follow this. If we called Reset() with a small delay (less than minimum period) wouldn't we just schedule a new callback precisely for the requested time?
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic.h
File src/kudu/rpc/periodic.h:
PS2, Line 40: Messenger::ScheduleOnReactor
I think it's worth a note here that, since the tasks are run on reactor threads, they are not allowed to block or otherwise do lengthy computation, etc.
PS2, Line 59: periodic_period_jitter_pct
hrm, I don't know about making this a general flag vs passing it in to the PeriodicTimer instance. I think we've been trying to avoid gflags being read too much from utility code that may affect multiple subsystems.
That said, if it's an experimental/unstable flag it's no big deal since we can always adjust later.
PS2, Line 83: period for the next
: // task.
not 100% clear here. Is this setting the period (overriding what's in the 'period' set in the ctor) or is this more of an 'initial delay' after which it will revert to rescheduling based on the configured period/jitter? Also, will jitter be applied to the provided value or will this be an exact delay?
Maybe something like "delay for scheduling the next task" or "delay after the task will first be invoked, after which it will revert to the scheduling paramters provided in the constructor" or something
Line 92: // If 'next_task_delta' is provided, it is used as the new period. Otherwise,
Similar to the above, not sure whether this means to reset the period for all subsequent schedulings, or just the subsequent call.
If the latter, maybe 'Snooze' is a better name than 'Reset' since reset sounds more like a permanent change?
Line 93: // the period is generated in accordance with the timer's period and jitter
in the case that it is boost::none, this still will "snooze" one full period worth, right?
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: periodic timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 2:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic-test.cc
File src/kudu/rpc/periodic-test.cc:
PS2, Line 51: ()
> The parens are optional when empty (just like the return value). Don't fee
I don't mind changing it.
Line 77: ASSERT_GT(counter_, 0);
> is this going to be flaky? maybe an AssertEventually is more robust?
Sure, will add an AssertEventually.
Line 88: TEST_P(PeriodicTimerTest, TestReset) {
> have you looped this with tsan? I'm worried it will be flaky if this main t
When I looped 1000 times with TSAN all I got were data races on access to counter_ after the test fixture had been destroyed. Explicit Messenger::Shutdown() in TearDown() ought to clear that up.
In general I have no idea how to write unit tests for timers that aren't timing-dependent in some way. I'd love more feedback on this.
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic.cc
File src/kudu/rpc/periodic.cc:
Line 38: DEFINE_double(periodic_period_jitter_pct, 0.25,
> I don't think exposing this is a good idea. It's not clear what it's actua
Even as an experimental flag? I couldn't see any reason to use different values for heartbeat and failure detection jitter, so I started off with a constant, then reasoned an experimental gflag is no worse than a constant (and may be better).
PS2, Line 54: messenger, functor
> I'm surprised clang-tidy dind't complain about these, I'd have expected pas
That is odd, but I will std::move() them.
PS2, Line 133: // We must schedule the next callback with the minimal period as the delay
: // so that if Reset() is called with a low value, the scheduled callback
: // can still honor it.
:
> I don't quite follow this. If we called Reset() with a small delay (less th
As we discussed on Slack, if we allowed Reset() to schedule a new callback (while an existing one was in flight), we could wind up with multiple never-ending callback "loops". To prevent that, we force each timer to stick to just one callback loop, with the downside that Reset()'s delay may not be honored if it drops below GetMinimumPeriod().
I'll update the comment and/or find another way to express this.
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic.h
File src/kudu/rpc/periodic.h:
PS2, Line 40: Messenger::ScheduleOnReactor
> I think it's worth a note here that, since the tasks are run on reactor thr
Yes, I meant to add that but forgot; thank you for the reminder.
PS2, Line 59: periodic_period_jitter_pct
> hrm, I don't know about making this a general flag vs passing it in to the
As I see it, we have three options:
1. Force callers to provide it.
2. Define a constant in the code.
3. Define a gflag.
I think #1 isn't interesting: certainly for the two callers I have so far there's no reason NOT to use the same percentage. So originally I did #2, then decided that #3, as long as the flag was tagged as experimental (which it is), is no worse.
PS2, Line 83: period for the next
: // task.
> not 100% clear here. Is this setting the period (overriding what's in the '
1. It's an 'initial delay'; it doesn't change the future period.
2. It'll be an exact delay, no jitter.
I'll reword to make this more clear.
Line 92: // If 'next_task_delta' is provided, it is used as the new period. Otherwise,
> Similar to the above, not sure whether this means to reset the period for a
It affects just the next call.
Will rename to Snooze.
Line 93: // the period is generated in accordance with the timer's period and jitter
> in the case that it is boost::none, this still will "snooze" one full perio
Yes, but it's not additive: if I call Snooze() at time X and then again at time X+0.1P, the task will run at task X+1.1P, not X+2P.
Line 97: void Reset(boost::optional<MonoDelta> next_task_delta = boost::none);
> May want to add a note that next_task_delta must be > GetMinimumPeriod(), o
Will do.
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: periodic timers
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic-test.cc
File src/kudu/rpc/periodic-test.cc:
PS2, Line 51: ()
The parens are optional when empty (just like the return value). Don't feel the need to change it, but I found that out recently and I think it tends to look a bit cleaner.
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic.h
File src/kudu/rpc/periodic.h:
Line 97: void Reset(boost::optional<MonoDelta> next_task_delta = boost::none);
May want to add a note that next_task_delta must be > GetMinimumPeriod(), otherwise it's not guaranteed to fire in a timely manner.
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: periodic timers
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic.cc
File src/kudu/rpc/periodic.cc:
Line 38: DEFINE_double(periodic_period_jitter_pct, 0.25,
> Even as an experimental flag? I couldn't see any reason to use different va
See my reply to the other comment about what I would do to make this go away. The why is that the name of the flag is bad, and it unnecessarily ties together two otherwise unrelated periods (and however many more get consolidated into this abstraction).
http://gerrit.cloudera.org:8080/#/c/7733/2/src/kudu/rpc/periodic.h
File src/kudu/rpc/periodic.h:
PS2, Line 59: periodic_period_jitter_pct
> As I see it, we have three options:
I think the interface would be cleaner as
// Creates a new PeriodicTimer.
//
// A ref is taken on 'messenger', which is used for scheduling callbacks.
//
// 'functor' defines the user's task and is owned for the lifetime of the
// PeriodicTimer.
//
// 'jitter_pct' controls the amount of jitter to introduce ... explain how it's [period - period * (jitter_pct / 100), period + period * (jitter_pct / 100)) ...
static std::shared_ptr<PeriodicTimer> Create(
std::shared_ptr<Messenger> messenger,
RunTaskFunctor functor,
MonoDelta period,
int jitter_pct = 25);
There are already flags in the failure detector that are meant to be the equivalent of setting the jitter here, but they are kind of vestigial at this point.
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: periodic timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7733
to look at the new patch set (#3).
Change subject: rpc: periodic timers
......................................................................
rpc: periodic timers
This patch introduces a generic periodic timer class. How does it work?
1. A timer is constructed with a user-provided task functor.
2. After Start() is called, the timer will run the functor repeatedly on a
fixed (and optionally, jittered) period.
3. When Reset() is called, it'll reset the period.
4. After Stop() is called, it won't run the functor anymore.
The implementation is based on Messenger::ScheduleOnReactor but it could
just as easily build on libev directly. I chose to use the Messenger so that
I wouldn't have to implement timer thread pooling. It's also just a generic
version of the Raft heartbeat logic found in Peer (see commit 1070e76).
In follow-on patches I will use this class to replace the bespoke Peer logic
as well as for Raft failure detection. If you're curious, it's actually
because of failure detection that both Start() and Reset() optionally accept
deltas to override the default period; the snoozing code likes to provide
heavily customized backoff.
Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/periodic-test.cc
A src/kudu/rpc/periodic.cc
A src/kudu/rpc/periodic.h
4 files changed, 491 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/7733/3
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] rpc: periodic timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: rpc: periodic timers
......................................................................
rpc: periodic timers
This patch introduces a generic periodic timer class. How does it work?
1. A timer is constructed with a user-provided task functor.
2. After Start() is called, the timer will run the functor repeatedly on a
fixed (and optionally, jittered) period.
3. When Reset() is called, it'll reset the period.
4. After Stop() is called, it won't run the functor anymore.
The implementation is based on Messenger::ScheduleOnReactor but it could
just as easily build on libev directly. I chose to use the Messenger so that
I wouldn't have to implement timer thread pooling. It's also just a generic
version of the Raft heartbeat logic found in Peer (see commit 1070e76).
In follow-on patches I will use this class to replace the bespoke Peer logic
as well as for Raft failure detection. If you're curious, it's actually
because of failure detection that both Start() and Reset() optionally accept
deltas to override the default period; the snoozing code likes to provide
heavily customized backoff.
Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Reviewed-on: http://gerrit.cloudera.org:8080/7733
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/periodic-test.cc
A src/kudu/rpc/periodic.cc
A src/kudu/rpc/periodic.h
4 files changed, 465 insertions(+), 0 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Adar Dembo: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] rpc: periodic timers
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: rpc: periodic timers
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I23f80631a5591b1ac023974f3d7d9eb1576cdb86
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No