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 4: (14 comments) http://gerrit.cloudera.org:8080/#/c/17022/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17022/3//COMMIT_MSG@10 PS3, Line 10: FINALIZE_IN_PROGRESS state to serve as an intermediate step between : COMMIT_IN_PROGRESS > Can you briefly describe the difference between COMMIT_IN_PROGRESS and FINA Done in transactions.proto http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/integration-tests/txn_commit-itest.cc File src/kudu/integration-tests/txn_commit-itest.cc: http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/integration-tests/txn_commit-itest.cc@456 PS3, Line 456: : // The transaction should ev > nit: update? Done http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/integration-tests/txn_commit-itest.cc@694 PS3, Line 694: } > nit: constexpr? Done http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/integration-tests/txn_commit-itest.cc@708 PS3, Line 708: } : for (const auto& s : results) { > readability nit: move this up to be right after the 'for()' cycle? Done http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/integration-tests/txn_commit-itest.cc@729 PS3, Line 729: for (int i = 0; i < kNumTxns; i++) { : shared_ptr<KuduSession> txn_session; > Is it needed for some test automation or benchmarking? Consider removing t I found this helpful for ensuring there was some variation in running this test. I'll move it to VLOG. http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/transactions.proto File src/kudu/transactions/transactions.proto: http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/transactions.proto@22 PS3, Line 22: // The following state changes are expected: : // : // BeginCommit FinalizeCommit CompleteCommit : // OPEN --> COMMIT_IN_PROGRESS --> FINALIZE_IN_PROGRESS --> COMMITTED : // : // BeginCommit BeginAbort FinalizeAbort : // OPEN --> COMMIT_IN_PROGRESS --> ABORT_IN_PROGRESS --> ABORTED : // : / > Do you mind adding a comment to show list of acceptable sequences of state Done http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/txn_status_manager-test.cc File src/kudu/transactions/txn_status_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/txn_status_manager-test.cc@809 PS3, Line 809: s = txn_manager_->AbortTransaction(kTxnId2, kOwner, &ts_error); : ASSERT_TRUE(s.IsIllegalState()) << s.ToString(); > I'm not sure I understand this: I thought it would be possible to call this Nope, CompleteCommitTransaction demarcates the point after which all the FINALIZE_COMMIT ops have been replicated. FinalizeCommitTransaction demarcates the start of us sending FINALIZE_COMMIT ops, and at that point, we cannot abort. I added clearer descriptions to transactions.proto -- hopefully that clears things up. http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/txn_status_manager.h File src/kudu/transactions/txn_status_manager.h: http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/txn_status_manager.h@148 PS3, Line 148: NOTE: this can be called from multiple tas > nit: is it acceptable to have this called when abort_txn_ is already TxnSta Good call. Done http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/txn_status_manager.h@155 PS3, Line 155: abort_txn_.compare_exchange_strong(expec > I don't quite understand why not check TxnStatePB::ABORT_IN_PROGRESS instea The gist is that this state isn't synchronized with the TSM's view of the state -- it's an indicator of what state the tasks should move to next. http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/txn_status_manager.h@160 PS3, Line 160: to be run after the state has alread > nit: is it acceptable for this method to be called multiple times? If not, Done http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/txn_status_manager.cc File src/kudu/transactions/txn_status_manager.cc: http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/txn_status_manager.cc@304 PS3, Line 304: // If this was the last participant op for this task, write the finalized > warning: do not use 'else' after 'return' [readability-else-after-return] Done http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/txn_status_manager.cc@1034 PS3, Line 1034: txn_id, SecureShortDebugString(pb)), > warning: 3rd function call argument is an uninitialized value [clang-analyz Ack http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/txn_status_manager.cc@1149 PS3, Line 1149: return Status::OK(); > warning: the parameter 'user' is copied for each invocation but only used a Done http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/txn_status_manager.cc@1184 PS3, Line 1184: 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: 4 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: Tue, 09 Mar 2021 21:37:44 +0000 Gerrit-HasComments: Yes
