[kudu-CR] rpc: periodic timers

[Adar Dembo (Code Review)]

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 
Tested-by: Adar Dembo 
---
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] rpc: periodic timers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] rpc: periodic timers

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] rpc: periodic timers

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] rpc: periodic timers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] rpc: periodic timers

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] rpc: periodic timers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] rpc: periodic timers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] rpc: periodic timers

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] rpc: periodic timers

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] rpc: periodic timers

[Dan Burkert (Code Review)]

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 Create(
  std::shared_ptr 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] rpc: periodic timers

[Adar Dembo (Code Review)]

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 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] rpc: periodic timers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] rpc: periodic timers

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] rpc: periodic timers

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] rpc: periodic timers

[Dan Burkert (Code Review)]

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 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] rpc: periodic timers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] rpc: periodic timers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] rpc: periodic timers

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon