Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11427 )
Change subject: KUDU-2463 pt 2: bump MVCC safe time on Raft no-op ...................................................................... Patch Set 12: Code-Review+1 (13 comments) looking good just minor feedback now http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/consensus/raft_consensus.h@929 PS12, Line 929: virtual void FinishConsensusOnlyRound(ConsensusRound* round) = 0; this method should be documented in the interface, either in the block comment above or inline here http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/mvcc.cc File src/kudu/tablet/mvcc.cc: http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/mvcc.cc@190 PS12, Line 190: Getting here means that we are about to apply a transaction out of : // order makes sense; did you observe this? http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/tablet_replica.cc@646 PS12, Line 646: no-op no-op used to assert term leadership http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/tablet_replica.cc@647 PS12, Line 647: further in the same and later http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/tablet_replica.cc@648 PS12, Line 648: with them using the timestamp of such no-ops http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/tablet_replica.cc@649 PS12, Line 649: it them http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/tablet_replica.cc@651 PS12, Line 651: such s/such/MVCC safe time/ http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/tablet_replica.cc@653 PS12, Line 653: started s/started/prepared/ http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/tablet_replica.cc@653 PS12, Line 653: submit s/submit/run/ http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/tablet_replica.cc@654 PS12, Line 654: on which transactions are which is the same mechanism we use to serialize transactions http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/tablet_replica.cc@654 PS12, Line 654: the adjustment function to the the safe time adjustment function on the http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/tablet_replica.cc@656 PS12, Line 656: op_type == consensus::NO_OP) { : DCHECK(replicate_msg->has_noop_request()); : : // If the flag is unset, the no-op is assumed to be the Raft leadership : // no-op, and thus, in order. : if (!replicate_msg->noop_request().has_timestamp_in_opid_order() || : replicate_msg->noop_request().timestamp_in_opid_order()) { can we just combine these if statements? i.e. // The timestamp of a Raft no-op is guaranteed to be lower than the // timestamps of writes in further terms. As such, we are able to bump the // MVCC safe time with them, as further transaction timestamps are guaranteed // to be higher than it. // // It is important for such updates to be serialized with respect to // transactions. To ensure that we only advance the safe time with the no-op // of term N after all transactions of term N-1 have been started, we submit // the adjustment function to the prepare thread, on which transactions are // serialized. // // If the 'timestamp_in_opid_order' flag is unset, the no-op is assumed to be // the Raft leadership no-op from a version of Kudu that only supported creating // a no-op to assert a new leadership term, in which case it would be in order. if (op_type == consensus::NO_OP && (!replicate_msg->noop_request().has_timestamp_in_opid_order() || replicate_msg->noop_request().timestamp_in_opid_order())) { DCHECK(replicate_msg->has_noop_request()); (note: I riffed a little bit on that last comment paragraph) http://gerrit.cloudera.org:8080/#/c/11427/12/src/kudu/tablet/tablet_replica.cc@664 PS12, Line 664: CHECK_OK I just traced this to make sure, but we may want to add comment with something like the following: // We are guaranteed that the prepare pool token is running now because TabletReplica::Stop() stops // RaftConsensus before it stops the prepare pool token and this callback is invoked while the // RaftConsensus lock is held. -- To view, visit http://gerrit.cloudera.org:8080/11427 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbf812e2cbeeee7c322fd980245cfe40c886a15a Gerrit-Change-Number: 11427 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 11 Oct 2018 17:47:12 +0000 Gerrit-HasComments: Yes
