[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13833 ) Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Reviewed-on: http://gerrit.cloudera.org:8080/13833 Reviewed-by: Andrew Wong Tested-by: Kudu Jenkins --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 752 insertions(+), 0 deletions(-) Approvals: Andrew Wong: Looks good to me, approved Kudu Jenkins: Verified -- 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: merged Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 20 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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 19: Code-Review+2 Thanks for the contribution! :) -- 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: 19 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin Gerrit-Comment-Date: Tue, 30 Jul 2019 03:16:02 + Gerrit-HasComments: No
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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 19: (2 comments) http://gerrit.cloudera.org:8080/#/c/13833/18/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/13833/18/src/kudu/tools/kudu-admin-test.cc@2374 PS18, Line 2374:RETURN_NOT_OK(result); : } > We should probably RETURN_NOT_OK(result) here, since all we know at this po Done http://gerrit.cloudera.org:8080/#/c/13833/18/src/kudu/tools/kudu-admin-test.cc@2573 PS18, Line 2573: > nit: I don't think you need to even define a variable for this. You could j 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: 19 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin Gerrit-Comment-Date: Tue, 30 Jul 2019 03:07:26 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#19). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 752 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/19 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 19 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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 18: Code-Review+1
(2 comments)
A couple minor nits, otherwise, looks good to me!
http://gerrit.cloudera.org:8080/#/c/13833/18/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/13833/18/src/kudu/tools/kudu-admin-test.cc@2374
PS18, Line 2374: }
: RETUR
We should probably RETURN_NOT_OK(result) here, since all we know at this point
is that it's not an IOError.
http://gerrit.cloudera.org:8080/#/c/13833/18/src/kudu/tools/kudu-admin-test.cc@2573
PS18, Line 2573: out_of_range_rows_to_insert
nit: I don't think you need to even define a variable for this. You could just
do:
check_bounds("[100]", "[200]", "INCLUSIVE_BOUND", "EXCLUSIVE_BOUND", 100, 100,
{ 99, 200 });
Same elsewhere.
--
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: 18
Gerrit-Owner: honeyhexin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu
Gerrit-Reviewer: honeyhexin
Gerrit-Comment-Date: Tue, 30 Jul 2019 02:14:57 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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 18:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13833/17/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/13833/17/src/kudu/tools/kudu-admin-test.cc@2372
PS17, Line 2372: return Status::NotFound(Substitute("No range partition
covers the insert value $0", i));
> Also don't need to explicitly close the session.
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: 18
Gerrit-Owner: honeyhexin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu
Gerrit-Reviewer: honeyhexin
Gerrit-Comment-Date: Mon, 29 Jul 2019 21:38:50 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#18). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 771 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/18 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 18 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Adar Dembo 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 17: Code-Review+1 (2 comments) Andrew this looks good to me; would you like to take another look? http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2426 PS15, Line 2426: > As we have discussed offline, keep the logic here. To add context, honeyhexin pointed out that other tests in kudu-admin-test.cc use this style (ASSERT_TRUE on s.ok()), and he'd prefer to be consistent with the rest of the file. http://gerrit.cloudera.org:8080/#/c/13833/17/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/13833/17/src/kudu/tools/kudu-admin-test.cc@2372 PS17, Line 2372: RETURN_NOT_OK(session->Close()); Also don't need to explicitly close the session. -- 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: 17 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin Gerrit-Comment-Date: Mon, 29 Jul 2019 18:23:02 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#17). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 772 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/17 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 17 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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 11: (6 comments) http://gerrit.cloudera.org:8080/#/c/13833/15//COMMIT_MSG Commit Message: PS15: > Could you restore the commit message indentation as it was before? git will Done http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2350 PS15, Line 2350: NO_FATALS(BuildAndStart()); > This function should be declared in an anonymous namespace, or declared sta Done http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2372 PS15, Line 2372: : // At first, the range partition is un > This isn't necessary; what is being flushed here? Done http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2426 PS15, Line 2426: // add or drop range partition. > ASSERT_OK(s) << ... would be more idiomatic. As we have discussed offline, keep the logic here. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2529 PS15, Line 2529: : // Drop the range partition ad > You can combine this into one line. Done http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/tool_action_table.cc@32 PS15, Line 32: #include > warning: #includes are not sorted properly [llvm-include-order] 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: 11 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin Gerrit-Comment-Date: Mon, 29 Jul 2019 08:30:30 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#16). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 773 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/16 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 16 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Adar Dembo 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 15: (6 comments) http://gerrit.cloudera.org:8080/#/c/13833/15//COMMIT_MSG Commit Message: PS15: Could you restore the commit message indentation as it was before? git will automatically indent the message after the commit is merged. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2350 PS15, Line 2350: Status InsertTestRows(const client::sp::shared_ptr& client, This function should be declared in an anonymous namespace, or declared static. It's also indented too much. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2372 PS15, Line 2372: RETURN_NOT_OK(session->Flush()); : RETURN_NOT_OK(session->Close()); This isn't necessary; what is being flushed here? http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2426 PS15, Line 2426: ASSERT_TRUE(s.ok()) << ToolRunInfo(s, stdout, stderr); ASSERT_OK(s) << ... would be more idiomatic. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/kudu-admin-test.cc@2529 PS15, Line 2529: Status s = InsertTestRows(client_, kTestTableName, value, 1); : EXPECT_TRUE(s.IsNotFound()); You can combine this into one line. http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/13833/15/src/kudu/tools/tool_action_table.cc@575 PS15, Line 575: case KuduColumnSchema::BOOL: FALLTHROUGH_INTENDED; : case KuduColumnSchema::FLOAT: FALLTHROUGH_INTENDED; : case KuduColumnSchema::DOUBLE: FALLTHROUGH_INTENDED; : case KuduColumnSchema::DECIMAL: FALLTHROUGH_INTENDED; As I mentioned in my earlier comment here, for "direct chaining" (i.e. cases that don't execute a single line of code), you don't need to add FALLTHROUGH_INTENDED. It's only necessary when the case does something and then falls through. -- 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: 15 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin Gerrit-Comment-Date: Sun, 28 Jul 2019 21:59:45 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#15). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 768 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/15 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 15 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#14). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 773 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/14 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 14 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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 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 errors;
: ElementDeleter drop();
: bool overflowed;
: session->GetPendingErrors(, );
: 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());
:
: unique_ptr lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", 0));
: unique_ptr upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", 1));
:
: unique_ptr
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& 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,
:
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#13). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 771 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/13 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 13 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Adar Dembo 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)
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: lower
upper
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& table,
Could some of TableScanner be reused for this? There's logic there to
deserialize JSON into predicates, which seems quite similar.
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,
),
: 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
Agreed with Andrew, though in this case I wouldn't use FALLTHROUGH_INTENDED,
because we generally only use it when the missing 'break' looks like a mistake.
For example:
switch (foo) {
case A:
DoA();
break;
case B:
DoB();
case C:
default:
DoDefault();
break;
When I read this, I notice that case B does something but doesn't have a
'break', which looks like an error. I also notice that case C chains directly
into the default case, which seems normal. If the intent for case B was to call
DoB() and DoDefault(), FALLTHROUGH_INTENDED is appropriate:
switch (foo) {
case A:
DoA();
break;
case B:
DoB();
FALLTHROUGH_INTENDED;
case C:
default:
DoDefault();
break;
Under the hood, FALLTHROUGH_INTENDED expands to the [[clang::fallthrough]]
attribute, which affects compilation with -Wimplicit-fallthrough. Of note: when
compiling a "direct chain" with -Wimplicit-fallthrough but without
[[clang::fallthrough]], no warning is generated. This further emphasizes that
FALLTHROUGH_INTENDED is only needed for non-obvious chaining. See
https://clang.llvm.org/docs/AttributeReference.html#fallthrough for details.
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@649
PS12, Line 649: if (!boost::iequals(FLAGS_lower_bound_type,
"INCLUSIVE_BOUND") &&
: !boost::iequals(FLAGS_lower_bound_type,
"EXCLUSIVE_BOUND")) {
: return Status::InvalidArgument(
: "wrong type of range lower bound : only
'INCLUSIVE_BOUND' or "
: "'EXCLUSIVE_BOUND' can be received");
: }
: if (!boost::iequals(FLAGS_upper_bound_type,
"INCLUSIVE_BOUND") &&
: !boost::iequals(FLAGS_upper_bound_type,
"EXCLUSIVE_BOUND")) {
: return Status::InvalidArgument(
: "wrong type of range upper bound : only
'INCLUSIVE_BOUND' or "
: "'EXCLUSIVE_BOUND' can be received");
: }
Can use a lambda here to deduplicate.
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.
I'd combine this in with the validation on L649-660. You can imagine a lambda
(or helper function) that returns a Status and writes a
KuduTableCreator::RangePartitionBound OUT parameter.
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@690
PS12, Line 690:
Nit: DCHECK_EQ(PartitionAction::DROP, action);
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@843
PS12, Line 843: lower
upper
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@846
PS12, Line 846: lower
upper
--
To view, visit http://gerrit.cloudera.org:8080/13833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch:
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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 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,
: "[]",
: "[]",
: }, , );
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, ));
: 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, ));
: 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
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.
...
}
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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: Verified+1 Unrelated test failure of ksck_remote-test -- 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 Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin Gerrit-Comment-Date: Thu, 25 Jul 2019 03:57:31 + Gerrit-HasComments: No
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Andrew Wong has removed a vote on this change. Change subject: KUDU-2881 Support create/drop range partition by command line .. Removed Verified-1 by Kudu Jenkins (120) -- 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: deleteVote Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 12 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Andrew Wong has removed a vote on this change. Change subject: KUDU-2881 Support create/drop range partition by command line .. Removed -Verified by Kudu Jenkins (120) -- 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: deleteVote Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 12 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#12). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 889 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/12 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 12 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#11). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 889 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/11 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 11 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#10). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 889 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/10 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 10 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#9). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 888 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/9 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 9 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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 8:
(11 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: Gets the vector containin
> nit: reword this as "Returns a vector containing the column indexes of the
Done
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.h@263
PS7, Line 263: Status GetRangeSchemaColumnIndexes(const Schema& schema,
: std::vector*
range_column_idxs) const;
:
> Is this used anywhere?
This function is useless now. I have delete it.
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: if (idx == Schema::kColumnNotFound) {
> Do we need to worry about what happens if the column ID isn't in the schema
I have add the process in the newest patch: if any of the columns is not in the
key range columns then an InvalidArgument status is returned.
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, TestAddAndDropUnboundedPartition) {
> Could you split this test into multiple test cases that test the following:
Done
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2354
PS7, Line 2354:
> nit: maybe call this kTestTableName so it's clear this is a table name inst
Done
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2370
PS7, Line 2370: turn Status::OK();
: };
:
: // At first, the range partition is unbounded, we can insert
any data into it.
: // We insert 100 row
> nit: for multi-line function calls, we generally use four spaces, per the C
Done
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2404
PS7, Line 2404: master_addr,
> nit: maybe pull this out into its own variable and reuse it everywhere
Done
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: values;
> nit: how about, "an empty row will be used"?
Done
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@653
PS7, Line 653: "'EXCLUSIVE_BOUND' can be received");
: }
: if (!boost::iequals(FLAGS_upper_bo
> nit: We don't need the Substitute() here. Could just be, "wrong type of ran
Done
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@694
PS7, Line 694: per_b
> nit: for readability, rather than passing boolean arguments, we generally t
Done
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@837
PS7, Line 837: kTableRangeLowerBoundArg,
: "String representation of lower
bound of "
> Could you also add that if the parameter is an empty array, the partition w
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: 8
Gerrit-Owner: honeyhexin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu
Gerrit-Reviewer: honeyhexin
Gerrit-Comment-Date: Wed, 24 Jul 2019 14:23:02 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#8). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 889 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/8 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 8 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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?
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()
:.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
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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 7: If the range partition now is unbounded, we can't add range partition for it directly, since it will explict with the existing range partition: UNBOUNDED. However, we can drop the unbounded partition at first and then add for it by the command line. -- 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 Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin Gerrit-Comment-Date: Mon, 22 Jul 2019 07:04:24 + Gerrit-HasComments: No
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#7). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the range partition is unbounded. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 593 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/7 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 7 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Reviewer: honeyhexin
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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
: [-lower_bound_type]
[-upper_bound_type]
: 2. kudu table drop_range_partition
: [-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 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 client;
: client::sp::shared_ptr table;
: RETURN_NOT_OK(CreateKuduClient(context, ));
: RETURN_NOT_OK(client->OpenTable(table_name, ));
: const auto& schema = table->schema();
:
: unique_ptr lower_bound(schema.NewRow());
: unique_ptr upper_bound(schema.NewRow());
:
: RETURN_NOT_OK(ConvertToKuduPartialRow(table,
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#6). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the table is unbounded. If it is an unbounded table, we can't add range partition for it since it will conflict with existing range partition: UNBOUNDED. However, drop_range_partition is still effective. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 606 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/6 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 6 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#5). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the table is unbounded. If it is an unbounded table, we can't add range partition for it since it will conflict with existing range partition: UNBOUNDED. However, drop_range_partition is still effective. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 606 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/5 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 5 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#4). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This change supports to drop/create range partition by command line. The usage is: 1. kudu table create_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/integration-tests/ts_itest-base.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 6 files changed, 444 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/4 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 4 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#3). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This change supports to drop/create range partition by command line. The usage is: 1. kudu table create_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/integration-tests/ts_itest-base.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 6 files changed, 438 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/3 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 3 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#2). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This change supports to drop/create range partition by command line. The usage is: 1. kudu table create_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/integration-tests/ts_itest-base.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 6 files changed, 439 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/2 -- 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: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 2 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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: (1 comment) 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 create_range_partition : [-lower_bound_type] [-upper_bound_type] : 2. kudu table drop_range_partition : [-lower_bound_type] [-upper_bound_type] Also have you considered how we would want to handle unbounded range partitions? -- 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 Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Thu, 11 Jul 2019 03:04:18 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
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& range_column_idxs) const;
> warning: non-const reference parameter 'range_column_idxs', make it const o
Alternatively, how about having this return vector?
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,
: ));
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 = table->schema().Column(key_index);
: const auto
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
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
honeyhexin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13833 Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This change supports to drop/create range partition by command line. The usage is: 1. kudu table create_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/integration-tests/ts_itest-base.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 6 files changed, 412 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/1 -- 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: newchange Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 1 Gerrit-Owner: honeyhexin
