Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool ...................................................................... Patch Set 13: (7 comments) http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/kudu-admin-test.cc@1502 PS13, Line 1502: // Grab list of tablet_ids from any tserver and check the output. Yeah but there's only one tserver. http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/kudu-admin-test.cc@1512 PS13, Line 1512: // Test a couple of error cases. Maybe consolidate the bulk of this into a lambda so it's not so verbose? http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/kudu-admin-test.cc@1598 PS13, Line 1598: // Since there isn't a great alternative way to validate the answer the tool A lambda might help here too. http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc@200 PS13, Line 200: nullptr Nit: /*field=*/ nullptr Below too. http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc@250 PS13, Line 250: case KuduColumnSchema::DECIMAL: If you're going to special-case DECIMAL as a "known" unsupported type, shouldn't you also special-case BOOL, FLOAT, and DOUBLE? Also, is this short-circuit actually necessary? Isn't it impossible to see those types here? And if we do, isn't it a sign that this is a future version of Kudu that supports these types in the primary key and could accept this predicate? http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc@282 PS13, Line 282: "all primary key columns specified but more than one matching tablet?"); Not that this is possible, but perhaps dump all of the matching tablet IDs? http://gerrit.cloudera.org:8080/#/c/11666/13/src/kudu/tools/tool_action_table.cc@355 PS13, Line 355: .AddRequiredParameter({ kKeyArg, What do you think about using AddRequiredVariadicParameter here, so that when using on the command line you don't need to quote the entire array in order for the tool to marshal it as a single argument? -- To view, visit http://gerrit.cloudera.org:8080/11666 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idcdcf10bfe6b9df686e86b7134e8634fc0efaac3 Gerrit-Change-Number: 11666 Gerrit-PatchSet: 13 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 17 Oct 2018 04:03:18 +0000 Gerrit-HasComments: Yes
