Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11666 )
Change subject: [tools] Add locate row tool ...................................................................... Patch Set 13: (7 comments) New changelist for the stuff I did: http://gerrit.cloudera.org:8080/11715 I'll post a second one for the BOOL, DOUBLE, FLOAT future-proofing. 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. Done 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? Done 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. These two do some different checks at the end and there's only two. 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 Done 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, shou DECIMAL is actually a supported PK type, but it doesn't have a representation in JSON so we'd have to invent one, and it's not straightforward to just use an integer to represent a decimal value because the int would need to be 128-bit and our json reading library doesn't support that. For the other types, yeah, we could future-proof the tool by supporting them here. 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? Done 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 wh Doesn't seem to work. For example, consider kudu table locate_row my_master my_table [ "foo bar" ] The tool will make the variadic args be {[, foo bar, ]}, so joining them together again gives [foo bar], which is bad json. If you escape the quotation marks, you get {[, "foo, bar",]}, which when concatenated again is [foobar], which is wrong. So on the one hand we need to escape quotation marks to handle strings properly, but then it does the wrong thing by breaking at spaces in the literal. -- 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 22:17:13 +0000 Gerrit-HasComments: Yes
