Andrew Wong 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: (7 comments) 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@154 PS1, Line 154: this->TxnStatusReplicaStateChanged(this->tablet_id(), reason); Do we also need to call cb() here? 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 su FWIW I think this would be more plumbing that it's worth. http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/tablet/tablet_replica.cc@302 PS1, Line 302: leader_cb nit: why does this need to be a lambda? 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@189 PS1, Line 189: // nit: remove extra slashes? http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/transactions/txn_status_manager.cc@347 PS1, Line 347: return s; // unreachable nit: you can use __builtin_unreachable() http://gerrit.cloudera.org:8080/#/c/16648/1/src/kudu/transactions/txn_status_manager.cc@357 PS1, Line 357: check nit: This gets used once -- why does it need to be a lambda? 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@1238 PS1, Line 1238: tablet_copy_pool_->Shutdown(); We should to shut down our new pool here. -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 27 Oct 2020 19:59:55 +0000 Gerrit-HasComments: Yes
