Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16648 )
Change subject: (wip) KUDU-2612: restrict TxnStatusManager calls to be made by the leader only ...................................................................... Patch Set 1: (6 comments) Just a quick first pass. http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/tablet/tablet_replica.cc@147 PS1, Line 147: reload_txn_status_tablet_pool_(reload_txn_status_tablet_pool), : txn_coordinator_(meta_->table_type() && : *meta_->table_type() == TableTypePB::TXN_STATUS_TABLE ? : DCHECK_NOTNULL(txn_coordinator_factory)->Create(this) : nullptr), : mark_dirty_clbk_(meta_->table_type() && : *meta_->table_type() == TableTypePB::TXN_STATUS_TABLE ? : [this](const string& reason) { : this->TxnStatusReplicaStateChanged(this->tablet_id(), reason); : } : std::move(cb)), Maybe, at this point it's time to separate TxnStatusTabletReplica info a sub-class of TabletReplica, and instantiating one ore another in TSTabletManager::CreateAndRegisterTabletReplica() based on meta_->table_type()? http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/tablet/tablet_replica.cc@302 PS1, Line 302: CHECK_OK(leader_cb()); Just curious: is this going to be called if leadership status hasn't changed for a replica, i.e. if the former leader wins latest election round again? http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/transactions/txn_status_manager.h File src/kudu/transactions/txn_status_manager.h: http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/transactions/txn_status_manager.h@87 PS1, Line 87: class ScopedLeaderSharedLock { Would it make sense to extract the common part of the code from CatalogManager::ScopedLeaderSharedLock and this class into some common class (maybe, using template class approach) and re-use the common code in catalog manager and here? http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/transactions/txn_status_manager.cc File src/kudu/transactions/txn_status_manager.cc: http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/transactions/txn_status_manager.cc@83 PS1, Line 83: constrution nit: construction Also, add a stop in the end of the sentence. http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/tserver/ts_tablet_manager.cc@386 PS1, Line 386: disks nit: data directories ? http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/tserver/ts_tablet_manager.cc@389 PS1, Line 389: RETURN_NOT_OK(ThreadPoolBuilder("txn-status-tablet-reload") : .set_max_threads(max_reload_threads) : .Build(&reload_txn_status_tablet_pool_)); Is it necessary to gracefully shutdown this pool along with other tablet pools uses in TsTabletManager or it's OK to rely on its destructor's logic as is? -- 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: 1 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: Mon, 26 Oct 2020 23:14:43 +0000 Gerrit-HasComments: Yes
