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

Reply via email to