Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17022 )
Change subject: KUDU-2612: allow aborting after beginning to commit ...................................................................... Patch Set 3: (8 comments) 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 eventually succeed, treating the deleted : // participant as committed. nit: update? http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/integration-tests/txn_commit-itest.cc@694 PS3, Line 694: const nit: constexpr? http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/integration-tests/txn_commit-itest.cc@708 PS3, Line 708: ASSERT_OK(CountRows(&num_rows)); : ASSERT_EQ(initial_row_count_, num_rows); readability nit: move this up to be right after the 'for()' cycle? http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/integration-tests/txn_commit-itest.cc@729 PS3, Line 729: LOG(INFO) << Substitute("Expecting $0 rows from $1 committed transactions", : expected_rows, num_committed_txns.load()); Is it needed for some test automation or benchmarking? Consider removing this since our tests are very chatty already because of a lot of logging output from cluster components. 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: enum TxnStatePB { : UNKNOWN = 0; : OPEN = 1; : ABORT_IN_PROGRESS = 5; : ABORTED = 2; : COMMIT_IN_PROGRESS = 3; : FINALIZE_IN_PROGRESS = 6; : COMMITTED = 4; : } Do you mind adding a comment to show list of acceptable sequences of state changes for a transaction? 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 before complete CompleteCommitTransaction is called, no? 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: abort_txn_ = TxnStatePB::ABORT_IN_PROGRESS; nit: is it acceptable to have this called when abort_txn_ is already TxnStatePB::ABORTED? If not, maybe it makes sense to add a DCHECK() to enforce such precondition? http://gerrit.cloudera.org:8080/#/c/17022/3/src/kudu/transactions/txn_status_manager.h@160 PS3, Line 160: commit_timestamp_ = commit_timestamp; nit: is it acceptable for this method to be called multiple times? If not, maybe it makes sense to add a DCHECK() on kInvalidTimestamp? -- 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: 3 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: Fri, 26 Feb 2021 07:31:57 +0000 Gerrit-HasComments: Yes
