Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17022 )
Change subject: KUDU-2612: allow aborting after beginning to commit ...................................................................... Patch Set 8: (13 comments) http://gerrit.cloudera.org:8080/#/c/17022/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17022/5//COMMIT_MSG@13 PS5, Line 13: The goal is to allow for aborts to occur in the event that we've begun > nit: FINALIZE_COMMIT Done http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/integration-tests/txn_commit-itest.cc File src/kudu/integration-tests/txn_commit-itest.cc: http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/integration-tests/txn_commit-itest.cc@433 PS5, Line 433: // The transaction should eventually fail, treating the deleted participant : // as a fatal error. > nit: update this comment to reflect the change of the transaction state to Done http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/integration-tests/txn_commit-itest.cc@740 PS5, Line 740: // sleep before > nit: might be constexpr? Done http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/integration-tests/txn_commit-itest.cc@760 PS5, Line 760: ASSERT_OK(CountRows(&num_rows)); : // NOTE: we can compare an exact count here and not worry about wh > Is my understanding is correct that there should be no flakiness here? As Exactly, it doesn't matter if a rollback is still in flight because the rows aren't visible anyway as long as they aren't committed, even if their aborts are still in progress. Done http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/integration-tests/txn_commit-itest.cc@764 PS5, Line 764: const int expected_rows = initial_row_count_ + kNumRowsPerTxn * num_committed_txns; > Is it worth adding a check to make sure all transaction are eventually comp Done http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/integration-tests/txn_commit-itest.cc@838 PS5, Line 838: shared_ptr<KuduTransaction> txn; : shared_ptr<KuduSession> txn > Just curious: is this scenario stable even in the presence of scheduling an Indeed, the injected latency is defined at L833 http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/transactions/transactions.proto File src/kudu/transactions/transactions.proto: http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/transactions/transactions.proto@22 PS5, Line 22: // The following state changes are expected: > Thank you for adding this in-line documentation! It helps to understand th Yep! Frankly I should have drafted something much earlier in code, but hopefully better late than never! http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/transactions/txn_status_manager.h File src/kudu/transactions/txn_status_manager.h: http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/transactions/txn_status_manager.h@159 PS5, Line 159: Expected : // to be run after a ABORT_IN_PROGRESS record has been persisted to dis > Does it make sense to add DCHECK() to enforce this invariant (if it's an in It's not very straightforward to check the state -- I'll at least disambiguate that I'm referring to the txn state and not some CommitTask member. http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/transactions/txn_status_manager.h@230 PS5, Line 230: > Is this still so even with SetNeedsFinalizeAbort() ? Good catch, I'll update this as it's not quite true. http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/transactions/txn_status_manager.cc File src/kudu/transactions/txn_status_manager.cc: http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/transactions/txn_status_manager.cc@386 PS5, Line 386: response, > nit: response Done http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/transactions/txn_status_manager.cc@1047 PS5, Line 1047: state == TxnStatePB::FINALIZE_IN_PROGRESS || > warning: 3rd function call argument is an uninitialized value [clang-analyz Ack http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/transactions/txn_status_manager.cc@1180 PS5, Line 1180: scoped_refptr<TransactionEntry> txn; > warning: 3rd function call argument is an uninitialized value [clang-analyz Ack http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/transactions/txn_status_manager.cc@1206 PS5, Line 1206: return Status::OK(); > warning: 3rd function call argument is an uninitialized value [clang-analyz Ack -- To view, visit http://gerrit.cloudera.org:8080/17022 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If1b6596df2db5601f7e17e528ad6dc68057b67f8 Gerrit-Change-Number: 17022 Gerrit-PatchSet: 8 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-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sun, 21 Mar 2021 17:07:25 +0000 Gerrit-HasComments: Yes
