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 7: (12 comments) http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.h@260 PS7, Line 260: Returns the vector stores nit: reword this as "Returns a vector containing the column indexes of the range partition keys." http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.h@263 PS7, Line 263: // Get whether the range rows are empty, returns true if all of the columns in the : // range partition key are unset in the row. : bool GetIsRangePartitionKeyEmpty(const KuduPartialRow& row) const; Is this used anywhere? 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: // Returns the vector stores column indexes of range keys. > Alternatively, how about having this return vector<int>? Done http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.cc@1245 PS7, Line 1245: range_column_idxs.push_back(schema.find_column_by_id(column_id)); Do we need to worry about what happens if the column ID isn't in the schema? http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2346 PS7, Line 2346: TEST_F(AdminCliTest, TestAddAndDropRangePartition) { Could you split this test into multiple test cases that test the following: * Test for adding and removing a partition with (inclusive, inclusive), (inclusive, exclusive), (exclusive, inclusive), (exclusive, exclusive), (inclusive, Inf), (exclusive, Inf), (Inf, inclusive), (Inf, exclusive) * Test that the tool doesn't work with incorrect inputs * Test that the tool works against keys of all types http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2354 PS7, Line 2354: kTestCreateAndDropRangePartitionId nit: maybe call this kTestTableName so it's clear this is a table name instead of a table ID or tablet ID. http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2370 PS7, Line 2370: .schema(&schema) : .set_range_partition_columns({ "key_range" }) : .add_range_partition(lower_bound.release(), upper_bound.release()) : .num_replicas(FLAGS_num_replicas) : .Create()); nit: for multi-line function calls, we generally use four spaces, per the C++ Google style guide, same elsewhere. https://google.github.io/styleguide/cppguide.html#Function_Calls http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2404 PS7, Line 2404: cluster_->master()->bound_rpc_addr().ToString() nit: maybe pull this out into its own variable and reuse it everywhere http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@489 PS7, Line 489: we will use default value nit: how about, "an empty row will be used"? http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@653 PS7, Line 653: Substitute("wrong type of range upper bound : only $0 or $1 can be received", : "INCLUSIVE_BOUND", : "EXCLUSIVE_BOUND" nit: We don't need the Substitute() here. Could just be, "wrong type of range upper bound: only 'INCLUSIVE_BOUND' or EXCLUSIVE_BOUND' are accepted" http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@694 PS7, Line 694: false nit: for readability, rather than passing boolean arguments, we generally try to pass around enums, even if it's a two-value enum, e.g. enum PartitionAction { ADD, REMOVE, }; http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@837 PS7, Line 837: "String representation of upper bound of " : "the table range partition as a JSON array" } Could you also add that if the parameter is an empty array, the partition will be unbounded? -- 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: 7 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-Reviewer: honeyhexin <[email protected]> Gerrit-Comment-Date: Tue, 23 Jul 2019 07:53:27 +0000 Gerrit-HasComments: Yes
