kudu-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Wong (Code Review)" <ger...@cloudera.org>
Subject [kudu-CR] KUDU-2612 p2: introduce transaction status management
Date Tue, 30 Jun 2020 19:10:52 GMT
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 <awong@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aserbin@cloudera.com>
Gerrit-Reviewer: Andrew Wong <awong@cloudera.com>
Gerrit-Reviewer: Attila Bukor <abukor@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 30 Jun 2020 19:10:52 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message