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 8: (11 comments) I'm on board with the runtime NO_OP clock updating change but I'm a little confused about the clock initialization aspect to this patch, and I wonder if we can/should factor that change out of this patch and address it separately. 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: sometimes the hybrid clock doesn't get updated before sending : out the first no-op This doesn't seem right. The HybridClock should always have a real-time component, therefore the timestamp should never be 1. Right? 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 ~ConsensusRoundHandler() {} nit: I know this is not your fault but destructor should go first per GSG declaration order 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? I guess the answer is that we don't have to because every entry in the master's WAL will have a monotonic timestamp, right? I'm actually not really sure about that. Maybe we should make this pure virtual and put an empty implementation in CatalogManager with a comment containing our thoughts about that. 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: int64_t initial_ts, const OpId& id nit: Mind switching the order of these (seems more intuitive for the OpId to be the first param to AddOp) and renaming initial_ts to base_ts? Can we make base_ts a Timestamp? http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/integration-tests/raft_consensus-itest.cc@180 PS8, Line 180: int64_t initial_ts, const OpId& id nit: same as above; let's make base_ts the last input param (before *req) 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? 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 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 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: // Updating the clock here means that any timestamps returned by the time this doesn't change anything in the case that the log doesn't have any time-based ops in it, right? http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/tablet/tablet_replica.cc@220 PS8, Line 220: clock_->Update(initial_safe_time); 1) Is this an attempt to guarantee monotonicity across leaders without leader leases? 2) Should we write a test around this change? http://gerrit.cloudera.org:8080/#/c/11427/8/src/kudu/tablet/tablet_replica.cc@592 PS8, Line 592: void TabletReplica::FinishConsensusOnlyRound(ConsensusRound* round) { nit: this comes after StartFollowerTransaction in the header file and the interface -- 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: 8 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, 20 Sep 2018 21:22:49 +0000 Gerrit-HasComments: Yes
