Dan Burkert has posted comments on this change. Change subject: consensus: use periodic timers for failure detection ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/7735/2/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: Line 252: std::bind([w]() { same comment about no-arg bind Line 319: rng_.Normal(GetFailureMonitorCheckMeanMs(), This might result in a negative value. It would probably be OK, but since it would very rarely occur in tests, it's perhaps best to bound it. Bigger picture, it seems like overkill to keep around the leader_failure_monitor_check_mean_ms and leader_failure_monitor_check_stddev_ms flags just for this very specific case. Perhaps remove them, and use the new failure detection timeperiod calculation (uniform distribution) instead? http://gerrit.cloudera.org:8080/#/c/7735/2/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: Line 27: #include <boost/none.hpp> This looks like it may be a bad interaction with Alexey's patch, but I don't think you should need both optional.hpp and none.hpp Line 523: void EnsureFailureDetectorEnabled( I think these methods would be better named 'EnableFailureDetector' and 'DisableFailureDetector' to match 'SnoozeFailureDetector'. Feel free to ignore if you feel that's outside the scope of this patch. -- To view, visit http://gerrit.cloudera.org:8080/7735 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8acdb44e12b975fda4a226aa784db95bc7b4e330 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
