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 5: Code-Review+1 (9 comments) Looks good overall, just a few nits. 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 succeed, treating the deleted : // participant as committed. nit: update this comment to reflect the change of the transaction state to aborted? It seems the comment at lines 457-458 has already been updated. http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/integration-tests/txn_commit-itest.cc@740 PS5, Line 740: int kMaxSleepMs nit: might be constexpr? http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/integration-tests/txn_commit-itest.cc@760 PS5, Line 760: const int expected_rows = initial_row_count_ + kNumRowsPerTxn * num_committed_txns; : VLOG(1) << Substitute("Expecting $0 rows from $1 committed transac Is my understanding is correct that there should be no flakiness here? As I understand, since KuduTransaction::Rollback() only initiates a rollback, some transactions might be still in flight at this point. However, we have 'read committed' isolation for multi-row transactional writes, and that guarantees there cannot be a flakiness here. BTW, do you think it's worth adding a comment explaining this? http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/integration-tests/txn_commit-itest.cc@764 PS5, Line 764: } Is it worth adding a check to make sure all transaction are eventually completed (i.e. committed or aborted) and no transaction is stuck in a limbo state due to some hiccup in the transaction orchestration process triggered by the race above? http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/integration-tests/txn_commit-itest.cc@838 PS5, Line 838: ASSERT_OK(txn->Commit(/*wait*/false)); : ASSERT_OK(txn->Rollback()); Just curious: is this scenario stable even in the presence of scheduling anomalies? I can imagine that the main test thread might be put aside from a CPU after calling txn->Commit(false) but before calling txn->Rollback() while transaction orchestration threads a running. Does it make sense to inject a delay into txn finalization step to avoid flakiness? 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 the transaction lifecycle and the orchestration process better. 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 the state has already been set to ABORT_IN_PROGRESS. Does it make sense to add DCHECK() to enforce this invariant (if it's an invariant indeed)? http://gerrit.cloudera.org:8080/#/c/17022/5/src/kudu/transactions/txn_status_manager.h@230 PS5, Line 230: Once set to non-UNKNOWN, cannot be changed. Is this still so even with SetNeedsFinalizeAbort() ? 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: reseponse nit: response -- 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: 5 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: Wed, 17 Mar 2021 05:52:48 +0000 Gerrit-HasComments: Yes
