Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management ...................................................................... Patch Set 6: (11 comments) http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc File src/kudu/transactions/txn_status_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@281 PS6, Line 281: ReservoirSample What is the significance of ReservoirSample() here if all_updates.size() < kNumUpdatesInParallel? As I understand, in this case not a single transaction will change its state in a concurrent manner, no? http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@328 PS6, Line 328: LOG(DFATAL) << "bad update"; Why LOG(DFATAL) is better than simply failing the test with FAIL() ? http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@348 PS6, Line 348: ASSERT_OK(txn_manager_->BeginTransaction(1, kOwner)); Do you mind adding a scenario to call BeginTransaction() after this, but call it with kWrongUser? http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@357 PS6, Line 357: ParticipantIdsByTxnId prts_by_txn_id = txn_manager_->GetParticipantsByTxnIdForTests(); What is the significance of the result? In other words, what is expected to be in the 'prts_by_txn_id' container? http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404 PS6, Line 404: can't nit: cannot http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404 PS6, Line 404: reigster register http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@409 PS6, Line 409: ASSERT_OK(txn_manager_->RegisterParticipant(kTxnId1, ParticipantId(1), kOwner)); Do you mind adding a scenario to show that register the same participant with the same transaction is an idempotent operation (i.e. calling this again will be a success)? http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.h File src/kudu/transactions/txn_status_manager.h: http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.h@114 PS6, Line 114: with that transaction ID What's the expectation on a per-transaction set of IDs? I see the implementation sorts those, maybe it's worth reflecting that in this in-line method doc? http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h File src/kudu/transactions/txn_status_manager.h: http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@77 PS5, Line 77: > Makes sense, thanks for the clarification. We discussed this with Andrew a bit last week or so. As I understand, it's possible to change this (maybe in a follow-up changelist) so no transaction ID would be necessary even in case of sharded XST. The idea is that the request to assign an transaction ID is sent randomly to one of XST tablets, and that tablet will read its last transaction ID record, increment it, and forward the request to create a transaction record to the appropriate partition (i.e. to other tablet of XST). That tablet on its own will check for the last transaction ID it recorded and try to insert a new record with the new ID. If failed, it will increment its last seen transaction ID and forward it to corresponding tablet XST tablet, and so on. http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc File src/kudu/transactions/txn_status_manager.cc: http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc@67 PS6, Line 67: highest_txn_id_ Is it guaranteed we don't have a race here while setting the highest_txn_id_? Could you add a comment to clarify on this? http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc@94 PS6, Line 94: !ret nit for here and elsewhere: wrap these unlikely error conditions into PREDICT_FALSE() ? -- To view, visit http://gerrit.cloudera.org:8080/16044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I371bb200cf65073ae3ac7cb311ab9a0b8344a636 Gerrit-Change-Number: 16044 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 07 Jul 2020 18:06:13 +0000 Gerrit-HasComments: Yes
