Todd Lipcon has posted comments on this change. Change subject: consensus: Improve contract for API to fetch last-logged op id ......................................................................
Patch Set 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/7717/1/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: Line 143: log_cache_.Init(queue_state_.last_appended); mind adding a TODO to merge LogCache::Init into the LogCache ctor, given we now have all the necessary parameters at construction time? http://gerrit.cloudera.org:8080/#/c/7717/1/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: Line 245: queue.get(), in the case that one of the RETURN_NOT_OK calls below fails, then you'll end up with peer_manager_ constructed with a pointer to a destructed 'queue'. I think you may need to also defer storage of peer_manager_ until down below as well. Line 250: pending->SetInitialCommittedOpId(info.last_committed_id); I believe it was on purpose that this was called after the appending of the pending ops to the PendingRound structure below. PS1, Line 1492: opt_local_last_logged_opid) ? you can use .value_or(MinimumOpId()) Line 1493: : MinimumOpId(); is this right? I thought, if we don't know our own local op id, then it would be unsafe to vote ever, no? ie it should be MaximumOpId or something? Line 2226: if (!queue_ || !pending_) return boost::none; think it's worth a comment here explaining the cases where we could hit this case PS1, Line 2234: default: : return boost::none; is there another valid value to pass here? not FATAL? http://gerrit.cloudera.org:8080/#/c/7717/1/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: Line 134: if (last_logged_opid && last_logged_opid->term() > caller_term) { same question as in the Vote case -- is it correct to allow replacement if our last opid is 'unknown'? Line 237: if (last_logged_opid && last_logged_opid->term() > remote_cstate_->current_term()) { same http://gerrit.cloudera.org:8080/#/c/7717/1/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: Line 460: if (last_logged_opid) { same question Line 474: int64_t last_logged_term = opt_last_logged_opid->term(); is this guaranteed to be non-none? -- To view, visit http://gerrit.cloudera.org:8080/7717 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia4e4501a61cd40fdee0dc918b77675a0bc2515e7 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
