kudu-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Attila Bukor (Code Review)" <ger...@cloudera.org>
Subject [kudu-CR] KUDU-2612 p2: introduce transaction status management
Date Tue, 30 Jun 2020 16:54:11 GMT
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 <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 16:54:11 +0000
Gerrit-HasComments: Yes

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