Mike Percy has posted comments on this change. Change subject: consensus: Get rid of RaftConsensus::Create() ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/7192/2//COMMIT_MSG Commit Message: PS2, Line 12: simplies > typo Done http://gerrit.cloudera.org:8080/#/c/7192/2/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS2, Line 226: failure_monitor_.reset(new RandomizedFailureMonitor(GetRandomSeed32(), : GetFailureMonitorCheckMeanMs(), : GetFailureMonitorCheckStddevMs())); : failure_detector_.reset(new TimedFailureDetector(MonoDelta::FromMilliseconds( : FLAGS_raft_heartbeat_interval_ms * : FLAGS_leader_failure_max_missed_heartbeat_periods))); > these can be moved to the ctor, right? We don't want these running for tombstoned tablets, so no. PS2, Line 238: LockGuard l(lock_); : current_term = GetCurrentTermUnlocked(); > why is the lock needed here? just because we have an assert? Yeah, but there's not much harm. It's not worth changing because Start() will have to be thread safe once we have tombstoned voting. http://gerrit.cloudera.org:8080/#/c/7192/2/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: PS2, Line 788: > this is no longer needed? maybe spell it out in the commit message? It was never used. I'll add a note. PS2, Line 125: Status Init(); > should probably say something about threading in Init() and Start() as they Done -- To view, visit http://gerrit.cloudera.org:8080/7192 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic28fb8fe64cd62d290cea1de22c4ba9dd1743a4e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
