Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14306 )
Change subject: add a tool to create table ...................................................................... Patch Set 1: (12 comments) http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@330 PS1, Line 330: message ColumnPB { What about encoding and compression? http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@341 PS1, Line 341: // lower bound value in format of string, include the value. : optional string in_lower_bound = 1; : // lower bound value in format of string, exclude the value. : optional string ex_lower_bound = 2; : // upper bound value in format of string, include the value. : optional string in_upper_bound = 3; : // upper bound value in format of string, exclude the value. : optional string ex_upper_bound = 4; Why not model this as two string values and two enums, like in add_range_partition()? http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@350 PS1, Line 350: required string column = 1; Shouldn't this be repeated, to match set_range_partition_columns()? http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@351 PS1, Line 351: // range split bound. Do we also want to support splits (i.e. range partition key values that define a split point in range partitioning)? Modeled after add_range_partition_split(). http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@368 PS1, Line 368: // create table protobuffer message. The JSON message provided by user Nit: got trailing whitespace here. http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool.proto@371 PS1, Line 371: required string table_name = 1; https://github.com/protocolbuffers/protobuf/issues/2497 explains why we don't use required in new messages. http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: PS1: In general Kudu adheres to the Google C++ Style Guide. Please reformat continuation lines for function calls as per the GSG guidance (https://google.github.io/styleguide/cppguide.html#Function_Calls). http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@877 PS1, Line 877: KuduValue* ParseValueOfType(const std::string& type, Can't we use the existing ParseValueOfType method, with some modifications for DECIMAL? http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@911 PS1, Line 911: Status SetColumnValue(const Slice& col_name, const std::string& type, Why can't we reuse ConvertToKuduPartialRow? http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@1023 PS1, Line 1023: KUDU_CHECK_OK(b.Build(&kudu_schema)); Use RETURN_NOT_OK instead of KUDU_CHECK_OK for cleaner failure behavior. http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@1031 PS1, Line 1031: KuduPartialRow *upper_bound = kudu_schema.NewRow(); Nit: KuduPartialRow* http://gerrit.cloudera.org:8080/#/c/14306/1/src/kudu/tools/tool_action_table.cc@1263 PS1, Line 1263: .AddOptionalParameter("json_file") : .AddOptionalParameter("json_object") This doesn't make sense to me; why not just a single required parameter that is the JSON object? Then the tool can be invoked in one of two ways: 1. kudu table create <master_addresses> "<JSON blob>" 2. kudu table create <master_addresses> "$(cat /path/to/json/file)" -- To view, visit http://gerrit.cloudera.org:8080/14306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0bce4733a504f8ef5f024089a16fe3c6f1e493f1 Gerrit-Change-Number: 14306 Gerrit-PatchSet: 1 Gerrit-Owner: YangSong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 26 Sep 2019 06:22:02 +0000 Gerrit-HasComments: Yes
