Todd Lipcon has posted comments on this change. Change subject: Make ConsensusMetadata thread-safe ......................................................................
Patch Set 9: (3 comments) just a few small items http://gerrit.cloudera.org:8080/#/c/6958/9/src/kudu/consensus/consensus_meta.cc File src/kudu/consensus/consensus_meta.cc: PS9, Line 135: lock_guard<Mutex> l(lock_); : switch (type) { : case ACTIVE_CONFIG: : return IsRaftConfigVoter(uuid, active_config_unlocked()); : case COMMITTED_CONFIG: : return IsRaftConfigVoter(uuid, committed_config_unlocked()); : case PENDING_CONFIG: : return IsRaftConfigVoter(uuid, pending_config_unlocked()); : default: : LOG(FATAL) << "Unsupported RaftConfigState: " << type; : } could this and the three functions below be simpler if you added something like: const RaftConfigPB& config_unlocked(RaftConfigState type) { switch (type) { case ACTIVE_CONFIG: return active_config_unlocked(); case COMMITTED_CONFIG: return committed_config_unlocked(); case PENDING_COPNFIG: return pending_config_unlocked(); } } bool IsVoterInConfig(const string& uuid, RaftConfigState type) { lock_guard<Mutex> l(lock_); return IsRaftConfigVoter(uuid, config_unlocked(type)); } ... http://gerrit.cloudera.org:8080/#/c/6958/9/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: Line 841: Status RaftConsensus::IsSingleVoterConfig(bool* single_voter) const { mind adding a TODO here that there's no reason for this to return a Status? should just be a normal getter returning bool Line 2345: int64_t committed_config_opid_index = cmeta_->GetConfigOpIdIndex(COMMITTED_CONFIG); isn't this equivalent to committed_config.opid_index() now? seems unnecessary -- To view, visit http://gerrit.cloudera.org:8080/6958 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
