Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17037 )
Change subject: KUDU-2612 tablet servers automatically register txn participants ...................................................................... Patch Set 2: (19 comments) http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/integration-tests/fuzz-itest.cc File src/kudu/integration-tests/fuzz-itest.cc: http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/integration-tests/fuzz-itest.cc@245 PS3, Line 245: val(std::move(v)) {} > warning: std::move of the variable 'v' of the trivially-copyable type 'opti ignored http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/integration-tests/txn_write_ops-itest.cc File src/kudu/integration-tests/txn_write_ops-itest.cc: http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/integration-tests/txn_write_ops-itest.cc@90 PS3, Line 90: > warning: using decl 'TabletServer' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/integration-tests/txn_write_ops-itest.cc@185 PS3, Line 185: > warning: std::move of the const expression has no effect; remove std::move( Done http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tablet/tablet_replica.h@144 PS3, Line 144: // TODO have a way to wait for any state? > warning: missing username/bug in TODO [google-readability-todo] Done 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 :) Yeah, this patch is very raw: I had too much distractions in the process, sorry 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 Done 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? I ended up renaming it into TxnOpDispatcher -- the rationale is that this class dispatches all txn-enabled write operations processed by a tablet replica. 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? Well, it turns out that's not possible to have 'ready_' set to 'true' and have non-empty 'ops_queue_'. I added corresponding CHECK(). 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 Right -- that's a point to clarify (and, actually, re-do this). http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tablet/tablet_replica.cc@540 PS3, Line 540: RETURN_NOT_OK(CheckRunning()); > warning: the parameter 'txn_participant_registrant' is copied for each invo Done 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 Exactly -- I was weighing in a couple of alternatives: I moved this out from here and switched to TxnSystemClient::CheckOpenTxnSystemTable() 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 I resorted to using std::function instead: I wasn't sure introducing a factory was the best option. I can be convinced otherwise, of course :) 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 Good point! I slightly modified this comment and added it into the code. http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tserver/ts_tablet_manager.cc@244 PS3, Line 244: using kudu::tablet::TABLET_DATA_READY; > warning: using decl 'OpCompletionCallback' is unused [misc-unused-using-dec Done http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tserver/ts_tablet_manager.cc@245 PS3, Line 245: using kudu::tablet::TABLET_DATA_TOMBSTONED; > warning: using decl 'ParticipantOpState' is unused [misc-unused-using-decls Done http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tserver/ts_tablet_manager.cc@262 PS3, Line 262: namespace tserver { > warning: using decl 'unique_ptr' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tserver/ts_tablet_manager.cc@1497 PS3, Line 1497: MonoTime deadline, > warning: method 'RegisterAndBeginParticipantTxnTask' can be made static [re Done http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tserver/ts_tablet_manager.cc@1852 PS3, Line 1852: > warning: the parameter 'replica' is copied for each invocation but only use Done http://gerrit.cloudera.org:8080/#/c/17037/3/src/kudu/tserver/ts_tablet_manager.cc@1856 PS3, Line 1856: > warning: the parameter 'cb' is copied for each invocation but only used as Done -- 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 <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 17 Feb 2021 04:01:04 +0000 Gerrit-HasComments: Yes
