David Ribeiro Alves has posted comments on this change. Change subject: Cleanup/refactor tracking of consensus watermarks ......................................................................
Patch Set 4: (17 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 202: queue_->SetLeaderMode(0, MinimumOpId().term(), BuildRaftConfigPBForTests(2)); use MinimumOpId.index() of the equivalent constant Line 242: queue_->SetLeaderMode(0, MinimumOpId().term(), BuildRaftConfigPBForTests(2)); same Line 305: queue_->SetLeaderMode(0, MinimumOpId().term(), BuildRaftConfigPBForTests(3)); same Line 311: ASSERT_EQ(queue_->GetMajorityReplicatedIndexForTests(), 0); same here and below 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? Line 499: boost::optional<int64_t> updated_commit_index; do we really need to introduce a boost dependency here? since this is just an int why not just have a NOT_UPDATED = -1 constant? 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 commit index here is safe, independently of whether we're leader or not (we might not be by the time we reach here) since the commit index is a property of the shared state machine and not something that is "decided" by the leader. 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 < queue_state_.committed_index); nit: rename the suffix of this var to _index for consistency 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 commit index" 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) << "============================================================"; ?? 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? 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 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 Line 583: LOG_WITH_PREFIX_UNLOCKED(WARNING) << "given start C_O=" << committed_op more info in the log statement or remove 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 -- 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
