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