Todd Lipcon has posted comments on this change. Change subject: Cleanup/refactor tracking of consensus watermarks ......................................................................
Patch Set 4: (23 comments) http://gerrit.cloudera.org:8080/#/c/4133/4/src/kudu/consensus/consensus.proto File src/kudu/consensus/consensus.proto: Line 333: // The id of the last committed operation in the configuration. This is the > s/id/index/ Done Line 334: // id of the last operation the leader deemed committed from a consensus > s/id/index/ Done http://gerrit.cloudera.org:8080/#/c/4133/4/src/kudu/consensus/consensus_peers-test.cc File src/kudu/consensus/consensus_peers-test.cc: Line 142: message_queue_->SetLeaderMode(0, 0, BuildRaftConfigPBForTests(3)); > Maybe better to use kMinimumTerm and kMinimumOpIdIndex from opid_util.h her Done http://gerrit.cloudera.org:8080/#/c/4133/4/src/kudu/consensus/consensus_queue-test.cc File src/kudu/consensus/consensus_queue-test.cc: Line 202: queue_->SetLeaderMode(0, MinimumOpId().term(), BuildRaftConfigPBForTests(2)); > 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(), BuildRaftConfigPBForTests(2)); > same Done Line 305: queue_->SetLeaderMode(0, MinimumOpId().term(), BuildRaftConfigPBForTests(3)); > same Done 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?) http://gerrit.cloudera.org:8080/#/c/4133/4/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: 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; CHECK_OK(queue->AppendOperation(make_scoped_refptr_replicate( CreateDummyReplicate(term, index, clock->Now(), payload_size).release()))); } } 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" Done Line 669: VLOG_WITH_PREFIX_UNLOCKED(2) << "Commit index advanced to " << *updated_commit_index; > add the "before" ci Done Line 676: (peer->last_known_committed_idx < queue_state_.committed_index); > nit: rename the suffix of this var to _index for consistency Done http://gerrit.cloudera.org:8080/#/c/4133/4/src/kudu/consensus/consensus_queue.h 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 Done http://gerrit.cloudera.org:8080/#/c/4133/4/src/kudu/consensus/raft_consensus-test.cc File src/kudu/consensus/raft_consensus-test.cc: 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 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? > 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 http://gerrit.cloudera.org:8080/#/c/4133/4/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: Line 171: > nit: extra line Done http://gerrit.cloudera.org:8080/#/c/4133/4/src/kudu/consensus/raft_consensus_state.cc File src/kudu/consensus/raft_consensus_state.cc: 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=" << committed_op > more info in the log statement or remove Done http://gerrit.cloudera.org:8080/#/c/4133/4/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: Line 2543: req.set_committed_index(0); > use constant same response as elsewhere -- 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
