Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/transactions.proto File src/kudu/transactions/transactions.proto: http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/transactions.proto@28 PS7, Line 28: TODO(awong): this is a bare-bones implementation so far. We'll certai nit: indent or add a new line after the todo, it looks as if the description would be a continuation of the todo http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet-test.cc File src/kudu/transactions/txn_status_tablet-test.cc: http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet-test.cc@258 PS7, Line 258: RET_IF_NOT_OK(status_tablet_->VisitTransactions(&visitor)); why are we returning instead of asserting? is this also failing the test? http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc File src/kudu/transactions/txn_status_tablet.cc: http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc@81 PS7, Line 81: static KuduOnceLambda col_idx_initializer; What's the benefit of using KuduOnceLambda over initializing these in a constructor? Also, aren't the column ids fixed if the schema is hardcoded? Seems like unnecessary function calls, but I could be missing something. http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc@251 PS7, Line 251: if (PREDICT_FALSE(!prev_txn_id || *prev_txn_id != txn_id)) { is the scan result guaranteed to be ordered (transaction, participant)? What if we add other entry types? -- To view, visit http://gerrit.cloudera.org:8080/16043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c Gerrit-Change-Number: 16043 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: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 23 Jun 2020 15:00:16 +0000 Gerrit-HasComments: Yes
