You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2021/09/01 04:41:04 UTC
[kudu-CR] KUDU-2671 allow a range to have no hash schema
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17825
Change subject: KUDU-2671 allow a range to have no hash schema
......................................................................
KUDU-2671 allow a range to have no hash schema
With this patch, the semantics of an empty hash schema for a range
changes. Now an empty per-range hash schema means no hash bucketing for
the range. That's better than the previous semantics because:
* The new semantics allow for having ranges with no hash bucketing
even if there is non-trivial hash bucketing at the table-wide level.
* The new semantics is less surprising.
Prior to this patch, an empty hash schema for a range meant using the
table-wide hash schema.
This patch updates several test cases to account for the change.
Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
---
M src/kudu/client/client.cc
M src/kudu/client/flex_partitioning_client-test.cc
M src/kudu/client/table_creator-internal.cc
M src/kudu/client/table_creator-internal.h
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/integration-tests/table_locations-itest.cc
9 files changed, 230 insertions(+), 232 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17825/1
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-2671 allow a range to have no hash schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17825
to look at the new patch set (#4).
Change subject: KUDU-2671 allow a range to have no hash schema
......................................................................
KUDU-2671 allow a range to have no hash schema
With this patch, the semantics of an empty hash schema for a range
changes. Now an empty per-range hash schema means no hash bucketing for
the range. Prior to this patch, an empty hash schema for a range meant
using the table-wide hash schema.
The new semantics are better because:
* they allow for having ranges with no hash bucketing even if there is
a non-trivial hash bucketing at the table-wide level
* they are less surprising to a user of the client API
This patch updates several test cases to account for the change and
adds a new test case to cover the new functionality.
Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/flex_partitioning_client-test.cc
M src/kudu/client/table_creator-internal.cc
M src/kudu/client/table_creator-internal.h
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/integration-tests/table_locations-itest.cc
10 files changed, 267 insertions(+), 268 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17825/4
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-2671 allow a range to have no hash schema
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17825 )
Change subject: KUDU-2671 allow a range to have no hash schema
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/client/flex_partitioning_client-test.cc
File src/kudu/client/flex_partitioning_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/client/flex_partitioning_client-test.cc@412
PS2, Line 412: add_hash_partition
nit: add_custom_range_partition()
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Sep 2021 17:50:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671 allow a range to have no hash schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17825 )
Change subject: KUDU-2671 allow a range to have no hash schema
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17825/3/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/17825/3/src/kudu/common/partition.cc@592
PS3, Line 592: const auto& current_range_hash_schema = bound.hash_schema;
> nit: can probably use 'bound.hash_schema' directly rather than using a vari
Done
http://gerrit.cloudera.org:8080/#/c/17825/3/src/kudu/common/partition.cc@654
PS3, Line 654: rtit
> Maybe this is for a separate patch, but I think updating the test with unbo
Indeed -- a new test scenario to cover this case is needed, apparently. I was thinking to add a test scenario in a follow-up patch since it suspect might escalate in the number of changes :)
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 19:52:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671 allow a range to have empty hash schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17825 )
Change subject: KUDU-2671 allow a range to have empty hash schema
......................................................................
KUDU-2671 allow a range to have empty hash schema
With this patch, the semantics of an empty hash schema for a range
changes. Now an empty per-range hash schema means no hash bucketing for
the range. Prior to this patch, an empty hash schema for a range meant
using the table-wide hash schema.
The new semantics are better because:
* they allow for having ranges with no hash bucketing even if there is
a non-trivial hash bucketing at the table-wide level
* they are less surprising to a user of the client API
This patch updates several test cases to account for the change and
adds a new test case to cover the new functionality.
Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Reviewed-on: http://gerrit.cloudera.org:8080/17825
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Mahesh Reddy <mr...@cloudera.com>
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/flex_partitioning_client-test.cc
M src/kudu/client/table_creator-internal.cc
M src/kudu/client/table_creator-internal.h
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/integration-tests/table_locations-itest.cc
10 files changed, 271 insertions(+), 268 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
Mahesh Reddy: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-2671 allow a range to have no hash schema
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17825 )
Change subject: KUDU-2671 allow a range to have no hash schema
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/client/client.cc@884
PS2, Line 884: // Using KuduTableCreator::add_range_partition() assumes the range partition
So to summarize:
1. Use add_range_partition() when the range uses the table wide schema.
2. Use add_custom_range_partition() and add a custom hash schema.
3. Use add_custom_range_partition() and don't add a hash schema to signify empty hash bucketing.
Just looking at the comments from client.h, can we clarify this https://github.com/apache/kudu/blob/master/src/kudu/client/client.h#L1308-L1309?
Specifically, the last part saying 'different from a schema used for a range partition'.
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/common/partition.cc@593
PS2, Line 593: // If current bound's HashSchema is empty, implies use of default
Update comment as well.
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/common/partition.cc@602
PS2, Line 602: int index = current_range_hash_schema.empty() ? -1 : i;
I believe we can get rid of this line and just directly add index to 'partition_idx_to_hash_schema_idx'.
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/common/partition.cc@656
PS2, Line 656: || partition_idx_to_hash_schema_idx[j] == -1
can remove this if change on L602 is successful.
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/common/partition_pruner-test.cc@1207
PS2, Line 1207: A
Comment is incorrect (my mistake), change to B = 0.
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 00:49:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671 allow a range to have no hash schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17825
to look at the new patch set (#2).
Change subject: KUDU-2671 allow a range to have no hash schema
......................................................................
KUDU-2671 allow a range to have no hash schema
With this patch, the semantics of an empty hash schema for a range
changes. Now an empty per-range hash schema means no hash bucketing for
the range. Prior to this patch, an empty hash schema for a range meant
using the table-wide hash schema.
The new semantics are better because:
* they allow for having ranges with no hash bucketing even if there is
non-trivial hash bucketing at the table-wide level
* they are less surprising to a user of the client API
This patch updates several test cases to account for the change and
adds a new test case to cover the new functionality.
Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
---
M src/kudu/client/client.cc
M src/kudu/client/flex_partitioning_client-test.cc
M src/kudu/client/table_creator-internal.cc
M src/kudu/client/table_creator-internal.h
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/integration-tests/table_locations-itest.cc
9 files changed, 230 insertions(+), 232 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17825/2
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-2671 allow a range to have no hash schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17825 )
Change subject: KUDU-2671 allow a range to have no hash schema
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/client/client.cc@884
PS2, Line 884: // Using KuduTableCreator::add_range_partition() assumes the range partition
> So to summarize:
Done
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/client/flex_partitioning_client-test.cc
File src/kudu/client/flex_partitioning_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/client/flex_partitioning_client-test.cc@412
PS2, Line 412: add_hash_partition
> nit: add_custom_range_partition()
add_custom_range_partition() is a method of KuduTableCreator; here the idea was to point that KuduRangePartition::add_hash_partition() hasn't been called on the object returned by CreateRangePartition.
I updated the comment to make it clearer.
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/common/partition.cc@593
PS2, Line 593: // If current bound's HashSchema is empty, implies use of default
> Update comment as well.
Good catch!
Done.
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/common/partition.cc@602
PS2, Line 602: int index = current_range_hash_schema.empty() ? -1 : i;
> I believe we can get rid of this line and just directly add index to 'parti
Ah, indeed. Thanks!
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/common/partition.cc@656
PS2, Line 656: || partition_idx_to_hash_schema_idx[j] == -1
> can remove this if change on L602 is successful.
Done
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/common/partition_pruner-test.cc
File src/kudu/common/partition_pruner-test.cc:
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/common/partition_pruner-test.cc@1207
PS2, Line 1207: A
> Comment is incorrect (my mistake), change to B = 0.
Done
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 04:47:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671 allow a range to have empty hash schema
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17825 )
Change subject: KUDU-2671 allow a range to have empty hash schema
......................................................................
Patch Set 5: Code-Review+1
LGTM as long as test case with no hash bucketing for last range where upper bound is unbounded is included in a follow up patch.
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 23:36:51 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2671 allow a range to have no hash schema
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17825 )
Change subject: KUDU-2671 allow a range to have no hash schema
......................................................................
Patch Set 4: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17825/4/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/17825/4/src/kudu/client/client.h@1251
PS4, Line 1251: Besides,
nit: "Besides" is a bit informal. Perhaps use "Alternatively"?
http://gerrit.cloudera.org:8080/#/c/17825/4/src/kudu/client/client.h@1312
PS4, Line 1312: the table-wide hash
: /// partitioning schema can be set using the
: /// @c KuduTableCreator::add_hash_partitions() method
nit: it seems a bit odd to mention the table-wide hash schema here, given this method _doesn't_ use the table-wide hash schema, and that KuduTableCreator::add_hash_partitions() isn't particularly relevant to this method in general. Maybe instead, mention the intricacies of add_custom_range_partition(), e.g.
- When called with a KuduRangePartition for which KuduRangePartition::add_hash_partitions() hasn't been called, a range with no hash sub-partitioning is created.
- To create a range with the table-wide hash schema, use KuduTableCreator::add_range_partition() instead.
I suppose both of these can be inferred from the comments elsewhere, but it seems nice to have these close to this method in particular, given I suspect folks will look at the table creator APIs before looking at the range partition API.
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/client/flex_partitioning_client-test.cc
File src/kudu/client/flex_partitioning_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/17825/2/src/kudu/client/flex_partitioning_client-test.cc@412
PS2, Line 412: tor::add_hash_part
> add_custom_range_partition() is a method of KuduTableCreator; here the idea
Ah, indeed. I misinterpreted this as, "calling add_custom_range_partition() without adding additional hash partitions means the range doesn't have any hash bucketing".
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 21:23:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671 allow a range to have no hash schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17825 )
Change subject: KUDU-2671 allow a range to have no hash schema
......................................................................
Patch Set 4: Verified+1
unrelated test failure in ShouldGCWals/ParticipantCopyITest.TestCopyParticipantOps/1
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 20:42:13 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2671 allow a range to have empty hash schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17825 )
Change subject: KUDU-2671 allow a range to have empty hash schema
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17825/4/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/17825/4/src/kudu/client/client.h@1251
PS4, Line 1251: Besides,
> nit: "Besides" is a bit informal. Perhaps use "Alternatively"?
Good point. Done.
http://gerrit.cloudera.org:8080/#/c/17825/4/src/kudu/client/client.h@1312
PS4, Line 1312: the table-wide hash
: /// partitioning schema can be set using the
: /// @c KuduTableCreator::add_hash_partitions() method
> nit: it seems a bit odd to mention the table-wide hash schema here, given t
Done
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 22:13:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2671 allow a range to have no hash schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17825
to look at the new patch set (#3).
Change subject: KUDU-2671 allow a range to have no hash schema
......................................................................
KUDU-2671 allow a range to have no hash schema
With this patch, the semantics of an empty hash schema for a range
changes. Now an empty per-range hash schema means no hash bucketing for
the range. Prior to this patch, an empty hash schema for a range meant
using the table-wide hash schema.
The new semantics are better because:
* they allow for having ranges with no hash bucketing even if there is
a non-trivial hash bucketing at the table-wide level
* they are less surprising to a user of the client API
This patch updates several test cases to account for the change and
adds a new test case to cover the new functionality.
Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/flex_partitioning_client-test.cc
M src/kudu/client/table_creator-internal.cc
M src/kudu/client/table_creator-internal.h
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/integration-tests/table_locations-itest.cc
10 files changed, 252 insertions(+), 250 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17825/3
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-2671 allow a range to have empty hash schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17825
to look at the new patch set (#5).
Change subject: KUDU-2671 allow a range to have empty hash schema
......................................................................
KUDU-2671 allow a range to have empty hash schema
With this patch, the semantics of an empty hash schema for a range
changes. Now an empty per-range hash schema means no hash bucketing for
the range. Prior to this patch, an empty hash schema for a range meant
using the table-wide hash schema.
The new semantics are better because:
* they allow for having ranges with no hash bucketing even if there is
a non-trivial hash bucketing at the table-wide level
* they are less surprising to a user of the client API
This patch updates several test cases to account for the change and
adds a new test case to cover the new functionality.
Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/flex_partitioning_client-test.cc
M src/kudu/client/table_creator-internal.cc
M src/kudu/client/table_creator-internal.h
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/integration-tests/table_locations-itest.cc
10 files changed, 271 insertions(+), 268 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17825/5
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-2671 allow a range to have empty hash schema
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17825 )
Change subject: KUDU-2671 allow a range to have empty hash schema
......................................................................
Patch Set 5: Code-Review+2
LGTM, though it's worth getting another look from Mahesh.
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 22:46:29 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2671 allow a range to have no hash schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: KUDU-2671 allow a range to have no hash schema
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-2671 allow a range to have no hash schema
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17825 )
Change subject: KUDU-2671 allow a range to have no hash schema
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17825/3/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/17825/3/src/kudu/common/partition.cc@592
PS3, Line 592: const auto& current_range_hash_schema = bound.hash_schema;
nit: can probably use 'bound.hash_schema' directly rather than using a variable given it's only used in one place now.
http://gerrit.cloudera.org:8080/#/c/17825/3/src/kudu/common/partition.cc@654
PS3, Line 654: else
Maybe this is for a separate patch, but I think updating the test with unbounded ranges so the last range has no hash bucketing would be good for testing purposes.
--
To view, visit http://gerrit.cloudera.org:8080/17825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia43df69ecd7040e285e098fde49d84a7a00d1fbb
Gerrit-Change-Number: 17825
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Sep 2021 08:30:22 +0000
Gerrit-HasComments: Yes