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 <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
