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 6: (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: transa > transaction? Done 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: > shouldn't these be unique_ptrs? Done http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@204 PS5, Line 204: } > I think it helps understanding this if you add that the reason why not all Done http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@236 PS5, Line 236: string prt = ParticipantId(i % kUniqueParticipantIds); > why not simply registering the txn first and then registering only the part I wanted to exercise the error cases for registration of participants in a concurrent context as well while ensuring some successes. I'll update a bit. http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager-test.cc@304 PS5, Line 304: threads) { > nit: lower-case "n" Done 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: // Builds a TransactionEntry for the given metadata and keeps track of it in > shouldn't these methods be commented? Done http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@73 PS5, Line 73: > nit: a Done http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@77 PS5, Line 77: > Wouldn't it be easier to change txn_id to an out parameter that is automati Yeah, that's a fair concern. I was hesitant to do so because I was thinking that the TxnStatusManager would one day be sharded via hash partitioning too, making the returning of the next available transaction ID more complex. I was thinking that instead, the TxnManager (or a low number of TxnManagers) would keep track of the next highest transaction ID across the table, and attempt using the next available transaction ID, which might end up in a different hash of the transaction status table. Given this table is expected to eventually be partitioned, starting out with a transaction ID at least helps us identify what tablet to send requests to, so for now it's at least serves that purpose. I'll leave a TODO for using the next-highest transaction ID in the partition though. http://gerrit.cloudera.org:8080/#/c/16044/5/src/kudu/transactions/txn_status_manager.h@86 PS5, Line 86: > nit: isn't? 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: 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, 30 Jun 2020 19:10:52 +0000 Gerrit-HasComments: Yes
