Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13833 )
Change subject: KUDU-2881 Support create/drop range partition by command line ...................................................................... Patch Set 1: (17 comments) Thanks for the contribution! Looks good overall, many stylistic nit-picky comments. http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/common/partition.h@260 PS1, Line 260: void GetRangeSchemaColumnIds(const Schema& schema, std::vector<int>& range_column_idxs) const; > warning: non-const reference parameter 'range_column_idxs', make it const o Alternatively, how about having this return vector<int>? http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/common/partition.cc@1242 PS1, Line 1242: GetRangeSchemaColumnIds Seems like is actually GetRangeSchemaColumnIndexes http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/integration-tests/ts_itest-base.h File src/kudu/integration-tests/ts_itest-base.h: http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/integration-tests/ts_itest-base.h@158 PS1, Line 158: void InsertTestRows(client::KuduTable* table, int num_rows, int first_row); nit: can you document what this does and what the arguments do? Similar to the other functions. Should also note any assumptions that a caller should be aware of (e.g. is the schema of 'table' important?) http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/kudu-admin-test.cc@2345 PS1, Line 2345: TEST_F(AdminCliTest, TestCreateAndDropRangePartition) { There's a decent amount of functionality that isn't tested by this, e.g. making sure that all types are covered when parsing, making sure that we get the expected behavior when passing correct or incorrect "exclusive"/"inclusive" arguments. http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/kudu-admin-test.cc@2378 PS1, Line 2378: : //Insert 100 nit: Can you keep the comments consistent in terms of spacing? I.e. // Insert 100 lines... We try to keep the style of comments consistent, following the C++ Google style guide https://google.github.io/styleguide/cppguide.html#Comment_Style http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/kudu-admin-test.cc@2380 PS1, Line 2380: ASSERT_NO_FATAL_FAILURE nit: For brevity, you can just do: NO_FATALS(InsertTestRows(...)); Same in other places. http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@53 PS1, Line 53: #include "kudu/common/partial_row.h" nit: Can you sort this alphabetically? http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@75 PS1, Line 75: using kudu::client::KuduTableCreator; This too. Could you sort this alphabetically? It makes it easier to verify what is present and what isn't. Can you do the same in other files too? http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@484 PS1, Line 484: /*field=*/nullptr, : &values)); nit: Could you add an extra space to align these with reader.root()? http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@488 PS1, Line 488: auto & nit: reverse the space order const auto& partit... http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@494 PS1, Line 494: key_indexes.size(), : values.size())); nit: Could you align the spacing here? http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@500 PS1, Line 500: to &column = table->schema().Column(key_index); : const auto &c nit: spacing for `const auto&` here too http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@509 PS1, Line 509: col_name, : K nit: Spacing alignment here and below. http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@518 PS1, Line 518: "unable to parse value for column '$0' of type $1" nit: maybe pull this out into a constant so we wouldn't have to change it so many places if we wanted to? Also how about showing the value, something like: Substitute("unable to parse value '$0' for column '$1' of type $2", values[i], col_name, type); http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@639 PS1, Line 639: FLAGS_lower_bound_type Should we validate this up-front that it is either "INCLUSIVE_BOUND" or "EXCLUSIVE_BOUND" and raise an error of not? Also, what do you think about using boost::iequals or an equivalent for case sensitivity? http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@654 PS1, Line 654: const string &table_name = FindOrDie(context.required_args, kTableNameArg); : const string &table_range_lower_bound = FindOrDie(context.required_args, kTableRangeLowerBound); : const string &table_range_upper_bound = FindOrDie(context.required_args, kTableRangeUpperBound); : : client::sp::shared_ptr<KuduClient> client; : client::sp::shared_ptr<KuduTable> table; : RETURN_NOT_OK(CreateKuduClient(context, &client)); : RETURN_NOT_OK(client->OpenTable(table_name, &table)); : const auto &schema = table->schema(); : : unique_ptr<KuduPartialRow> lower_bound(schema.NewRow()); : unique_ptr<KuduPartialRow> upper_bound(schema.NewRow()); : : RETURN_NOT_OK(ConvertToKuduPartialRow(table, table_range_lower_bound, lower_bound.get())); : RETURN_NOT_OK(ConvertToKuduPartialRow(table, table_range_upper_bound, upper_bound.get())); : : KuduTableCreator::RangePartitionBound lower_bound_type = FLAGS_lower_bound_type == "INCLUSIVE_BOUND" ? : KuduTableCreator::INCLUSIVE_BOUND : : KuduTableCreator::EXCLUSIVE_BOUND; : KuduTableCreator::RangePartitionBound upper_bound_type = FLAGS_upper_bound_type == "EXCLUSIVE_BOUND" ? : KuduTableCreator::EXCLUSIVE_BOUND : : KuduTableCreator::INCLUSIVE_BOUND; nit: It seems like lot of this can be encapsulated in its own function(s) and reused from DropRangePartition(). http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@813 PS1, Line 813: create nit: to keep this consistent with the rest of the tools, capitalize this. -- To view, visit http://gerrit.cloudera.org:8080/13833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 1 Gerrit-Owner: honeyhexin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu <[email protected]> Gerrit-Comment-Date: Thu, 11 Jul 2019 03:03:03 +0000 Gerrit-HasComments: Yes
