Mike Percy has posted comments on this change. Change subject: KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread ......................................................................
Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/5294/4//COMMIT_MSG Commit Message: PS4, Line 12: which we know Not sure this is true, see comments inline PS4, Line 18: consensus thread Is the consensus thread the prepare thread? Not sure which pool we are talking about here. http://gerrit.cloudera.org:8080/#/c/5294/4/src/kudu/tablet/transactions/transaction_driver.cc File src/kudu/tablet/transactions/transaction_driver.cc: Line 102: prepare_latch_(0) { Since a given transaction prepares only once, why not initialize the prepare_latch_ to 1? Then on abort we can count it down. PS4, Line 118: in the consensus thread I'm confused by this comment. I'm pretty sure this is executed on a TabletService service pool thread: TabletServiceImpl::Write() -> TabletPeer::SubmitWrite() -> TabletPeer::NewLeaderTransactionDriver() -> TransactionDriver::Init() PS4, Line 148: innited typo: inited Line 391: scoped_refptr<TransactionDriver> ref(this); This is pretty smelly. Can we not ensure the caller has a ref, instead? Also, if this is so racy, what guarantees that this object is not destroyed before we take the ref on this line? If we really must do this then we should do a more careful job of documenting the lifecycle guarantees. Line 435: nit: spurious line -- To view, visit http://gerrit.cloudera.org:8080/5294 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
