Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage ...................................................................... Patch Set 8: (7 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 descriptio Done 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@156 PS7, Line 156: > nit: ... and have the latest values ... ? Done http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet-test.cc@258 PS7, Line 258: SimpleTransactionsVisitor visitor; > why are we returning instead of asserting? is this also failing the test? This is just to make failure a bit more graceful -- rather than failing in a thread, the macro is collecting the Statuses and checking for errors down below, which should make it a bit nicer, especially if multiple threads fail. http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.h File src/kudu/transactions/txn_status_tablet.h: http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.h@72 PS7, Line 72: > nit: drop? Done http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.h@76 PS7, Line 76: no > nit: not ? Done 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? There may be multiple transaction status tablets in a given tablet server, so we may end up calling the constructor multiple times, creating multiple of the same schema in the process. Putting this in a KuduOnceLambda ensures we only do that work once. > Also, aren't the column ids fixed if the schema is hardcoded? Column IDs and indexes are somewhat fixed, but this is a bit more defensive -- the API we've defined for schemas revolves around calling these methods and I'd rather rely on the API rather than some internal details, even if we don't expect them to change. Also, we're using column indexes here instead of column IDs because lookups by column indexes are trivial. > Seems like unnecessary function calls, but I could be missing something. Yeah, but we're only doing them once, and IMO it makes the call-sites more legible. 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)? Wha Yeah, the scan spec is ordered and only includes TRANSACTION and PARTICIPANT records. See the initialization at L201. -- 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: 8 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 19:51:36 +0000 Gerrit-HasComments: Yes
