Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 )
Change subject: WIP KUDU-2612 tablet servers automatically register txn participants ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.h@371 PS2, Line 371: DoIt Heh :) Maybe SubmitBeginTxnAndWriteOps or somesuch? http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.h@485 PS2, Line 485: std::unordered_map<int64_t, TxnParticipantRegistrator> txn_registrators_; Will this ever get erased from? Maybe when we apply the commit or abort for the transaction? http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.cc@1194 PS2, Line 1194: TxnParticipantRegistrator nit: maybe TxnParticipantRegistrant? http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.cc@1234 PS2, Line 1234: // If there are still pending operations into the queue, enqueue this one : // to order them in FIFO manner. Would it make sense to submit them all here? http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tablet/tablet_replica.cc@1258 PS2, Line 1258: std::lock_guard<simple_spinlock> guard(lock_); : while (!ops_queue_.empty()) { Is it possible that we'll insert to the queue as we're submitting? or after submitting? If so, will we be left with pending writes that never get submitted? http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/transactions/txn_system_client.cc File src/kudu/transactions/txn_system_client.cc: http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/transactions/txn_system_client.cc@422 PS2, Line 422: // TODO(aserbin): should it be done only conditionally? : s = txn_client->OpenTxnStatusTable(); : } Might this cause issues if lazily initializing the transaction status table? http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tserver/tablet_service.cc@1655 PS2, Line 1655: avoid dependency on TxnSystemClient. I'm fine leaving this as is, but another approach would be to use generics to define some tablet::TxnRegistrationFactory that's define at the TSTabletManager layer, similar to the TxnStatusManagerFactory http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/17037/2/src/kudu/tserver/ts_tablet_manager.cc@1510 PS2, Line 1510: RegisterParticipant A future improvement we can do here is to use the async variant that shares a callback with the BEGIN_TXN op, and upon calling the callback, each could check whether both the registration and the BEGIN_TXN op are complete, and if so, run submit the writes. That said, it seems like it would complicate the error handling quite a lot, so it's probably fine leaving it as future work, given this only happens once per transaction anyway. -- To view, visit http://gerrit.cloudera.org:8080/17037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia383f7afd208c44695c57aab82e3818fa1712ce6 Gerrit-Change-Number: 17037 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 08 Feb 2021 20:34:35 +0000 Gerrit-HasComments: Yes
