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 6: (18 comments) http://gerrit.cloudera.org:8080/#/c/13833/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13833/1//COMMIT_MSG@13 PS1, Line 13: 1. kudu table add_range_partition <master_addresses> <table_name> : <lower_bound> <upper_bound> [-lower_bound_type] [-upper_bound_type] : 2. kudu table drop_range_partition <master_addresses> <table_name> : <lower_bound> <upper_bound> [-lower_bound_type] [-upper_bound_type] > Also have you considered how we would want to handle unbounded range partit For the unbouded table, it will confict with the existing range partition if we add range partition for it. However, drop range partition is still effective. The newest patch has considered this situation. 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: chema::GetRangeSchemaCo > Seems like is actually GetRangeSchemaColumnIndexes Done 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: > nit: can you document what this does and what the arguments do? Similar to Done 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: > There's a decent amount of functionality that isn't tested by this, e.g. ma Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/kudu-admin-test.cc@2378 PS1, Line 2378: : // Lambda fu > nit: Can you keep the comments consistent in terms of spacing? I.e. Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/kudu-admin-test.cc@2380 PS1, Line 2380: // insert num_rows from > nit: For brevity, you can just do: Done 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/util/jsonreader.h" > nit: Can you sort this alphabetically? Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@75 PS1, Line 75: using strings::Split; > This too. Could you sort this alphabetically? It makes it easier to verify Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@477 PS1, Line 477: } > warning: the parameter 'table' is copied for each invocation but only used Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@484 PS1, Line 484: ; : RETURN_NOT_OK(reader.ExtractObjectArray(reader.ro > nit: Could you add an extra space to align these with reader.root()? Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@488 PS1, Line 488: > nit: reverse the space order Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@494 PS1, Line 494: ition_schema = table->partition_schema(); : vector<int> key_indexes = partit > nit: Could you align the spacing here? Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@500 PS1, Line 500: values.size())); : } > nit: spacing for `const auto&` here too Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@509 PS1, Line 509: chema::INT8: { : int64_t value; > nit: Spacing alignment here and below. Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@518 PS1, Line 518: > nit: maybe pull this out into a constant so we wouldn't have to change it s Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@639 PS1, Line 639: angeLowerBoundArg); > Should we validate this up-front that it is either "INCLUSIVE_BOUND" or "EX Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@654 PS1, Line 654: "INCLUSIVE_BOUND", : "EXCLUSIVE_BOUND")); : } : : 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())); : : const auto& partition_schema = table->partition_schema(); : bool lower_unbounded = partition_schema.GetIsRangePartitionKeyEmpty(*lower_bound); : bool upper_unbounded = partition_schema.GetIsRangePartitionKeyEmpty(*upper_bound); : bool range_unbounded = false; : if (lower_unbounded && upper_unbounded) { : range_unbounded = true; > nit: It seems like lot of this can be encapsulated in its own function(s) a Done http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@813 PS1, Line 813: ramete > nit: to keep this consistent with the rest of the tools, capitalize this. 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: 6 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: Sun, 21 Jul 2019 14:30:39 +0000 Gerrit-HasComments: Yes
