Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management ...................................................................... Patch Set 7: (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@57 PS6, Line 57: using std::string; > warning: using decl 'shared_ptr' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@281 PS6, Line 281: vector<Status> all_updates.size = 3 * 10, and kNumUpdatesInParallel = 20, so we're selecting a random subset to do in parallel. > in this case not a single transaction will change its state in a concurrent > manner, no? I'm not sure I understand why this wouldn't be considered concurrent, regardless of count. http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@328 PS6, Line 328: > Why LOG(DFATAL) is better than simply failing the test with FAIL() ? Done http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@348 PS6, Line 348: ASSERT_OK(txn_manager_->RegisterParticipant(1, Partic > Do you mind adding a scenario to call BeginTransaction() after this, but ca Done http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@357 PS6, Line 357: s = txn_manager_->RegisterParticipant(1, ParticipantId(2), kWrongUser); > What is the significance of the result? In other words, what is expected t Whoops, done http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404 PS6, Line 404: s.IsIlle > register Done http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@404 PS6, Line 404: _TRUE > nit: cannot This is common all over our codebase and has virtually no grammatical difference. Any particular reason? http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager-test.cc@409 PS6, Line 409: ST_F(TxnStatusManagerTest, TestRegisterParticipantsWithStates) { > Do you mind adding a scenario to show that register the same participant wi Done 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: > What's the expectation on a per-transaction set of IDs? I see the implemen Done 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: // locking. > Is it guaranteed we don't have a race here while setting the highest_txn_id Done This isn't a thread-safe method. Updated the method comments and added a note here. http://gerrit.cloudera.org:8080/#/c/16044/6/src/kudu/transactions/txn_status_manager.cc@94 PS6, Line 94: :loc > nit for here and elsewhere: wrap these unlikely error conditions into PREDI Done -- 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: 7 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: Wed, 08 Jul 2020 07:19:39 +0000 Gerrit-HasComments: Yes
