Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16044 )
Change subject: KUDU-2612 p2: introduce transaction status management ...................................................................... Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_entry.h File src/kudu/transactions/txn_status_entry.h: http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_entry.h@43 PS5, Line 43: status transaction? http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc File src/kudu/transactions/txn_status_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@176 PS5, Line 176: make_shared shouldn't these be unique_ptrs? http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@204 PS5, Line 204: // As a sanity check, there should have been at least one success per batch. I think it helps understanding this if you add that the reason why not all of the transactions are successful is that the BeginTransaction will return an error if it knows about a higher txn id. http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@236 PS5, Line 236: // have registered participants before the transaction was registered. why not simply registering the txn first and then registering only the participants concurrently and expect all of them to succeed? http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@304 PS5, Line 304: transactioN nit: lower-case "n" 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@55 PS5, Line 55: void VisitTransactionEntries(int64_t txn_id, TxnStatusEntryPB status_entry_pb, shouldn't these methods be commented? http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@73 PS5, Line 73: an nit: a http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@77 PS5, Line 77: Status BeginTransaction(int64_t txn_id, const std::string& user); Wouldn't it be easier to change txn_id to an out parameter that is automatically incremented? otherwise multiple clients could try to begin a transaction with the same ID and have to retry. http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@86 PS5, Line 86: is user-initiated nit: isn't? -- 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: 5 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, 30 Jun 2020 16:54:11 +0000 Gerrit-HasComments: Yes
