Mike Percy has posted comments on this change. Change subject: consensus: Improve contract for API to fetch last-logged op id ......................................................................
Patch Set 1: (15 comments) http://gerrit.cloudera.org:8080/#/c/7717/1/src/kudu/consensus/consensus_peers-test.cc File src/kudu/consensus/consensus_peers-test.cc: Line 210: ASSERT_OPID_EQ(first, message_queue_->GetLastOpIdInLog()); > warning: local copy '_left210' of the variable 'first' is never modified; c Done http://gerrit.cloudera.org:8080/#/c/7717/1/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: Line 125: log_cache_(metric_entity, std::move(log), local_peer_pb_.permanent_uuid(), tablet_id_), > warning: passing result of std::move() as a const reference argument; no mo Done Line 126: metrics_(std::move(metric_entity)), > warning: passing result of std::move() as a const reference argument; no mo Done Line 143: log_cache_.Init(queue_state_.last_appended); > mind adding a TODO to merge LogCache::Init into the LogCache ctor, given we Done 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 en Done Line 250: pending->SetInitialCommittedOpId(info.last_committed_id); > I believe it was on purpose that this was called after the appending of the Ah, thank you for the catch. This caused a bug that I was having trouble diagnosing. PS1, Line 1492: opt_local_last_logged_opid) ? > you can use .value_or(MinimumOpId()) Actually, in this patch I can bypass this because we know RaftConsensus is running so we can use queue_->GetLastOpIdInLog() Line 1493: : MinimumOpId(); > is this right? I thought, if we don't know our own local op id, then it wou Not sure what I was thinking. Removed. Line 2226: if (!queue_ || !pending_) return boost::none; > think it's worth a comment here explaining the cases where we could hit thi Done PS1, Line 2234: default: : return boost::none; > is there another valid value to pass here? not FATAL? Hrm, yeah. It's a protobuf enum that is exposed to a remote read API via RPC, so for forward compatibility I'll make it a DFATAL and return boost::none. http://gerrit.cloudera.org:8080/#/c/7717/1/src/kudu/tablet/tablet_metadata.h File src/kudu/tablet/tablet_metadata.h: Line 216: void SetPreFlushCallback(StatusClosure callback) { pre_flush_callback_ = callback; } > warning: parameter 'callback' is passed by value and only copied once; cons Done 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 I'm not sure what else we can do. If we have tombstoned a failed replica, we will not know the last-logged opid, and this covers that case. Line 237: if (last_logged_opid && last_logged_opid->term() > remote_cstate_->current_term()) { > same see above 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 see my response in the in other file Line 474: int64_t last_logged_term = opt_last_logged_opid->term(); > is this guaranteed to be non-none? Ah, indeed no. I'll give it the same treatment as the others for now. -- 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: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
