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 <[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
