[kudu-CR] KUDU-2823 [java client] Support setting dimension for the newly created tablet
Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13875 ) Change subject: KUDU-2823 [java client] Support setting dimension for the newly created tablet .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/13875/2/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/13875/2/src/kudu/master/master.proto@375 PS2, Line 375: required TSInfoPB ts_info = 1; > Sounds good. Sure. :) -- To view, visit http://gerrit.cloudera.org:8080/13875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5fc7797011df578000544147938bd15a6027f4c8 Gerrit-Change-Number: 13875 Gerrit-PatchSet: 5 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Mon, 22 Jul 2019 02:57:42 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2823 [java client] Support setting dimension for the newly created tablet
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13875 to look at the new patch set (#5). Change subject: KUDU-2823 [java client] Support setting dimension for the newly created tablet .. KUDU-2823 [java client] Support setting dimension for the newly created tablet Change-Id: I5fc7797011df578000544147938bd15a6027f4c8 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto 7 files changed, 111 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/13875/5 -- To view, visit http://gerrit.cloudera.org:8080/13875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5fc7797011df578000544147938bd15a6027f4c8 Gerrit-Change-Number: 13875 Gerrit-PatchSet: 5 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13864 to look at the new patch set (#3). Change subject: KUDU-2625: Support per-row error check in prepare stage .. KUDU-2625: Support per-row error check in prepare stage Tablet servers reject the whole batch if there is a row that violates table schema constraints (e.g., presence of null values for non-nullable columns). This behavior is different from the case when errors happen at later stage of 'applying' received write operations (e.g., a duplicate key error). This patch reject only the 'bad' rows instead of the whole batch when checked errors in prepare stage. Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 --- M src/kudu/client/client-test.cc M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/common/schema.h M src/kudu/tablet/row_op.cc M src/kudu/tablet/row_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/transactions/write_transaction.cc M src/kudu/tablet/transactions/write_transaction.h 10 files changed, 323 insertions(+), 114 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/13864/3 -- To view, visit http://gerrit.cloudera.org:8080/13864 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 Gerrit-Change-Number: 13864 Gerrit-PatchSet: 3 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
honeyhexin has posted comments on this change. (
http://gerrit.cloudera.org:8080/13833 )
Change subject: KUDU-2881 Support create/drop range partition by command line
..
Patch Set 6:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/13833/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13833/1//COMMIT_MSG@13
PS1, Line 13: 1. kudu table add_range_partition
: [-lower_bound_type]
[-upper_bound_type]
: 2. kudu table drop_range_partition
: [-lower_bound_type]
[-upper_bound_type]
> Also have you considered how we would want to handle unbounded range partit
For the unbouded table, it will confict with the existing range partition if we
add range partition for it. However, drop range partition is still effective.
The newest patch has considered this situation.
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/common/partition.cc@1242
PS1, Line 1242: chema::GetRangeSchemaCo
> Seems like is actually GetRangeSchemaColumnIndexes
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/integration-tests/ts_itest-base.h
File src/kudu/integration-tests/ts_itest-base.h:
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/integration-tests/ts_itest-base.h@158
PS1, Line 158:
> nit: can you document what this does and what the arguments do? Similar to
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/kudu-admin-test.cc
File src/kudu/tools/kudu-admin-test.cc:
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/kudu-admin-test.cc@2345
PS1, Line 2345:
> There's a decent amount of functionality that isn't tested by this, e.g. ma
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/kudu-admin-test.cc@2378
PS1, Line 2378:
: // Lambda fu
> nit: Can you keep the comments consistent in terms of spacing? I.e.
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/kudu-admin-test.cc@2380
PS1, Line 2380: // insert num_rows from
> nit: For brevity, you can just do:
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@53
PS1, Line 53: #include "kudu/util/jsonreader.h"
> nit: Can you sort this alphabetically?
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@75
PS1, Line 75: using strings::Split;
> This too. Could you sort this alphabetically? It makes it easier to verify
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@477
PS1, Line 477: }
> warning: the parameter 'table' is copied for each invocation but only used
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@484
PS1, Line 484: ;
: RETURN_NOT_OK(reader.ExtractObjectArray(reader.ro
> nit: Could you add an extra space to align these with reader.root()?
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@488
PS1, Line 488:
> nit: reverse the space order
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@494
PS1, Line 494: ition_schema = table->partition_schema();
: vector key_indexes = partit
> nit: Could you align the spacing here?
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@500
PS1, Line 500:values.size()));
: }
> nit: spacing for `const auto&` here too
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@509
PS1, Line 509: chema::INT8: {
: int64_t value;
> nit: Spacing alignment here and below.
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@518
PS1, Line 518:
> nit: maybe pull this out into a constant so we wouldn't have to change it s
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@639
PS1, Line 639: angeLowerBoundArg);
> Should we validate this up-front that it is either "INCLUSIVE_BOUND" or "EX
Done
http://gerrit.cloudera.org:8080/#/c/13833/1/src/kudu/tools/tool_action_table.cc@654
PS1, Line 654:"INCLUSIVE_BOUND",
:"EXCLUSIVE_BOUND"));
: }
:
: client::sp::shared_ptr client;
: client::sp::shared_ptr table;
: RETURN_NOT_OK(CreateKuduClient(context, ));
: RETURN_NOT_OK(client->OpenTable(table_name, ));
: const auto& schema = table->schema();
:
: unique_ptr lower_bound(schema.NewRow());
: unique_ptr upper_bound(schema.NewRow());
:
: RETURN_NOT_OK(ConvertToKuduPartialRow(table,
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#6). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the table is unbounded. If it is an unbounded table, we can't add range partition for it since it will conflict with existing range partition: UNBOUNDED. However, drop_range_partition is still effective. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 606 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/6 -- To view, visit http://gerrit.cloudera.org:8080/13833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 6 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2881 Support create/drop range partition by command line
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Yao Xu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13833 to look at the new patch set (#5). Change subject: KUDU-2881 Support create/drop range partition by command line .. KUDU-2881 Support create/drop range partition by command line Sometimes we need to drop the range partition and then recreate it in order to rewrite in case that the partition was written wrongly. This patch supports to add/drop range partition by command line. The command can be used as: 1. kudu table add_range_partition [-lower_bound_type] [-upper_bound_type] 2. kudu table drop_range_partition [-lower_bound_type] [-upper_bound_type] The parameters of lower_bound and upper_bound can be empty, which means we will use the default value. If both these two parameters are empty, it means the table is unbounded. If it is an unbounded table, we can't add range partition for it since it will conflict with existing range partition: UNBOUNDED. However, drop_range_partition is still effective. The parameters of lower_bound_type and upper_bound_type is optional. If these two parameters are not specified, the default value are INCLUSIVE_BOUND and EXCLUSIVE_BOUND respectively. These two parameters are both case-insensitive. Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c --- M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_table.cc 4 files changed, 606 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13833/5 -- To view, visit http://gerrit.cloudera.org:8080/13833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I647d9c3cf01807bd8ae9f8cf4091568ea6f1161c Gerrit-Change-Number: 13833 Gerrit-PatchSet: 5 Gerrit-Owner: honeyhexin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Yingchun Lai has posted comments on this change. (
http://gerrit.cloudera.org:8080/13864 )
Change subject: KUDU-2625: Support per-row error check in prepare stage
..
Patch Set 2:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/13864/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13864/1//COMMIT_MSG@14
PS1, Line 14: This patch reject only the 'bad' rows instead of the whole batch
when
> +1
Added some test cases in client/client-test.cc
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations-test.cc
File src/kudu/common/row_operations-test.cc:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations-test.cc@554
PS1, Line 554: TEST_F(
> Any particular reason not to rely on compiler's judgment whether to inline
No more reason, I just copy it from common/partial_row.cc. I'll remove it.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations-test.cc@554
PS1, Line 554: TEST_F(
> Agreed, especially for a test function.
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.h
File src/kudu/common/row_operations.h:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.h@20
PS1, Line 20: #include
> nit: drop the empty line
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.h@82
PS1, Line 82: };
> To clarify, "The 'result' member will only be updated the first time this f
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc
File src/kudu/common/row_operations.cc:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@84
PS1, Line 84: DCHECK(!s.ok());
> You could DCHECK(!s.ok()); all callers respect that invariant.
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@395
PS1, Line 395: ust
> Nit: not your fault, but add an end parens here.
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@409
PS1, Line 409:
> The control flow here is tricky. Would it be possible to apply the same rul
Each SetFailureStatusOnce has different following flow, seems SCOPED_CLEANUP is
not suitable here.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@448
PS1, Line 448:
> Given the current semantics of the SetFailureStatusOnce(), I'm not sure it
Because a row has several columns, we have to consume the following cells of
this row (by ReadColumn in L458), otherwise, the following rows of the same
request could not be read normally. And for this cell, it is not set, so we
'continue' here and not to ReadColumn. On the other hand, on L454, there is no
'continue', because the cell is set (a NULL value), we have to read the cell by
ReadColumn.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@454
PS1, Line 454: if (PREDICT_FALSE(client_set_to_null)) {
: op->SetFailureStatusOnce(Status::InvalidArgument("NULL
values not
> Shouldn't we break or continue after this?
The same as above.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@491
PS1, Line 491: "NULL
> Why to continue if SetFailureStatusOnce() is not going to update the status
The same as above.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@500
PS1, Line 500: // No actual column updates specified!
: op->SetFailureStatusOnce(Status::InvalidArgument("No
fields updated, key is",
> Why don't we return after this, to avoid the extra nesting on L504-513?
The same as above.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@521
PS1, Line 521: const ColumnSchema& col =
tablet_schema_->column(tablet_col_idx);
> The same here: why to continue if the op result status is not going to be u
The same, just to consume input stream.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/common/row_operations.cc@572
PS1, Line 572:
> Nit: there's an unnecessary space here.
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.h
File src/kudu/tablet/row_op.h:
PS1:
> It seems changes in this file are unrelated to the essence of this patch, r
Done
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.h@71
PS1, Line 71: DecodedRowOperation decoded_op;
> FWIW, the existing non-underscore naming reflects the fact that this is a s
Reverted, and I will remove the extra '_' for 'orig_result_from_log_'.
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.cc
File src/kudu/tablet/row_op.cc:
http://gerrit.cloudera.org:8080/#/c/13864/1/src/kudu/tablet/row_op.cc@41
PS1, Line 41: validated = true;
> Should add a comment here explaining why we're doing this.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
[kudu-CR] KUDU-2625: Support per-row error check in prepare stage
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13864 to look at the new patch set (#2). Change subject: KUDU-2625: Support per-row error check in prepare stage .. KUDU-2625: Support per-row error check in prepare stage Tablet servers reject the whole batch if there is a row that violates table schema constraints (e.g., presence of null values for non-nullable columns). This behavior is different from the case when errors happen at later stage of 'applying' received write operations (e.g., a duplicate key error). This patch reject only the 'bad' rows instead of the whole batch when checked errors in prepare stage. Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 --- M src/kudu/client/client-test.cc M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/common/schema.h M src/kudu/tablet/row_op.cc M src/kudu/tablet/row_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/transactions/write_transaction.cc M src/kudu/tablet/transactions/write_transaction.h 10 files changed, 323 insertions(+), 114 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/13864/2 -- To view, visit http://gerrit.cloudera.org:8080/13864 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I497fc3d5d1c9cbb0c183997c9adb8f5efeb9c9d0 Gerrit-Change-Number: 13864 Gerrit-PatchSet: 2 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yingchun Lai <405403...@qq.com>
