honeyhexin 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 13: (14 comments) http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2357 PS12, Line 2357: for (int i = start_key; i < num_rows + start_key; ++i) { : unique_ptr<KuduInsert> insert(table->NewInsert()); : auto* row = insert->mutable_row(); : RETURN_NOT_OK(row->SetInt32("key", i)); : RETURN_NOT_OK(row->SetInt32("int_val", 12345)); : RETURN_NOT_OK(row->SetString("string_val", "hello")); : Status result = session->Apply(insert.release()); : if (result.IsIOError()) { : vector<kudu::client::KuduError*> errors; : ElementDeleter drop(&errors); : bool overflowed; : session->GetPendingErrors(&errors, &overflowed); : EXPECT_FALSE(overflowed); : EXPECT_EQ(1, errors.size()); : > This seems pretty similar to insert_test_rows(), defined in the next test. Done http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2401 PS12, Line 2401: // drop all rows when dropping range partition. After dropping there will : // be 0 rows left. : string stdout, stderr; : Status s = RunKuduTool({ : "table", : "drop_range_partition", : master_addr, : kTableId, > ASSERT_OK() here? Done http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2461 PS12, Line 2461: ASSERT_OK(table_creator->table_name(kTestTableName) > How about we catch any errors here, and "unwrap" the pending errors and ret Done http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2668 PS12, Line 2668: : TEST_F(AdminCliTest, TestAddAndDropRangePartitionWithWrongParameters) { : FLAGS_num_tablet_servers = 1; : FLAGS_num_replicas = 1; : : NO_FATALS(BuildAndStart()); : : const string& master_addr = cluster_->master()->bound_rpc_addr().ToString(); : const string kTestTableName = "TestTable1"; : : // Build the schema. : KuduSchema schema; : KuduSchemaBuilder builder; : builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull(); : builder.SetPrimaryKey({ "key" }); : ASSERT_OK(builder.Build(&schema)); : : unique_ptr<KuduPartialRow> lower_bound(schema.NewRow()); : ASSERT_OK(lower_bound->SetInt32("key", 0)); : unique_ptr<KuduPartialRow> upper_bound(schema.NewRow()); : ASSERT_OK(upper_bound->SetInt32("key", 1)); : : unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator()); : ASSERT_OK(table_creator->table_name(kTestTableName) : > This same pattern is repeated many times. Maybe wrap this in a lambda like: Done http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2696 PS12, Line 2696: .Create()); : : // Lambda function to c > Is there a reason we're using 1000 rows? Why not 100 like in the other test Done http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2790 PS12, Line 2790: ASSERT_OK(lower_bound->SetInt32("key_INT32", 2)); : ASSERT_OK(lower_bou > nit: could we pass in just the argument, and have this be: Done http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@98 PS12, Line 98: upper > upper Done http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@484 PS12, Line 484: Status ConvertToKuduPartialRow(const client::sp::shared_ptr<KuduTable>& table, > Could some of TableScanner be reused for this? There's logic there to deser As we have discussed offline , keep this logic here. http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@592 PS12, Line 592: : Status ModifyRangePartition(const RunnerContext& context, PartitionAction action) { : const string& table_name = FindOrDie(context.required_args, kTableNameArg); : const string& table_range_lower_bound = FindOrDie(context.required_args, : kTableRangeLowerBoundArg); : const string& table_range_upper_bound = FindOrDie(context.required_args, : kTableRangeUpperBoundArg); : : const auto check_bounds = [&] (const string& range_bound, : const string& FLAGS_range_bound_type, : K > Agreed with Andrew, though in this case I wouldn't use FALLTHROUGH_INTENDED Done http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@649 PS12, Line 649: upper_bound_type)->Alter(); : } : : Status DropRangePartition(const RunnerContext& context) { : return ModifyRangePartition(context, PartitionAction::DROP); : } : : Status AddRangePartition(const RunnerContext& context) { : return ModifyRangePartition(context, PartitionAction::ADD); : } : : } / > Can use a lambda here to deduplicate. Done http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@675 PS12, Line 675: .AddRequiredParameter({ kTableNameArg, "Name of the table to describe" }) : .AddOptionalParameter("show_attributes") : .Build(); : : unique_ptr<Action> list_tables = : ActionBuilder("list", &ListTables) : .Description("List tables") > I'd combine this in with the validation on L649-660. You can imagine a lamb Done http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@690 PS12, Line 690: .ExtraDescription("Provide the primary key as a JSON array of primary " > Nit: DCHECK_EQ(PartitionAction::DROP, action); Done http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@843 PS12, Line 843: > upper Done http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@846 PS12, Line 846: > upper Done -- 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: 13 Gerrit-Owner: honeyhexin <honeyhe...@sohu.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu <ocla...@gmail.com> Gerrit-Reviewer: honeyhexin <honeyhe...@sohu.com> Gerrit-Comment-Date: Sun, 28 Jul 2019 07:37:44 +0000 Gerrit-HasComments: Yes
