Andrew Wong 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 9: (11 comments) http://gerrit.cloudera.org:8080/#/c/11427/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11427/8//COMMIT_MSG@22 PS8, Line 22: once_writes-itest to actually churn elections. Previously it : attempted this wi > This doesn't seem right. The HybridClock should always have a real-time com You're right, I was seeing issues with the mock clock and with certain bootstrapping scenarios, and I was conflating the root cause of both to be this update of the clock. I've backed out this change and handled the mock clock issue specifically in the test that was failing. http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/consensus/raft_consensus.h@927 PS8, Line 927: virtual Status StartFollowerTransaction(const scoped_refptr<ConsensusRound>& context) = 0; > nit: I know this is not your fault but destructor should go first per GSG d Done http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/consensus/raft_consensus.h@929 PS8, Line 929: ; > Shouldn't we implement this on masters as well? Nope, this is implemented by the TabletReplica, which the master uses. Though that is a good question if directed at Part 3. Will have to think about it a bit more. http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/integration-tests/raft_consensus-itest.cc@179 PS8, Line 179: const OpId& id, int64_t base_ts, C > nit: Mind switching the order of these (seems more intuitive for the OpId t It could be a timestamp, but given how I'm using it below (basic arithmetic functions not available to Timestamps) I'll leave it as is. http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/integration-tests/raft_consensus-itest.cc@180 PS8, Line 180: const OpId& id, int64_t base_ts, > nit: same as above; let's make base_ts the last input param (before *req) Done http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/integration-tests/raft_consensus-itest.cc@232 PS8, Line 232: int64_t > Hmm, can we make this return a Timestamp? Same here, I could, but it doesn't lend itself well to how I'm using it, so I'll hold off. http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/integration-tests/timestamp_advancement-itest.cc File src/kudu/integration-tests/timestamp_advancement-itest.cc: http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/integration-tests/timestamp_advancement-itest.cc@245 PS8, Line 245: > nit: extra semicolon Done http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/integration-tests/timestamp_advancement-itest.cc@251 PS8, Line 251: > ... when a leader gets elected and replicates a no-op message Done http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/tablet/tablet_replica.cc@217 PS8, Line 217: // may invoke TabletReplica::StartFollowerTransaction() during startup, > this doesn't change anything in the case that the log doesn't have any time Ended up removing this. http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/tablet/tablet_replica.cc@220 PS8, Line 220: RETURN_NOT_OK(consensus_->Start( > 1) Is this an attempt to guarantee monotonicity across leaders without lead Ended up removing this. http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/tablet/tablet_replica.cc@592 PS8, Line 592: consensus::ReplicateMsg* replicate_msg = round->replicate_msg(); > nit: this comes after StartFollowerTransaction in the header file and the i Done -- 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: 9 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: Fri, 21 Sep 2018 00:00:06 +0000 Gerrit-HasComments: Yes
