Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16952 )
Change subject: wip KUDU-2612: background task to commit transaction ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/16952/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16952/1//COMMIT_MSG@24 PS1, Line 24: There are some nuances here I haven't looked deep into the code yet, just a few top-level questions at this point. What if the leadership of txn status replica has changed while the commit phase was in progress? I saw the status of the finalization tasks is being re-fetched from the txn status tablet, but I guess it's better to ask those explicitly. One edge case is when FINALIZE_COMMIT is sent to every participant, and then leadership changes before receiving response from some of those (or all). Now, while the leadership of the txn status replica was migrating from one replica to another, one or few of the participants became unavailable. What does TxnStatusManager do? I guess it would try sending FINALIZE_COMMIT again and again before the whole process times out. What's next? http://gerrit.cloudera.org:8080/#/c/16952/1/src/kudu/tserver/tablet_server.cc File src/kudu/tserver/tablet_server.cc: http://gerrit.cloudera.org:8080/#/c/16952/1/src/kudu/tserver/tablet_server.cc@107 PS1, Line 107: RETURN_NOT_OK(TxnSystemClient::Create(opts_.master_addresses, &txn_client_)); > I'll probably need to borrow some code from the TxnManager to do this async I sounds like a good idea: it might be the case when the client cannot connect to the cluster right away because this tablet server is the first component to start in the cluster or something like that. http://gerrit.cloudera.org:8080/#/c/16952/1/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: http://gerrit.cloudera.org:8080/#/c/16952/1/src/kudu/tserver/ts_tablet_manager.h@415 PS1, Line 415: std::unique_ptr<ThreadPool> txn_commit_pool_; nit: add a doc http://gerrit.cloudera.org:8080/#/c/16952/1/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/16952/1/src/kudu/tserver/ts_tablet_manager.cc@367 PS1, Line 367: 10 Would it make sense to make this configurable via a gflag? -- To view, visit http://gerrit.cloudera.org:8080/16952 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2258dded3ab3d527cb5d0abdc7d5e7deb4da15e Gerrit-Change-Number: 16952 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <[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-Comment-Date: Fri, 15 Jan 2021 04:29:52 +0000 Gerrit-HasComments: Yes
