Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16648 )
Change subject: KUDU-2612: restrict TxnStatusManager calls to be made by the leader only ...................................................................... Patch Set 11: (10 comments) Overall looks good, just more nits and a couple of test suggestions. http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/tablet/tablet_replica.h@329 PS11, Line 329: transaction status manager table nit: just "transaction status table" http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/tablet/tablet_replica.h@332 PS11, Line 332: Thus, the caller needs to call : // 'DecreaseTxnCoordinatorTaskCounter' after the task is executed or : // aborted to unblock the tablet shutting down process. nit: more concretely, if this returns 'true', callers must call DecreaseTxnCoordinatorTaskCounter(). Same below. http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/tablet/tablet_replica.h@336 PS11, Line 336: successfully nit: these methods don't run anything, so how about "if the caller should run the staleness task" Same below http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/tablet/tablet_replica.h@340 PS11, Line 340: transaction status manager : // table nit: here too http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/tablet/tablet_replica.cc@1010 PS11, Line 1010: DCHECK nit: could use DCHECK_GE http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/transactions/txn_status_manager.h File src/kudu/transactions/txn_status_manager.h: http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/transactions/txn_status_manager.h@100 PS11, Line 100: The expect type of 'txn_coordinator' is a subclass of TxnCoordinator, : // e.g. TxnStatusManager. nit: more specifically, 'txn_coordinator' must be a TxnStatusManager* because of the down_cast, no? http://gerrit.cloudera.org:8080/#/c/16648/6/src/kudu/transactions/txn_status_manager.cc File src/kudu/transactions/txn_status_manager.cc: http://gerrit.cloudera.org:8080/#/c/16648/6/src/kudu/transactions/txn_status_manager.cc@83 PS6, Line 83: ing all operations replicated " : "by the previou > After thinking more, I decide to remove the crash logic and just return, si Sounds good. Curious if we have test coverage for this. http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/transactions/txn_status_manager.cc File src/kudu/transactions/txn_status_manager.cc: http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/transactions/txn_status_manager.cc@239 PS11, Line 239: TABLET_NOT_RUNNING); Do we have tests for this? http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/transactions/txn_status_manager.cc@356 PS11, Line 356: status_tablet_.tablet_replica_->IsShuttingDown(); nit: why not put this directly in the if() condition? Same with other instances with task_enabled http://gerrit.cloudera.org:8080/#/c/16648/11/src/kudu/transactions/txn_status_manager.cc@358 PS11, Line 358: "The tablet is already shutting down or shutdown. "; nit: this might not be that useful. It'll be obvious that the tablet is shutting down from surrounding messages. This should probably just say "Not loading TxnStatusManager" or something -- To view, visit http://gerrit.cloudera.org:8080/16648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42c1ad095dcb4bdffcbe0ecf9631a60bac208c2a Gerrit-Change-Number: 16648 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 21 Jan 2021 07:24:11 +0000 Gerrit-HasComments: Yes
