You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "honeyhexin (Code Review)" <ge...@cloudera.org> on 2019/07/10 13:44:54 UTC
[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 <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]
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 <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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<int>& range_column_idxs) const;
> warning: non-const reference parameter 'range_column_idxs', make it const o
Alternatively, how about having this return vector<int>?
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,
: &values));
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 &column = table->schema().Column(key_index);
: const auto &c
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 boost::iequals or an equivalent for case sensitivity?
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@654
PS1, Line 654: const string &table_name = FindOrDie(context.required_args, kTableNameArg);
: const string &table_range_lower_bound = FindOrDie(context.required_args, kTableRangeLowerBound);
: const string &table_range_upper_bound = FindOrDie(context.required_args, kTableRangeUpperBound);
:
: 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()));
:
: KuduTableCreator::RangePartitionBound lower_bound_type = FLAGS_lower_bound_type == "INCLUSIVE_BOUND" ?
: KuduTableCreator::INCLUSIVE_BOUND :
: KuduTableCreator::EXCLUSIVE_BOUND;
: KuduTableCreator::RangePartitionBound upper_bound_type = FLAGS_upper_bound_type == "EXCLUSIVE_BOUND" ?
: KuduTableCreator::EXCLUSIVE_BOUND :
: KuduTableCreator::INCLUSIVE_BOUND;
nit: It seems like lot of this can be encapsulated in its own function(s) and reused from DropRangePartition().
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@813
PS1, Line 813: create
nit: to keep this consistent with the rest of the tools, capitalize this.
--
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Thu, 11 Jul 2019 03:03:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Mon, 29 Jul 2019 18:23:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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<int>* 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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 14:23:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Mon, 22 Jul 2019 07:04:24 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Sun, 21 Jul 2019 14:30:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <aw...@cloudera.com>
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Thu, 25 Jul 2019 06:19:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Mon, 29 Jul 2019 21:38:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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<KuduTable>& 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, &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
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: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c
Gerrit-Change-Number: 13833
Gerrit-PatchSet: 12
Gerrit-Owner: honeyhexin <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Thu, 25 Jul 2019 19:11:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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<KuduClient>& 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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Sun, 28 Jul 2019 21:59:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Thu, 25 Jul 2019 03:57:31 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <gflags/gflags_declare.h>
> 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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Mon, 29 Jul 2019 08:30:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Tue, 30 Jul 2019 03:07:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <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 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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Thu, 11 Jul 2019 03:04:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
honeyhexin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13833 )
Change subject: KUDU-2881 Support create/drop range partition by command line
......................................................................
Patch Set 13:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2357
PS12, Line 2357: for (int i = start_key; i < num_rows + start_key; ++i) {
: unique_ptr<KuduInsert> insert(table->NewInsert());
: auto* row = insert->mutable_row();
: RETURN_NOT_OK(row->SetInt32("key", i));
: RETURN_NOT_OK(row->SetInt32("int_val", 12345));
: RETURN_NOT_OK(row->SetString("string_val", "hello"));
: Status result = session->Apply(insert.release());
: if (result.IsIOError()) {
: vector<kudu::client::KuduError*> errors;
: ElementDeleter drop(&errors);
: bool overflowed;
: session->GetPendingErrors(&errors, &overflowed);
: EXPECT_FALSE(overflowed);
: EXPECT_EQ(1, errors.size());
:
> This seems pretty similar to insert_test_rows(), defined in the next test.
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2401
PS12, Line 2401: // drop all rows when dropping range partition. After dropping there will
: // be 0 rows left.
: string stdout, stderr;
: Status s = RunKuduTool({
: "table",
: "drop_range_partition",
: master_addr,
: kTableId,
> ASSERT_OK() here?
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2461
PS12, Line 2461: ASSERT_OK(table_creator->table_name(kTestTableName)
> How about we catch any errors here, and "unwrap" the pending errors and ret
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2668
PS12, Line 2668:
: TEST_F(AdminCliTest, TestAddAndDropRangePartitionWithWrongParameters) {
: FLAGS_num_tablet_servers = 1;
: FLAGS_num_replicas = 1;
:
: NO_FATALS(BuildAndStart());
:
: const string& master_addr = cluster_->master()->bound_rpc_addr().ToString();
: const string kTestTableName = "TestTable1";
:
: // Build the schema.
: KuduSchema schema;
: KuduSchemaBuilder builder;
: builder.AddColumn("key")->Type(KuduColumnSchema::INT32)->NotNull();
: builder.SetPrimaryKey({ "key" });
: ASSERT_OK(builder.Build(&schema));
:
: unique_ptr<KuduPartialRow> lower_bound(schema.NewRow());
: ASSERT_OK(lower_bound->SetInt32("key", 0));
: unique_ptr<KuduPartialRow> upper_bound(schema.NewRow());
: ASSERT_OK(upper_bound->SetInt32("key", 1));
:
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: ASSERT_OK(table_creator->table_name(kTestTableName)
:
> This same pattern is repeated many times. Maybe wrap this in a lambda like:
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2696
PS12, Line 2696: .Create());
:
: // Lambda function to c
> Is there a reason we're using 1000 rows? Why not 100 like in the other test
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/kudu-admin-test.cc@2790
PS12, Line 2790: ASSERT_OK(lower_bound->SetInt32("key_INT32", 2));
: ASSERT_OK(lower_bou
> nit: could we pass in just the argument, and have this be:
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@98
PS12, Line 98: upper
> upper
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@484
PS12, Line 484: Status ConvertToKuduPartialRow(const client::sp::shared_ptr<KuduTable>& table,
> Could some of TableScanner be reused for this? There's logic there to deser
As we have discussed offline , keep this logic here.
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@592
PS12, Line 592:
: Status ModifyRangePartition(const RunnerContext& context, PartitionAction action) {
: const string& table_name = FindOrDie(context.required_args, kTableNameArg);
: const string& table_range_lower_bound = FindOrDie(context.required_args,
: kTableRangeLowerBoundArg);
: const string& table_range_upper_bound = FindOrDie(context.required_args,
: kTableRangeUpperBoundArg);
:
: const auto check_bounds = [&] (const string& range_bound,
: const string& FLAGS_range_bound_type,
: K
> Agreed with Andrew, though in this case I wouldn't use FALLTHROUGH_INTENDED
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@649
PS12, Line 649: upper_bound_type)->Alter();
: }
:
: Status DropRangePartition(const RunnerContext& context) {
: return ModifyRangePartition(context, PartitionAction::DROP);
: }
:
: Status AddRangePartition(const RunnerContext& context) {
: return ModifyRangePartition(context, PartitionAction::ADD);
: }
:
: } /
> Can use a lambda here to deduplicate.
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@675
PS12, Line 675: .AddRequiredParameter({ kTableNameArg, "Name of the table to describe" })
: .AddOptionalParameter("show_attributes")
: .Build();
:
: unique_ptr<Action> list_tables =
: ActionBuilder("list", &ListTables)
: .Description("List tables")
> I'd combine this in with the validation on L649-660. You can imagine a lamb
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@690
PS12, Line 690: .ExtraDescription("Provide the primary key as a JSON array of primary "
> Nit: DCHECK_EQ(PartitionAction::DROP, action);
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@843
PS12, Line 843:
> upper
Done
http://gerrit.cloudera.org:8080/#/c/13833/12/src/kudu/tools/tool_action_table.cc@846
PS12, Line 846:
> upper
Done
--
To view, visit http://gerrit.cloudera.org:8080/13833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c
Gerrit-Change-Number: 13833
Gerrit-PatchSet: 13
Gerrit-Owner: honeyhexin <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Sun, 28 Jul 2019 07:37:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13833 )
Change subject: KUDU-2881 Support create/drop range partition by command line
......................................................................
Patch Set 7:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.h
File src/kudu/common/partition.h:
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.h@260
PS7, Line 260: Returns the vector stores
nit: reword this as "Returns a vector containing the column indexes of the range partition keys."
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.h@263
PS7, Line 263: // Get whether the range rows are empty, returns true if all of the columns in the
: // range partition key are unset in the row.
: bool GetIsRangePartitionKeyEmpty(const KuduPartialRow& row) const;
Is this used anywhere?
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/common/partition.h
File src/kudu/common/partition.h:
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/common/partition.h@260
PS1, Line 260: // Returns the vector stores column indexes of range keys.
> Alternatively, how about having this return vector<int>?
Done
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/common/partition.cc@1245
PS7, Line 1245: range_column_idxs.push_back(schema.find_column_by_id(column_id));
Do we need to worry about what happens if the column ID isn't in the schema?
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2346
PS7, Line 2346: TEST_F(AdminCliTest, TestAddAndDropRangePartition) {
Could you split this test into multiple test cases that test the following:
* Test for adding and removing a partition with (inclusive, inclusive), (inclusive, exclusive), (exclusive, inclusive), (exclusive, exclusive), (inclusive, Inf), (exclusive, Inf), (Inf, inclusive), (Inf, exclusive)
* Test that the tool doesn't work with incorrect inputs
* Test that the tool works against keys of all types
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2354
PS7, Line 2354: kTestCreateAndDropRangePartitionId
nit: maybe call this kTestTableName so it's clear this is a table name instead of a table ID or tablet ID.
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2370
PS7, Line 2370: .schema(&schema)
: .set_range_partition_columns({ "key_range" })
: .add_range_partition(lower_bound.release(), upper_bound.release())
: .num_replicas(FLAGS_num_replicas)
: .Create());
nit: for multi-line function calls, we generally use four spaces, per the C++ Google style guide, same elsewhere.
https://google.github.io/styleguide/cppguide.html#Function_Calls
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/kudu-admin-test.cc@2404
PS7, Line 2404: cluster_->master()->bound_rpc_addr().ToString()
nit: maybe pull this out into its own variable and reuse it everywhere
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@489
PS7, Line 489: we will use default value
nit: how about, "an empty row will be used"?
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@653
PS7, Line 653: Substitute("wrong type of range upper bound : only $0 or $1 can be received",
: "INCLUSIVE_BOUND",
: "EXCLUSIVE_BOUND"
nit: We don't need the Substitute() here. Could just be, "wrong type of range upper bound: only 'INCLUSIVE_BOUND' or EXCLUSIVE_BOUND' are accepted"
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@694
PS7, Line 694: false
nit: for readability, rather than passing boolean arguments, we generally try to pass around enums, even if it's a two-value enum, e.g.
enum PartitionAction {
ADD,
REMOVE,
};
http://gerrit.cloudera.org:8080/#/c/13833/7/src/kudu/tools/tool_action_table.cc@837
PS7, Line 837: "String representation of upper bound of "
: "the table range partition as a JSON array" }
Could you also add that if the parameter is an empty array, the partition will be unbounded?
--
To view, visit http://gerrit.cloudera.org:8080/13833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c
Gerrit-Change-Number: 13833
Gerrit-PatchSet: 7
Gerrit-Owner: honeyhexin <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Tue, 23 Jul 2019 07:53:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Tue, 30 Jul 2019 03:16:02 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
Gerrit-Comment-Date: Tue, 30 Jul 2019 02:14:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: honeyhexin <ho...@sohu.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Posted by "honeyhexin (Code Review)" <ge...@cloudera.org>.
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 <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]
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 <ho...@sohu.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>