Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17370 )
Change subject: [txns][tools] tool to list transactions ...................................................................... Patch Set 5: (9 comments) Took a quick look over the patch. Overall looks good, just a few naming nits so far. 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 where files contains code working with multiple txn objects, but nevertheless they have 'txn', but but 'txns' in there names. 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. Maybe, omit the username from the list of columns in this test scenario? 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? 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 where files contains code working with multiple txn objects, but nevertheless they have 'txn', but but 'txns' in there names. 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 'lingering' transactions. Did it materialize yet? I guess there would be necessary to store an extra column in the txn status table for that, right? http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/tool_action_txns.cc@148 PS5, Line 148: transactions nit: multi-row transactions ? 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 tablet", "kudu master", etc. Those tools work with the multitude of objects as well. http://gerrit.cloudera.org:8080/#/c/17370/5/src/kudu/tools/tool_action_txns.cc@154 PS5, Line 154: transactions nit: multi-row transactions ? 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 multiple tables as well. -- 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: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 04 May 2021 02:30:14 +0000 Gerrit-HasComments: Yes
