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 12: (8 comments) This is looking good! Most of my comments are pointing out potential code reuse in the tests. 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: // Lambda function to insert test rows into ktableId, : // the key value is pass by parameter, the other two columns' : // value are specified. : auto insert_test_row = [&] (int32_t value) -> Status { : unique_ptr<KuduInsert> insert(table->NewInsert()); : auto* row = insert->mutable_row(); : RETURN_NOT_OK(row->SetInt32("key", value)); : RETURN_NOT_OK(row->SetInt32("int_val", 12345)); : RETURN_NOT_OK(row->SetString("string_val", "hello")); : auto session = client_->NewSession(); : RETURN_NOT_OK(session->Apply(insert.release())); : RETURN_NOT_OK(session->Flush()); : RETURN_NOT_OK(session->Close()); : return Status::OK(); : }; This seems pretty similar to insert_test_rows(), defined in the next test. How about defining this into its own function in an anonymous namespace, and then reusing it in the next test case too. Something like: Status InsertTestRows(int start_key, int num_rows) { ... } I'd also consider using the same schema in the next test case so we can reuse it without worrying about schema differences, and it shouldn't change the functionality of TestAddAndDropRangePartition. http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2401 PS12, Line 2401: s = RunKuduTool({ : "table", : "add_range_partition", : master_addr, : kTableId, : "[]", : "[]", : }, &stdout, &stderr); ASSERT_OK() here? http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2461 PS12, Line 2461: RETURN_NOT_OK(session->Apply(insert.release())); How about we catch any errors here, and "unwrap" the pending errors and return the first error? Basically, have this function return the first per-row error. That way, we could reuse this function in insert_out_of_range_row(). http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2668 PS12, Line 2668: { : // Test adding (EXCLUSIVE_BOUND, UNBOUNDED) range partition. : : // Adding (0,unbouded), lower range bound is 0, upper range bound is unbounded, : // 0 is exclusive. : ASSERT_OK(add_range_partition_using_CLI("[0]", "[]", "-lower_bound_type=EXCLUSIVE_BOUND", : "-upper_bound_type=EXCLUSIVE_BOUND")); : : // Insert 1000 rows from line_1 to line_1000, now there are 1000 rows, : // data range is: [1-1000]. : ASSERT_OK(client_->OpenTable(kTestTableName, &table)); : ASSERT_OK(insert_test_rows(1, 1000)); : ASSERT_EQ(1000, CountTableRows(table.get())); : : // Test insert out of range rows, which will return error in lambda as we expect. : // Since the upper range bound is unbouded, there is no num out of range as well : // as it greater than 0. : ASSERT_OK(insert_out_of_range_row(0)); : : // Drop range partition of (0,unbouded) by command line, now there are 0 rows left. : ASSERT_OK(drop_range_partition_using_CLI("[0]", "[]", "-lower_bound_type=EXCLUSIVE_BOUND", : "-upper_bound_type=EXCLUSIVE_BOUND")); : ASSERT_OK(client_->OpenTable(kTestTableName, &table)); : ASSERT_EQ(0, CountTableRows(table.get())); : } This same pattern is repeated many times. Maybe wrap this in a lambda like: const auto check_bounds = [&] (const string& lower_bound, const string& upper_bound, const string& lower_bound_type, const string& upper_bound_type, int start_row_to_insert, int num_rows_to_insert, vector<int> out_or_range_rows_to_insert) { // Adds a partition, inserts some rows in that partition, inserts rows outside that partition, drops the partition, and verifies no rows are left. ... } http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2696 PS12, Line 2696: : // Adding (unbouded,1000), lower range bound unbound, upper range bound is 1000, : // 1000 is inclusive. Is there a reason we're using 1000 rows? Why not 100 like in the other tests? Or we could even drop all of these by a factor of 10 (maybe that'd improve the runtime of the test!) http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2790 PS12, Line 2790: lower_bound_type, : upper_bound_type, nit: could we pass in just the argument, and have this be: Substitute("-lower_bound_type=$0", lower_bound_type), Substitute("-upper_bound_type=$0", upper_bound_type), That way, the flag name is defined once. 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@592 PS12, Line 592: case KuduColumnSchema::BOOL: { : bool value; : RETURN_NOT_OK_PREPEND( : reader.ExtractBool(values[i], /*field=*/nullptr, &value), : Substitute(kErrorMsgArg, : values[i], : col_name, : KuduColumnSchema::DataTypeToString(type))); : range_bound_partial_row->SetBool(col_name, value); : break; : } Per https://kudu.apache.org/docs/schema_design.html#primary-keys, boolean primary keys aren't currently supported. Same with floats and doubles. Could you write this as: ... case KuduColumnSchema::BOOL: FALLTHROUGH_INTENDED; case KuduColumnSchema::FLOAT: FALLTHROUGH_INTENDED; case KuduColumnSchema::DOUBLE: FALLTHROUGH_INTENDED; case KuduColumnSchema::DECIMAL: FALLTHROUGH_INTENDED; default: return Status::NotSupported(...); The FALLTHROUGH_INTENDED make it obvious to readers that the fallthrough is intentional, and they will all have the same error. http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@675 PS12, Line 675: boost::iequals(FLAGS_lower_bound_type, "INCLUSIVE_BOUND") ? : KuduTableCreator::INCLUSIVE_BOUND : : KuduTableCreator::EXCLUSIVE_BOUND; : KuduTableCreator::RangePartitionBound upper_bound_type = : boost::iequals(FLAGS_upper_bound_type, "EXCLUSIVE_BOUND") ? : KuduTableCreator::EXCLUSIVE_BOUND : : KuduTableCreator::INCLUSIVE_BOUND; nit: per the style guide, indent at 4 spaces for multi-line statements. -- 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: 12 Gerrit-Owner: honeyhexin <honeyhe...@sohu.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: Thu, 25 Jul 2019 06:19:22 +0000 Gerrit-HasComments: Yes
