Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17370 )
Change subject: [txns][tools] tool to list transactions ...................................................................... Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/kudu-txns-cli-test.cc File src/kudu/tools/kudu-txns-cli-test.cc: PS5: > nit: maybe, name this file kudu-txn-cli-test.cc? We have other instances wh Done http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/kudu-txns-cli-test.cc@87 PS5, Line 87: awong > Here and below: I'm not sure this will be valid for a pre-commit run. Mayb Nice catch. I'm surprised the pre-commit passed here. http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/tool_action.h File src/kudu/tools/tool_action.h: http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/tool_action.h@338 PS5, Line 338: TxnsMode > nit: why not just TxnMode? Done http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/tool_action_txns.cc File src/kudu/tools/tool_action_txns.cc: PS5: > nit: maybe, name this file tool_action_txn.cc? We have other instances wher Done http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/tool_action_txns.cc@102 PS5, Line 102: commit_ts > I recall there was an idea to add start time for the transaction to see 'li No that has not materialized. Yes that would be more metadata for the txn status table, which I'll work on in a separate patch. http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/tool_action_txns.cc@148 PS5, Line 148: transactions > nit: multi-row transactions ? Done http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/tool_action_txns.cc@151 PS5, Line 151: show_hybrid_timestamps > nit: maybe, just I prefer this pluralized version because "show_hybrid_timestamp" makes it seem like there is a field that we are showing called "hybrid_timestamp", which isn't the case. http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/tool_action_txns.cc@153 PS5, Line 153: "txns" > nit: maybe, just "txn"? It will be well aligned with "kudu table", "kudu t Done http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/tool_action_txns.cc@154 PS5, Line 154: transactions > nit: multi-row transactions ? Done http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/tool_main.cc File src/kudu/tools/tool_main.cc: http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/tool_main.cc@74 PS5, Line 74: BuildTxnsMode > nit: BuildTxnMode()? We have BuildTableMode(), and that works with multipl Done -- To view, visit http://gerrit.cloudera.org:8080/17370 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6136fe8eea7842802c5a84609a0c8e2101f6a693 Gerrit-Change-Number: 17370 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 04 May 2021 06:50:26 +0000 Gerrit-HasComments: Yes
