Todd Lipcon has posted comments on this change.

Change subject: consensus: move more logic from ReplicaState to RaftConsensus
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4709/1/src/kudu/consensus/consensus_queue.h
File src/kudu/consensus/consensus_queue.h:

Line 65: // TODO Right now this class is able to track one outstanding operation
> warning: missing username/bug in TODO [google-readability-todo]
Done


PS1, Line 257: bool IsCommittedIndexInCurrentTerm() const;
> Is there an opportunity for the caller and the queue to disagree on what is
I don't think so because the current term is advanced only under the lock held 
by RaftConsensus, and this is called by RaftConsensus.


http://gerrit.cloudera.org:8080/#/c/4709/1/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

Line 618:     
DCHECK(round->replicate_msg()->change_config_record().has_old_config());
> are these DCHECKs really necessary? old/new config are required fields of C
I just moved this code. I think the idea is that since this might be a proto 
that we created on our side, it hasn't necessarily been verified to be fully 
initialized yet (so the fact that the fields are required doesn't mean that 
they're necessarily set). I'm guessing this was added because of some bug where 
we were missing these fields in some code path.


-- 
To view, visit http://gerrit.cloudera.org:8080/4709
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
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: 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