Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14273 )
Change subject: add a tool to create table ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/client/value.h File src/kudu/client/value.h: http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/client/value.h@53 PS7, Line 53: static KuduValue* FromGeneral(const std::string& type, const std::string& val); This doesn't need to be part of KuduValue. It should be defined in tool_action_table.cc, because it's only relevant to the kind of encoding used by "kudu table create". http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/common/partial_row.h File src/kudu/common/partial_row.h: http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/common/partial_row.h@112 PS7, Line 112: Status SetValue(const Slice& col_name, const std::string& type, Likewise, this should also be in tool_action_table.cc because it shouldn't be part of the general client API surface. http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool.proto@329 PS7, Line 329: Please document these new messages and their fields. http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool.proto@342 PS7, Line 342: required string table_name = 1; New PBs shouldn't use 'required'. http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool.proto@343 PS7, Line 343: required SchemaPB schema = 2; : optional PartitionSchemaPB partition = 3; As discussed, we shouldn't expose internal PBs to CLI tooling, so that they can evolve independently of the CLI. Please duplicate as needed, removing irrelevant implementation details along the way. http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool_action_table.cc@881 PS7, Line 881: const string& file_name = FindOrDie(context.required_args, kCreateFileNameArg); : int fd = open(file_name.c_str(), O_RDONLY); : if (fd < 0) { : return Status::InvalidArgument(Substitute( : "create table json file open error")); : } As we discussed, let's always retrieve the JSON blob as a CLI argument. If a user wants to pass it via file, they can do that via shell redirection. On a related note, there is some precedence for JSON-based parsing of values/rows. See the range partitioning CLI tools as well as column_set_default (all in this file). Would any of that help here? If in the schema you specified e.g. the range partition bound values as string, would the PB JSON conversion for CreateTablePB leave those JSON snippets intact, allowing you to provide custom handling using JsonReader? Something like this: 1. Deserialize the entire JSON blob to a CreateTablePB. 2. Retrieve the strings for the e.g. default values for each column. Each one is still in a JSON representation. 3. Use ParseValueOfType on each of those strings to convert them from JSON to a KuduValue. http://gerrit.cloudera.org:8080/#/c/14273/7/src/kudu/tools/tool_action_table.cc@887 PS7, Line 887: ControlShellProtocol protocol(ControlShellProtocol::SerializationMode::JSON, : ControlShellProtocol::CloseMode::CLOSE_ON_DESTROY, : fd, STDOUT_FILENO); You shouldn't use this because you're not actually using the control shell. Instead, duplicate whatever functionality out of ReceiveMessage that you need. -- To view, visit http://gerrit.cloudera.org:8080/14273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacd9ae64dc07e3da6efe840e4bbcfaf3df0ecdc7 Gerrit-Change-Number: 14273 Gerrit-PatchSet: 7 Gerrit-Owner: YangSong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong <[email protected]> Gerrit-Comment-Date: Wed, 25 Sep 2019 04:41:04 +0000 Gerrit-HasComments: Yes
