Hello David Ribeiro Alves, Mike Percy,

I'd like you to do a code review.  Please visit


to review the following change.

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_
* 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

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
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, 121 insertions(+), 98 deletions(-)

  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/4709/1
To view, visit http://gerrit.cloudera.org:8080/4709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0f377ebb132f3f58f984605197831f41198d78c5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>

Reply via email to