Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17017 )
Change subject: KUDU-2612: add background task to abort transaction participants ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/17017/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17017/5//COMMIT_MSG@10 PS5, Line 10: when TxnStatusManager::AbortTransaction() > nit: when TxnStatusManager::AbortTransaction() is called Done http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/client/transaction-internal.cc File src/kudu/client/transaction-internal.cc: http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/client/transaction-internal.cc@309 PS5, Line 309: case TxnStatePB::ABORT_IN_PROGRESS: : *is_complete = false; : *completion_status = Status::Aborted("transaction is being aborted"); : break; > I understand we consider ABORT_IN_PROGRESS to be eventually aborted no matt Done http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc File src/kudu/integration-tests/txn_commit-itest.cc: http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@262 PS5, Line 262: > nit: if you interested in syntactic sugar by any chance, with C++17 it's po Nice! That's really convenient, thanks! I separated out a few of these into a separate patch. http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@330 PS5, Line 330: // to wait for this to happen, since 'is_complete' is contingent on > So, this now reports IsAborted() for both TXN_ABORTED and TXN_ABORT_IN_PROG Right. Though I updated this so 'is_complete' is only true if we've finished aborting. http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@332 PS5, Line 332: UALLY([&] { > What should we report on a status of a transaction in ABORT_IN_PROGRESS sta At first my rationale was that if the transaction was aborted, from a client perspective, there is no difference between ABORT_IN_PROGRESS and ABORTED, since any input rows wouldnt be returned either way. After thinking about it a bit more though, there is a difference in that ABORT_IN_PROGRESS implies that locks may still be held, whereas ABORTED implies that all locks have been released, which does seem to be useful to know about. I'll take this suggestion. http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/integration-tests/txn_commit-itest.cc@784 PS5, Line 784: > nit: finalize commit and abort Done http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/transactions/txn_status_manager.cc File src/kudu/transactions/txn_status_manager.cc: http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/transactions/txn_status_manager.cc@331 PS5, Line 331: : } > nit: update to write the abort record? Done http://gerrit.cloudera.org:8080/#/c/17017/5/src/kudu/transactions/txn_status_manager.cc@982 PS5, Line 982: RETURN_NOT_OK(status_tablet_.UpdateTransaction( : txn_id, mutable_data->pb, ts_error)); : txn_lock.Commit(); : return Status::OK(); : } : : Status TxnStatusManager::FinalizeAbortTransaction(int64_t txn_id) { : leader_lock_.AssertAcquiredForReading(); : > Just curious, it seems this should belong to the previous patch which intro Ah nice catch! This isn't necessary -- the commit tasks clean up after themselves. This must be from an earlier version based off an older version of the previous patch. As for the commit timestamp, I'll remove it from this patch, but re-add in in a follow-up, since I need to update the commit logic a bit to handle races between commit and abort. -- To view, visit http://gerrit.cloudera.org:8080/17017 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I484c315c6f7331c5ec12cb06370fbaae9c7c343e Gerrit-Change-Number: 17017 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 09 Feb 2021 07:18:11 +0000 Gerrit-HasComments: Yes
