David Ribeiro Alves has posted comments on this change.

Change subject: Cleanup/refactor tracking of consensus watermarks
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4133/4/src/kudu/consensus/consensus_queue-test.cc
File src/kudu/consensus/consensus_queue-test.cc:

Line 311:   ASSERT_EQ(queue_->GetMajorityReplicatedIndexForTests(), 0);
> I disagree in this case, since it's obvious what the numeric constant is re
ok


http://gerrit.cloudera.org:8080/#/c/4133/4/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:

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 wa
sounds reasonable


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 th
would have into the history to remind myself why this is here, but I seem to 
recall this being an important thing


Line 499:   boost::optional<int64_t> updated_commit_index;
> given we already use boost::optional pretty often elsewhere, I dont think t
ok


Line 636:     // If we're the leader, we are in charge of computing the new
> hrm, why do you say we might not be the leader? We're still inside the lock
queue is decoupled from state so isn't it possible that we lost an election but 
the queue doesn't know about it yet (i.e. that the call to change the queue's 
mode is queued behind this thread competing for the lock?


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

Line 1267:   // TODO: interrogate queue rather than state?
> trying to reduce the amount of duplication of responsibility (ideally could
that makes sense in general, though I'm not totally sure what it looks like


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2aa294472f018013d88f36a9358e9ebd9d5ed8f8
Gerrit-PatchSet: 4
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: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to