Todd Lipcon has posted comments on this change.

Change subject: Cleanup/refactor tracking of consensus watermarks

Patch Set 4:

File src/kudu/consensus/consensus.proto:

Line 333:   // The id of the last committed operation in the configuration. 
This is the
> s/id/index/

Line 334:   // id of the last operation the leader deemed committed from a 
> s/id/index/
File src/kudu/consensus/

Line 142:   message_queue_->SetLeaderMode(0, 0, BuildRaftConfigPBForTests(3));
> Maybe better to use kMinimumTerm and kMinimumOpIdIndex from opid_util.h her
File src/kudu/consensus/

Line 202:   queue_->SetLeaderMode(0, MinimumOpId().term(), 
> use MinimumOpId.index() of the equivalent constant
switched to using kMinimumOpIdIndex and kMinimumTerm here and elsewhere per 
Mike's suggestion

Line 242:   queue_->SetLeaderMode(0, MinimumOpId().term(), 
> same

Line 305:   queue_->SetLeaderMode(0, MinimumOpId().term(), 
> same

Line 311:   ASSERT_EQ(queue_->GetMajorityReplicatedIndexForTests(), 0);
> same here and below
I disagree in this case, since it's obvious what the numeric constant is 
representing (given the other side of the equality assertion). In the 
"SetLeaderMode" case it's nice because you don't necessarily remember the order 
of the two integer arguments.

Also I think it's important to realize that it's 0 as an integer, rather than 
maybe thinking it's 1 (which is actually the first term and index that any peer 
claims, right?)
File src/kudu/consensus/

Line 112:   queue_state_.current_term = 0;
> use constants here and elsewhere?
actually per my reply elsewhere I think 0's clearer

Line 137:   // TODO: reset first_index_in_term? make it optional, reset here 
and set on the
hrm, having some trouble with this TODO actually. Trying to clean up the way 
the queue tracks current_term as leader, and I think some of our tests may be 
abusing the queue. In particular:

static inline void AppendReplicateMessagesToQueue(
    PeerMessageQueue* queue,
    const scoped_refptr<server::Clock>& clock,
    int first,
    int count,
    int payload_size = 0) {

  for (int i = first; i < first + count; i++) {
    int term = i / 7;
    int index = i;
        CreateDummyReplicate(term, index, clock->Now(), 

this code is appending ops from different terms to a leader-mode queue, which I 
don't think would ever happen in real life, right? Any time the term advances, 
the leader would step down and then have to call SetLeaderMode again for the 
next term?

Mike/David: If this seems right to you guys, I'll make the change so that the 
queue doesn't track current term by "snooping" on ops anymore, but instead 
takes a 'first_opid_in_term' parameter to SetLeaderMode, which will be the opid 
of the NO_OP.

Line 447:       // TODO: why is this only if successful? shouldn't we still use
added this TODO in this patch but I think it's a pre-existing issue. Any 
thoughts on this issue? otherwise I"m inclined to leave this as a TODO here

Line 499:   boost::optional<int64_t> updated_commit_index;
> do we really need to introduce a boost dependency here? since this is just 
given we already use boost::optional pretty often elsewhere, I dont think the 
"introduce a dependency" argument carries that much weight. I like optional 
because it's impossible to accidentally miss the "special" value (eg 
accidentally compare vs the -1 value and decide something is above this commit 
index, or whatever)

Line 636:     // If we're the leader, we are in charge of computing the new
> I think it would make sense to document somewhere that calculating the comm
hrm, why do you say we might not be the leader? We're still inside the lock 
here, so we know we're in leader mode.

Line 658:       if (queue_state_.majority_replicated_index >= 
queue_state_.first_index_in_term) {
> maybe rename this var to "first_index_in_current_term"

Line 669:         VLOG_WITH_PREFIX_UNLOCKED(2) << "Commit index advanced to " 
<< *updated_commit_index;
> add the "before" ci

Line 676:         (peer->last_known_committed_idx < 
> nit: rename the suffix of this var to _index for consistency
File src/kudu/consensus/consensus_queue.h:

Line 139:   // of the commit index from the follower code.
> having some trouble parsing this sentence: "updating the queue of the commi
yea this was some note to myself and I don't think it's relevant anymore.

Line 415:   // TODO: doc me
> todo
File src/kudu/consensus/

Line 388:   LOG(WARNING) << 
> ??
ah, was trying to separate out the gmock-related junk above from the 
gmock-related junk below while debugging something :) will remove
File src/kudu/consensus/

Line 1267:   // TODO: interrogate queue rather than state?
> what would be the difference/advantage?
trying to reduce the amount of duplication of responsibility (ideally could 
actually remove all knowledge of the commit index from 'state', so that 'state' 
is really just a subscriber to 'hey, start this operation', 'abort these 
operations', 'commit these operations' and not partially-responsible for this 
other stuff. In other words 'state' should represent the state machine, not the 
raft-internal state, and I think it'd be a cleaner design
File src/kudu/consensus/raft_consensus.h:

Line 171: 
> nit: extra line
File src/kudu/consensus/

Line 575:         LOG(FATAL) << "TODO: remove me once convinced that this 
doesnt happen in tests";
> add an actual error message that people can parse also add prefix
yea I was just using this during test looping, will just remove it now.

Line 583:     LOG_WITH_PREFIX_UNLOCKED(WARNING) << "given start C_O=" << 
> more info in the log statement or remove
File src/kudu/integration-tests/

Line 2543:   req.set_committed_index(0);
> use constant
same response as elsewhere

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I2aa294472f018013d88f36a9358e9ebd9d5ed8f8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <>
Gerrit-Reviewer: David Ribeiro Alves <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to