Todd Lipcon has submitted this change and it was merged. Change subject: consensus: move more logic from ReplicaState to RaftConsensus ......................................................................
consensus: move more logic from ReplicaState to RaftConsensus ReplicaState currently has two main responsibilities: * manage the set of pending transactions, including committing and aborting them. The main state here is the pending_txns_ map. * act as a wrapper to ConsensusMeta (stored as cmeta_). These two items are relatively distinct. I drew a graph of the dependencies between methods and members of ReplicaState[1] and it became clear that most methods either pertained to one or the other of these responsibilities, but not both. So, this patch seeks to try to clean up the methods which were straddling the two responsibilities as follows: 1. AbortOpsAfterUnlocked, AddPendingOperation, AdvanceCommittedIndexUnlocked These three are mainly focused around managing pending transactions, but were also taking care of setting the pending and committed Raft configuration in the case of configuration change operations. The patch moves the handling of configuration changes into RaftConsensus itself by using the existing "replicated callback" to perform the necessary work when the operation finishes, and by checking for config changes before adding pending operations. 4. CheckHasCommittedOpInCurrentTermUnlocked This one is used to check whether the last committed operation is in the current term. Recently the PeerMessageQueue became responsible for tracking the committed OpId, and already has the necessary state to provide the same functionality. After the refactor, the class clearly shows two separate groups of methods and members[2]. This will make it easier to maintain/understand the code as well as open more opportunities for finer-grained locking by having smaller classes with better-defined responsibilities. I looped raft_consensus-itest 100 times with this patch and passed 100%[3]. NOTE: for the sake of the dependency graphs, I removed the state_ variable, ToString methods, and locking methods and left only the "important" bits. [1] http://i.imgur.com/fpyWwyM.png [2] http://i.imgur.com/7K9Dc5J.png [3] http://dist-test.cloudera.org/job?job_id=todd.1476324853.3445 Change-Id: I0f377ebb132f3f58f984605197831f41198d78c5 Reviewed-on: http://gerrit.cloudera.org:8080/4709 Tested-by: Kudu Jenkins Reviewed-by: David Ribeiro Alves <[email protected]> Reviewed-by: Mike Percy <[email protected]> --- M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/consensus_queue.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_state.cc M src/kudu/consensus/raft_consensus_state.h M src/kudu/integration-tests/raft_consensus-itest.cc 7 files changed, 129 insertions(+), 100 deletions(-) Approvals: David Ribeiro Alves: Looks good to me, approved Mike Percy: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4709 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I0f377ebb132f3f58f984605197831f41198d78c5 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[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]>
