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 6: (3 comments) looks pretty good overall http://gerrit.cloudera.org:8080/#/c/11427/5/src/kudu/consensus/time_manager.cc File src/kudu/consensus/time_manager.cc: http://gerrit.cloudera.org:8080/#/c/11427/5/src/kudu/consensus/time_manager.cc@81 PS5, Line 81: clock_->Update(last_serial_ts_assigned_); Is this needed? http://gerrit.cloudera.org:8080/#/c/11427/5/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/11427/5/src/kudu/tablet/tablet_replica.cc@594 PS5, Line 594: BOOTSTRAPPING Do we want to adjust the same time while it's bootstrapping? Won't that happen in tablet_bootstrap.cc automatically? http://gerrit.cloudera.org:8080/#/c/11427/5/src/kudu/tablet/tablet_replica.cc@605 PS5, Line 605: tablet_ Based on the implementation, we don't have to, but I wonder if we should make a copy of tablet_ under the lock_ in case this gets called concurrent with Shutdown() or something. Based on looking at the code right now, as written that's not possible, because this callback would be invoked from the raft_pool_token_, which is Shutdown() from RaftConsensus::Stop(), which gets called from TabletReplica::Stop(). If we don't want to make a future-defensive copy, maybe we can add a DCHECK(tablet_) here and specify in the contract that the TabletReplica cannot be Shutdown() when the callback gets invoked; the tough thing about that is that ConsensusRoundHandler is an interface and it's not really supposed to know anything about a TabletReplica. Maybe we should just make the defensive copy, and add a comment about this. I wonder if Alexey has an opinion on this. -- 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: 6 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 03:16:30 +0000 Gerrit-HasComments: Yes
