You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mahesh Reddy (Code Review)" <ge...@cloudera.org> on 2020/12/11 04:20:39 UTC
[kudu-CR] [master] Range specific hashing at table creation time.
Mahesh Reddy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16859
Change subject: [master] Range specific hashing at table creation time.
......................................................................
[master] Range specific hashing at table creation time.
This patch updates CreateTableRequestPB to allow different
hash schemas to be defined per range at table creation time.
This new field is appropriately decoded in catalog_manager.cc.
The changes to kudu/common include some refactoring and
putting functions back into an anonymous namespace.
Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
---
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
6 files changed, 205 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/16859/1
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.cc@451
PS1, Line 451: // The number of ranges before being split should match the size of range_hash_schemas.
> 'bounds_with_hash_schemas' is the resulting number of bounds after the boun
I guess I need to understand what are split rows to better understand what's going on here.
It might be worth adding a comment here.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Dec 2020 22:11:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16859
to look at the new patch set (#4).
Change subject: [master] Range specific hashing at table creation time.
......................................................................
[master] Range specific hashing at table creation time.
This patch updates CreateTableRequestPB to allow different
hash schemas to be defined per range at table creation time.
This new field is appropriately decoded in catalog_manager.cc.
The changes to kudu/common include some refactoring and
putting functions back into an anonymous namespace.
Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
---
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
7 files changed, 384 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/16859/4
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc@1157
PS4, Line 1157: struct PartitionData {
> I see, thanks for clarifying.
The only reason I can think of now is concerning the metadata. Currently, the metadata of the table contains the table wide hash schemas and the metadata for each partition contains the hash buckets, and both the start and end of its partition keys. If we decide to store the per-range schemas in the metadata (which I imagine we probably will?), then a change to either the Partition/PartitionSchema class will be necessary.
http://gerrit.cloudera.org:8080/#/c/16859/5/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/5/src/kudu/common/partition-test.cc@1166
PS5, Line 1166: {
: {0}, string("a1\0\0\0\0c1", 8), string("a2\0\0\0\0c2", 8),
: string("\0\0\0\0" "a1\0\0\0\0c1", 12), string("\0\0\0\0" "a2\0\0\0\0c2", 12)
: },
> nit: the GSG is a bit fuzz here since there are multiple layers of initiali
Done
http://gerrit.cloudera.org:8080/#/c/16859/5/src/kudu/common/partition-test.cc@1218
PS5, Line 1218: if (d1.range_key_start == d2.range_key_start) {
: return d1.partition_key_start < d2.partition_key_start;
: }
: return d1.range_key_start < d2.range_key_start;
> Aren't all of the partition_key_start values unique? Can't we just return d
They're all unique, but if I don't check for the range_key_start then they won't be grouped by their ranges like they should be. For example, all the partitions with hash_buckets = 0 will be returned first since that causes their partition key to be less than the other partition keys.
For this test case, I actually don't need a sorting function since I insert them in order anyways. I just added it in case we want to come back to this at any point and insert them out of order.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Dec 2020 02:01:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc@418
PS2, Line 418: if (!split_rows.empty()) {
: for (const auto& hash_schemas : range_hash_schemas) {
: if (!hash_schemas.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: }
> Currently, the size of 'range_hash_schemas' matches 'range_bounds' through the client.
Isn't this untrue if the user isn't using the new per-range hash schemas feature? Won't the existing range partitioning APIs still work and not add 'range_hash_schema'? I am under the impression the new interface will not support 'split_rows'.
Maybe it'd help to post the client portion of your work. I'm under the assumption that only the following cases are possible:
- The user is using the old API. 'split_rows' and 'range_bounds' are allowed, but not 'range_hash_schemas'
- The user is using the new API. 'split_rows' is not allowed at all, but 'range_hash_schemas' is allowed. If 'range_hash_schemas' is an empty vector, this is equivalent to a vector of empty schemas. An empty schema is shorthand for using the table-wide default.
Are either of these incorrect assumptions? If not, I don't see why we would allow 'split_rows' and 'range_bounds' to be set with a non-empty vector of 'range_hash_schemas', even if every hash schema were empty, given the whole point of disallowing 'split_rows' is that it confounds the mapping of 'range_hash_schemas' to bounds.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Dec 2020 01:45:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc@1157
PS4, Line 1157: struct PartitionData {
> Maybe I'm missing something -- why do you need to access the hash schemas per range from the Partition? It already doesn't have information about column IDs, even without this new feature. Correct me if I'm wrong, but I'm assuming if we have a Partition, we also have a corresponding PartitionSchema, since the Partition itself is about as stripped down as it can be to represent a single partition.
The reason it doesn't have information about column IDs is because PartitionSchema has the relevant information about the hash schemas. With this new feature, PartitionSchema would only contain the table wide hash schemas and not the schemas per each range. And yes, the set of Partitions returned from CreatePartitions() have a corresponding PartitionSchema.
As for why we would need to access the hash schemas per range, I don't have a good answer for that. I just thought about this whole going through previous test cases, so I'll probably have to look into this some more to see if additional fields are even necessary.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Dec 2020 00:49:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16859
to look at the new patch set (#3).
Change subject: [master] Range specific hashing at table creation time.
......................................................................
[master] Range specific hashing at table creation time.
This patch updates CreateTableRequestPB to allow different
hash schemas to be defined per range at table creation time.
This new field is appropriately decoded in catalog_manager.cc.
The changes to kudu/common include some refactoring and
putting functions back into an anonymous namespace.
Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
---
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
6 files changed, 254 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/16859/3
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc@418
PS2, Line 418: if (!split_rows.empty()) {
: for (const auto& hash_schemas : range_hash_schemas) {
: if (!hash_schemas.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: }
Should we be checking here that ranger_hash_schemas is empty if split_rows is non-empty?
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/integration-tests/table_locations-itest.cc@474
PS1, Line 474: EQ(14,
nit: can just use ASSERT_FALSE
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Sat, 12 Dec 2020 00:25:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc@1157
PS4, Line 1157: struct PartitionData {
> Should I refactor other test cases in this test file to include this vector of structs?
I don't feel strongly about it; let's start with the tests in this patch though. If you do decide to update the others, please do so in a separate patch.
> It got me thinking about the fields of the Partition class, since in the PartitionDebugString() method it prints the hash bucket schemas for each partition by accessing the 'hash_bucket_schemas_' field of the PartitionSchema class. Currently, a Partition object has fields for its hash buckets and both the start and end of its partition key. With this new feature and the current design, there is no way to access the entire hash schema(s) per a range. Basically, the missing parts are the columns that are hashed on and the hash seed.
Maybe I'm missing something -- why do you need to access the hash schemas per range from the Partition? It already doesn't have information about column IDs, even without this new feature. Correct me if I'm wrong, but I'm assuming if we have a Partition, we also have a corresponding PartitionSchema, since the Partition itself is about as stripped down as it can be to represent a single partition.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Dec 2020 00:20:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16859
to look at the new patch set (#5).
Change subject: [master] Range specific hashing at table creation time.
......................................................................
[master] Range specific hashing at table creation time.
This patch updates CreateTableRequestPB to allow different
hash schemas to be defined per range at table creation time.
This new field is appropriately decoded in catalog_manager.cc.
The changes to kudu/common include some refactoring and
putting functions back into an anonymous namespace.
Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
---
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
7 files changed, 447 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/16859/5
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] KUDU-2671: Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] KUDU-2671: Range specific hashing at table creation time.
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc@418
PS2, Line 418: if (!range_hash_schemas.empty()) {
: if (!split_rows.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: if (range_bounds.size() != range_hash_schemas.size()) {
:
> Ack
After much thought, I've decided that the best route is what you said above about only these cases being possible:
- The user is using the old API. 'split_rows' and 'range_bounds' are allowed, but not 'range_hash_schemas'
- The user is using the new API. 'split_rows' is not allowed at all, but 'range_hash_schemas' is allowed. If 'range_hash_schemas' is an empty vector, this is equivalent to a vector of empty schemas. An empty schema is shorthand for using the table-wide default.
If the user wants to specify 'split_rows' with 'range_bounds', we should exclude the new API from that. On the other hand, if the user wants to specify some 'range_bounds' using the new API but for other 'range_bounds' they don't want to specify a hash schema, they should still use the new API but just input an empty vector.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 10
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Dec 2020 21:26:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2671: Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: [master] KUDU-2671: Range specific hashing at table creation time.
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 9
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.h
File src/kudu/common/partition.h:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.h@167
PS1, Line 167: FromPBHelper
> For a public function it should be named that suggests what's actually bein
Ack
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.cc@172
PS1, Line 172: hash_bucket
> std::move(hash_bucket)
Ack
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.cc@451
PS1, Line 451: DCHECK_EQ(range_hash_schemas.size(), bounds_with_hash_schemas.size() - split_rows.size());
> I didn't understand this change.
'bounds_with_hash_schemas' is the resulting number of bounds after the bounds are split by split_rows. Example, if we have 3 bounds and 2 valid splits then 'bounds_with_hash_schemas' will have a size of 5. 'range_hash_schema' is only defined per each range within 'range_bounds'.
A use case exists where a user defines both 'split_rows' and 'range_bounds' but 'range_hash_schema' is only populated with empty vectors to indicate that the table wide hash schema will be used per range. Of course, if 'range_hash_schema' is populated with anything but empty vectors with this current use case, a Status::InvalidArgument() would have been returned on Line 421. So if 3 bounds and 2 valid splits are defined, 'range_hash_schema' will only have a size of 3 since its size matches the number of ranges before they're split.
Perhaps a more simple check would be 'DCHECK_EQ(range_hash_schemas.size(), range_bounds.size());'
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/integration-tests/table_locations-itest.cc@205
PS1, Line 205: PartitionSchemaPB_HashBucketSchemaPB*
> Nit: auto*
Ack
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/master/catalog_manager.cc@1716
PS1, Line 1716: PerRangeHashBucketSchemasPB
> auto
Ack
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/master/catalog_manager.cc@1722
PS1, Line 1722: hash_bucket_schemas
> std::move(hash_bucket_schemas)
Ack
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/master/master-test.cc@180
PS1, Line 180: const vector<vector<HashBucketSchema>>& range_hash_schema,
> Nit: This arg can be the last one with a default value to avoid changing ex
Ack
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Dec 2020 20:27:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2671: Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] KUDU-2671: Range specific hashing at table creation time.
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16859/10/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/10/src/kudu/common/partition-test.cc@1148
PS10, Line 1148:
> Now that there's no per-range hash partitioning in this test, does this tes
I meant to delete this since it's no longer relevant, thanks for pointing it out.
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc@418
PS2, Line 418: if (!range_hash_schemas.empty()) {
: if (!split_rows.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: if (range_bounds.size() != range_hash_schemas.size()) {
:
> SGTM, thanks for the update.
Ack
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 11
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Jan 2021 00:22:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2671: Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16859
to look at the new patch set (#10).
Change subject: [master] KUDU-2671: Range specific hashing at table creation time.
......................................................................
[master] KUDU-2671: Range specific hashing at table creation time.
This patch updates CreateTableRequestPB to allow different
hash schemas to be defined per range at table creation time.
This new field is appropriately decoded in catalog_manager.cc.
While this patch handles the logic for creating the correct
partitions, it does not update the metadata for either the
table or tablets. The new per-range schemas will need to be
added to the table metadata in a following patch.
The changes to kudu/common include some refactoring and
putting functions back into an anonymous namespace.
Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
---
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
7 files changed, 387 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/16859/10
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 10
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.h
File src/kudu/common/partition.h:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.h@167
PS1, Line 167: FromPBHelper
For a public function it should be named that suggests what's actually being done.
ExtractHashBucketSchemasFromPB() or such?
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.cc@172
PS1, Line 172: hash_bucket
std::move(hash_bucket)
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.cc@451
PS1, Line 451: DCHECK_EQ(range_hash_schemas.size(), bounds_with_hash_schemas.size() - split_rows.size());
I didn't understand this change.
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/integration-tests/table_locations-itest.cc@205
PS1, Line 205: PartitionSchemaPB_HashBucketSchemaPB*
Nit: auto*
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/master/catalog_manager.cc@1716
PS1, Line 1716: PerRangeHashBucketSchemasPB
auto
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/master/catalog_manager.cc@1722
PS1, Line 1722: hash_bucket_schemas
std::move(hash_bucket_schemas)
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/master/master-test.cc@180
PS1, Line 180: const vector<vector<HashBucketSchema>>& range_hash_schema,
Nit: This arg can be the last one with a default value to avoid changing existing callsites that don't need to be updated.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 11 Dec 2020 19:15:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2671: Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] KUDU-2671: Range specific hashing at table creation time.
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16859/10/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/10/src/kudu/common/partition-test.cc@1148
PS10, Line 1148: (splits, bounds, {},
Now that there's no per-range hash partitioning in this test, does this test add anything given TestCompoundRangeKeyEncoding exists? More tests is generally good, but preferably only if adds test coverage relevant to the patch at hand.
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc@418
PS2, Line 418: if (!range_hash_schemas.empty()) {
: if (!split_rows.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: if (range_bounds.size() != range_hash_schemas.size()) {
:
> After much thought, I've decided that the best route is what you said above
SGTM, thanks for the update.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 10
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Jan 2021 07:20:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 3: Verified+1
The test failure is unrelated.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Dec 2020 08:43:27 +0000
Gerrit-HasComments: No
[kudu-CR] [master] KUDU-2671: Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] KUDU-2671: Range specific hashing at table creation time.
......................................................................
Patch Set 11: Verified+1 Code-Review+2
Test failure was unrelated.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 11
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Jan 2021 02:42:58 +0000
Gerrit-HasComments: No
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16859
to look at the new patch set (#7).
Change subject: [master] Range specific hashing at table creation time.
......................................................................
[master] Range specific hashing at table creation time.
This patch updates CreateTableRequestPB to allow different
hash schemas to be defined per range at table creation time.
This new field is appropriately decoded in catalog_manager.cc.
While this patch handles the logic for creating the correct
partitions, it does not update the metadata for either the
table or tablets. The new per-range schemas will need to be
added to the table metadata in a following patch.
The changes to kudu/common include some refactoring and
putting functions back into an anonymous namespace.
Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
---
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
7 files changed, 447 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/16859/7
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 7
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 3:
(10 comments)
Overall looks good. Mostly nits and some test suggestions.
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc@418
PS2, Line 418: if (!split_rows.empty()) {
: for (const auto& hash_schemas : range_hash_schemas) {
: if (!hash_schemas.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: }
> I meant to say if the user was using the new client API, the size of 'range
Ack
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/common/partition.cc@417
PS3, Line 417:
: if (!split_rows.empty()) {
: for (const auto& hash_schemas : range_hash_schemas) {
: if (!hash_schemas.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: }
: }
:
: if (!range_hash_schemas.empty() && range_bounds.size() != range_hash_schemas.size()) {
: return Status::InvalidArgument("The number of range bounds does not match the number of user"
: "supplied hash schemas.");
: }
Do we have a partition-test case in which split rows is non-empty and range_hash_schemas is also non-empty but contains empty schemas? If not, can you add one?
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/integration-tests/table_locations-itest.cc@216
PS3, Line 216: auto* partition_schema_pb = req.mutable_partition_schema();
Just FYI, this renders req.has_partition_schema() = true
See https://developers.google.com/protocol-buffers/docs/reference/cpp-generated and search for "mutable_"
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/integration-tests/table_locations-itest.cc@459
PS3, Line 459: w}
nit: add a space
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/integration-tests/table_locations-itest.cc@473
PS3, Line 473: user
nit: these are all "user" schemas. "per-range" would be more accurate.
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/integration-tests/table_locations-itest.cc@473
PS3, Line 473: table wide
nit: capitalize "Table-wide"
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/integration-tests/table_locations-itest.cc@491
PS3, Line 491: // Tablets returned in sorted order by partition key range.
: EXPECT_EQ(string("\0\0\0\0" "\0\0\0\0" "a" "\0\0", 11),
: resp.tablet_locations(0).partition().partition_key_start());
: EXPECT_EQ(string("\0\0\0\0" "\0\0\0\1" "a" "\0\0", 11),
: resp.tablet_locations(1).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\0" "c" "\0\0", 7),
: resp.tablet_locations(2).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\0" "e" "\0\0", 7),
: resp.tablet_locations(3).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\1" "\0\0\0\0" "a" "\0\0", 11),
: resp.tablet_locations(4).partition().partition_key_start());
: EXPECT_EQ(string("\0\0\0\1" "\0\0\0\1" "a" "\0\0", 11),
: resp.tablet_locations(5).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\1" "c" "\0\0", 7),
: resp.tablet_locations(6).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\1" "e" "\0\0", 7),
: resp.tablet_locations(7).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\2" "\0\0\0\0" "a" "\0\0", 11),
: resp.tablet_locations(8).partition().partition_key_start());
: EXPECT_EQ(string("\0\0\0\2" "\0\0\0\1" "a" "\0\0", 11),
: resp.tablet_locations(9).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\2" "c" "\0\0", 7),
: resp.tablet_locations(10).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\2" "e" "\0\0", 7),
: resp.tablet_locations(11).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\3" "\0\0\0\0" "a" "\0\0", 11),
: resp.tablet_locations(12).partition().partition_key_start());
: EXPECT_EQ(string("\0\0\0\3" "\0\0\0\1" "a" "\0\0", 11),
: resp.tablet_locations(13).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\3" "c" "\0\0", 7),
: resp.tablet_locations(14).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\3" "e" "\0\0", 7),
: resp.tablet_locations(15).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\4" "c" "\0\0", 7),
: resp.tablet_locations(16).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\4" "e" "\0\0", 7),
: resp.tablet_locations(17).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\5" "c" "\0\0", 7),
: resp.tablet_locations(18).partition().partition_key_start());
nit: it would be easier to read and extend as:
set<string> key_starts({
string(...partitions for a),
...
string(...partitions for c),
...
string(...partitions for e),
...
});
vector<string> sorted_key_starts(key_starts.begin(), key_starts.end());
ASSERT_EQ(key_starts.size(), resp.tablet_locations_size());
for (int i = 0; i < sorted_key_starts.size(); i++) {
EXPECT_EQ(sorted_key_starts[i], resp.tablet_locations(i).partition().partition_key_start());
}
That way it's really obvious that we have the right number of hashes per range, and it's also clear what "correct" ordering is.
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/master/master.proto@507
PS3, Line 507: per user's request
nit: remove?
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/master/master.proto@507
PS3, Line 507: per
nit: for
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/master/master.proto@508
PS3, Line 508: Only populated when user creates range bounds and not split rows, populated in the same order.
nit: we can be more specific and refer to other fields. Also mind filling this out with proper grammar? How about:
"Only populated when 'split_rows_range_bounds' specifies range bounds, and must be empty if any split rows are specified. If set, must have the same number of elements as there are range bounds, and must be in the same order. If empty, 'partition_schema' is assumed for every range bound."
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Dec 2020 08:42:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16859/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16859/7/src/kudu/master/catalog_manager.cc@1722
PS7, Line 1722: }
: }
:
: PartitionSchema::RangeHashSchema range_hash_schemas;
: if (FLAGS_enable_per_range_hash_schemas) {
: for (int i = 0; i < req.range_hash_schemas_size(); i++) {
: PartitionSchema::HashBucketSchemas hash_bucket_schemas;
:
> Since the full functionality isn't complete, and since can lead to weird be
Done
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 8
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Dec 2020 20:39:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2671: Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16859
to look at the new patch set (#9).
Change subject: [master] KUDU-2671: Range specific hashing at table creation time.
......................................................................
[master] KUDU-2671: Range specific hashing at table creation time.
This patch updates CreateTableRequestPB to allow different
hash schemas to be defined per range at table creation time.
This new field is appropriately decoded in catalog_manager.cc.
While this patch handles the logic for creating the correct
partitions, it does not update the metadata for either the
table or tablets. The new per-range schemas will need to be
added to the table metadata in a following patch.
The changes to kudu/common include some refactoring and
putting functions back into an anonymous namespace.
Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
---
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
7 files changed, 459 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/16859/9
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 9
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] KUDU-2671: Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] KUDU-2671: Range specific hashing at table creation time.
......................................................................
Patch Set 9: Verified+1 Code-Review+2
The failed test are known flaky tests. Will let this sit a bit in case others want to do another pass.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 9
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 Dec 2020 00:20:56 +0000
Gerrit-HasComments: No
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc@1157
PS4, Line 1157: struct PartitionData {
> nit: would it be possible to write this in a similar style to TableLocation
Should I refactor other test cases in this test file to include this vector of structs?
Also, while going through previous partition tests I came across 'PartitionDebugString()' which prints its hash schemas and its range. For example,
"HASH (a) PARTITION 0, HASH (b) PARTITION 0, "
RANGE (a, b, c) PARTITION ("a1", "b1", "c1") <= VALUES < ("a2", "b2", "")".
It got me thinking about the fields of the Partition class, since in the PartitionDebugString() method it prints the hash bucket schemas for each partition by accessing the 'hash_bucket_schemas_' field of the PartitionSchema class. Currently, a Partition object has fields for its hash buckets and both the start and end of its partition key. With this new feature and the current design, there is no way to access the entire hash schema(s) per a range. Basically, the missing parts are the columns that are hashed on and the hash seed.
Technically, the partition key holds all the relevant information about the hash and range schema for a single partition. I'll look into this some more, but do you think it's worth adding a field to the Partition class similar to 'hash_bucket_schemas_' of the PartitionSchema class?
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/common/partition.cc@417
PS3, Line 417:
: if (!split_rows.empty()) {
: for (const auto& hash_schemas : range_hash_schemas) {
: if (!hash_schemas.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: }
: }
:
: if (!range_hash_schemas.empty() && range_bounds.size() != range_hash_schemas.size()) {
: return Status::InvalidArgument("The number of range bounds does not match the number of user "
: "supplied hash schemas.");
: }
> If there are unexercised code paths, more coverage couldn't hurt. That said
added another test for this case in table_locations-itest
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/integration-tests/table_locations-itest.cc@482
PS4, Line 482: G
> nit: there doesn't seem to be a point to this extra scope, since we're only
Done
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/integration-tests/table_locations-itest.cc@517
PS4, Line 517: t i = 0; i < resp.tablet_locations_size(); i++) {
: EXPECT_EQ(partition_key_starts[i],
:
> nit: Isn't this the default comparator for strings? Can we get by without i
Done
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Dec 2020 00:00:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/integration-tests/table_locations-itest.cc@442
PS2, Line 442: TEST_F(TableLocationsTest, TestRangeSpecificHashing) {
We chatted about this over a call, but for posterity, it'd be nice to test the table-wide default case as well.
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/master/catalog_manager.cc@1714
PS2, Line 1714: PartitionSchema::HashBucketSchemas hash_bucket_schemas;
: if (j < req.range_hash_schemas_size()) {
: const auto& hash_schemas_pb = req.range_hash_schemas(j++);
: if (!hash_schemas_pb.hash_schemas().empty()) {
: RETURN_NOT_OK(
: PartitionSchema::ExtractHashBucketSchemasFromPB(schema,
: hash_schemas_pb.hash_schemas(),
: &hash_bucket_schemas));
: }
: }
IMO the intermixed parsing of 'range_hash_schemas' and 'ops' makes this pretty confusing, since it opens the door to questions like: did we actually get all of the requested 'range_hash_schemas'? What if 'ops.size()' and 'range_hash_schemas_size()' don't match?
I think it would be less confusing and still correct to do:
for (int i = 0; i < ops.size(); i++) {
switch (op.type) {
... don't change the existing parsing
}
}
// Pull out all of the requested hash bucket schemas.
for (int i = 0; i < req.range_hash_schema_size(); i++) {
HashBucketSchemas hash_bucket_schemas;
RETURN_NOT_OK(Extract(&hash_bucket_schemas)); // hash_bucket_schemas may be empty.
range_hash_schema.emplace_back(std::move(hash_bucket_schemas));
}
if (!range_hash_schema.empty()) {
if (!split_rows.empty()) {
return an error;
}
if (range_bounds.size() != range_hash_schema.size()) {
return an error;
}
}
CreatePartitions(...)
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/master/master.proto@504
PS2, Line 504: optional PartitionSchemaPB partition_schema = 7;
nit: can you also comment how this is used if 'range_hash_schemas' is specified?
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/master/master.proto@505
PS2, Line 505: repeated PerRangeHashBucketSchemasPB range_hash_schemas = 12;
nit: add docs to how this should be populated, its ordering constraints with 'split_rows_rand_bounds', invariants w.r.t split_rows, etc.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Sat, 12 Dec 2020 00:43:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2671: Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] KUDU-2671: Range specific hashing at table creation time.
......................................................................
[master] KUDU-2671: Range specific hashing at table creation time.
This patch updates CreateTableRequestPB to allow different
hash schemas to be defined per range at table creation time.
This new field is appropriately decoded in catalog_manager.cc.
While this patch handles the logic for creating the correct
partitions, it does not update the metadata for either the
table or tablets. The new per-range schemas will need to be
added to the table metadata in a following patch.
The changes to kudu/common include some refactoring and
putting functions back into an anonymous namespace.
Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Reviewed-on: http://gerrit.cloudera.org:8080/16859
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
7 files changed, 254 insertions(+), 47 deletions(-)
Approvals:
Andrew Wong: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 12
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16859
to look at the new patch set (#6).
Change subject: [master] Range specific hashing at table creation time.
......................................................................
[master] Range specific hashing at table creation time.
This patch updates CreateTableRequestPB to allow different
hash schemas to be defined per range at table creation time.
This new field is appropriately decoded in catalog_manager.cc.
The changes to kudu/common include some refactoring and
putting functions back into an anonymous namespace.
Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
---
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
7 files changed, 447 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/16859/6
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] KUDU-2671: Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: [master] KUDU-2671: Range specific hashing at table creation time.
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 11
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 7:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/16859/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16859/6//COMMIT_MSG@12
PS6, Line 12:
> While this appears to create the correct partitions, it looks like the unde
Ack
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc@1157
PS4, Line 1157: struct PartitionData {
> Ah, thanks for calling this out. I looked through the rest of the create ta
Ack
http://gerrit.cloudera.org:8080/#/c/16859/5/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/5/src/kudu/common/partition-test.cc@1218
PS5, Line 1218: if (d1.range_key_start == d2.range_key_start) {
: return d1.partition_key_start < d2.partition_key_start;
: }
: return d1.range_key_start < d2.range_key_start;
> Ah I forgot for a second that the comparator here is for sorting and not fo
Ack
http://gerrit.cloudera.org:8080/#/c/16859/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16859/6/src/kudu/master/catalog_manager.cc@1852
PS6, Line 1852: partition_schema
> We should also pass 'range_hash_schemas' here. As is, looks like we're drop
makes sense, this is the only use of CreateTableInfo as well. One thing I'll have to look out for is that 'range_hash_schema' is not sorted at this point, it's in the order of the bounds added by the user.
http://gerrit.cloudera.org:8080/#/c/16859/6/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/16859/6/src/kudu/master/master.proto@178
PS6, Line 178: // The table's partitioning schema.
: optional PartitionSchemaPB partition_schema = 9;
> Should also include the PerRangeHashBucketSchemasPB here.
Ack
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 7
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Dec 2020 06:53:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 2:
(1 comment)
> Patch Set 2:
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc@418
PS2, Line 418: if (!split_rows.empty()) {
: for (const auto& hash_schemas : range_hash_schemas) {
: if (!hash_schemas.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: }
> > Currently, the size of 'range_hash_schemas' matches 'range_bounds' throug
I meant to say if the user was using the new client API, the size of 'range_hash_schemas' would match the size of 'range_bounds'. If we decide that the user can ONLY use the old API or ONLY use the new API, then I agree with your two assumptions. However, I've been operating under the assumption that the user can use both API's simultaneously. I've been trying to cover the case where both 'split_rows' and 'range_bounds' are defined.
As for your last point, if both 'split_rows' and 'range_bounds' are defined, and 'range_hash_schema' was a vector of empty vectors, then the same default table wide schema would be applied to each bound so there's no ambiguity to 'split_rows' being defined. Ultimately, I will say the implementation would be a lot easier and more readable if we decided that only the two cases you stated were possible, but if we continue the current way this patch is written then more flexibility can be given to the user at the cost of some code readability.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Dec 2020 02:52:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] KUDU-2671: Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16859
to look at the new patch set (#11).
Change subject: [master] KUDU-2671: Range specific hashing at table creation time.
......................................................................
[master] KUDU-2671: Range specific hashing at table creation time.
This patch updates CreateTableRequestPB to allow different
hash schemas to be defined per range at table creation time.
This new field is appropriately decoded in catalog_manager.cc.
While this patch handles the logic for creating the correct
partitions, it does not update the metadata for either the
table or tablets. The new per-range schemas will need to be
added to the table metadata in a following patch.
The changes to kudu/common include some refactoring and
putting functions back into an anonymous namespace.
Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
---
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
7 files changed, 254 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/16859/11
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 11
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc@418
PS2, Line 418: if (!split_rows.empty()) {
: for (const auto& hash_schemas : range_hash_schemas) {
: if (!hash_schemas.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: }
> I'm not sure I follow the second point. If split_rows is non-empty, shouldn
Currently, the size of 'range_hash_schemas' matches 'range_bounds' through the client. So for each range_bound added, 'range_hash_schema' will either add an empty vector or a user defined schema. So if I add an additional check for 'range_hash_schema' to be empty if 'split_rows' is not empty, then with this new interface a user cannot define both 'range_bounds' and 'split_rows' simultaneously.
Even if the user does not wish to define any hash schemas per range_bound, since an empty vector will be added to 'range_hash_schema' for that range, this additional check will fail if we wish to define 'split_rows' as well.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Dec 2020 00:39:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16859/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16859/7/src/kudu/master/catalog_manager.cc@1722
PS7, Line 1722: for (int i = 0; i < req.range_hash_schemas_size(); i++) {
: PartitionSchema::HashBucketSchemas hash_bucket_schemas;
: RETURN_NOT_OK(
: PartitionSchema::ExtractHashBucketSchemasFromPB(schema,
: req.range_hash_schemas(i).hash_schemas(),
: &hash_bucket_schemas));
: range_hash_schemas.emplace_back(std::move(hash_bucket_schemas));
: }
Since the full functionality isn't complete, and since can lead to weird behavior until the feature is complete, let's guard this by an unsafe gflag, like FLAGS_enable_per_range_hash_schemas or somesuch. It'd be pretty bad if a newer client tried to send a request that led to unusable metadata.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 7
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Dec 2020 19:38:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc@1157
PS4, Line 1157: EXPECT_EQ(0, partitions[0].hash_buckets()[0]);
nit: would it be possible to write this in a similar style to TableLocationsTest.TestRangeSpecificHashing? IMO it's much easier to understand and verify correctness.
If you want to continue checking the hash buckets, you could even put the hash key, range keys, and partition key into a struct; form the expected vector of structs; sort that by the partition key start; and then compare.
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/common/partition.cc@417
PS3, Line 417:
: if (!split_rows.empty()) {
: for (const auto& hash_schemas : range_hash_schemas) {
: if (!hash_schemas.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: }
: }
:
: if (!range_hash_schemas.empty() && range_bounds.size() != range_hash_schemas.size()) {
: return Status::InvalidArgument("The number of range bounds does not match the number of user "
: "supplied hash schemas.");
: }
> nope, I can add one. Should I add another one at a higher level (in table_l
If there are unexercised code paths, more coverage couldn't hurt. That said, I expect even more test coverage of these paths when your client patch lands anyway.
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/integration-tests/table_locations-itest.cc@482
PS4, Line 482: {
nit: there doesn't seem to be a point to this extra scope, since we're only sending one 'req'. Other tests use this to avoid naming collisions, which isn't an issue here.
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/integration-tests/table_locations-itest.cc@517
PS4, Line 517: [](const string& s1, const string& s2) {
: return s1.compare(s2) < 0;
: });
nit: Isn't this the default comparator for strings? Can we get by without it?
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Dec 2020 02:24:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16859
to look at the new patch set (#2).
Change subject: [master] Range specific hashing at table creation time.
......................................................................
[master] Range specific hashing at table creation time.
This patch updates CreateTableRequestPB to allow different
hash schemas to be defined per range at table creation time.
This new field is appropriately decoded in catalog_manager.cc.
The changes to kudu/common include some refactoring and
putting functions back into an anonymous namespace.
Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
---
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
6 files changed, 197 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/16859/2
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc@418
PS2, Line 418: if (!split_rows.empty()) {
: for (const auto& hash_schemas : range_hash_schemas) {
: if (!hash_schemas.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: }
> Ultimately, if 'range_hash_schema' is a vector filled with empty vectors it
I'm not sure I follow the second point. If split_rows is non-empty, shouldn't we disallow supplying range_hash_schemas entirely? I'm not suggesting anything beyond an additional check here.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Dec 2020 00:28:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16859
to look at the new patch set (#8).
Change subject: [master] Range specific hashing at table creation time.
......................................................................
[master] Range specific hashing at table creation time.
This patch updates CreateTableRequestPB to allow different
hash schemas to be defined per range at table creation time.
This new field is appropriately decoded in catalog_manager.cc.
While this patch handles the logic for creating the correct
partitions, it does not update the metadata for either the
table or tablets. The new per-range schemas will need to be
added to the table metadata in a following patch.
The changes to kudu/common include some refactoring and
putting functions back into an anonymous namespace.
Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
---
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
7 files changed, 459 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/16859/8
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 8
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 5: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16859/5/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/5/src/kudu/common/partition-test.cc@1166
PS5, Line 1166: {
: {0}, string("a1\0\0\0\0c1", 8), string("a2\0\0\0\0c2", 8),
: string("\0\0\0\0" "a1\0\0\0\0c1", 12), string("\0\0\0\0" "a2\0\0\0\0c2", 12)
: },
nit: the GSG is a bit fuzz here since there are multiple layers of initializer lists, but i'd suggest:
{
{0}, string, string, // four spaces
string, string // four extra from previous line since this is a continuation of the previous line
},
https://google.github.io/styleguide/cppguide.html#Braced_Initializer_List_Format
http://gerrit.cloudera.org:8080/#/c/16859/5/src/kudu/common/partition-test.cc@1218
PS5, Line 1218: if (d1.range_key_start == d2.range_key_start) {
: return d1.partition_key_start < d2.partition_key_start;
: }
: return d1.range_key_start < d2.range_key_start;
Aren't all of the partition_key_start values unique? Can't we just return d1.partition_key_start < d2.partition_key_start?
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Dec 2020 01:24:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc@1157
PS4, Line 1157: struct PartitionData {
> > Maybe I'm missing something -- why do you need to access the hash schemas
I see, thanks for clarifying.
It'd depend on how we intend on using PartitionSchema moving forward. I think it makes sense to continue separating the concerns of Partition and PartitionSchema -- PartitionSchema worries about metadata that's shared between partitions, Partitions are descriptors that rely on a PartitionSchema. Rather than extending the purview of Partition, conceptually I'd prefer we instead either extend PartitionSchema, or even use multiple PartitionSchemas for tables that have per-range schemas, in which each PartitionSchemas would have the same RangeSchema, but correspond to different bounds.
Whether or not the implementation will allow that cleanly is a different question, but for now, I don't think it's worth adding. Unless, of course, you're going to need it very soon.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Dec 2020 01:03:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/common/partition.cc@451
PS1, Line 451: // The number of ranges before being split should match the size of range_hash_schemas.
> I guess I need to understand what are split rows to better understand what'
added a comment in the second patch, lmk if you think it needs further clarification.
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/common/partition.cc@418
PS2, Line 418: if (!split_rows.empty()) {
: for (const auto& hash_schemas : range_hash_schemas) {
: if (!hash_schemas.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: }
> Should we be checking here that ranger_hash_schemas is empty if split_rows
Ultimately, if 'range_hash_schema' is a vector filled with empty vectors it essentially serves the same purpose as 'range_hash_schema' being empty. At that point, the table wide hash schema will be applied to each new range. 'range_hash_schema' doesn't add any new functionality unless one of its vectors is defined and that is where the incompatibility with 'split_rows' comes in.
Now, the reason why I need 'range_hash_schema' to remain the same way is due to the ordering of the bounds and their schemas being passed in. If I omit an entry in 'range_hash_schema' due to its respective bound not having a user defined schema while other bounds have their respective schemas defined, then I won't have a way to match the schemas to the bound. A way around this is to modify the current 'range_bounds' parameter to 'vector<tuple<KuduPartialRow, KuduPartialRow,vector<HashBucketSchema>>>'. This way, the bounds are matched to their respective hash schemas. If you think this is more clear (or using a struct instead of a tuple), I can consider making that change.
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16859/1/src/kudu/integration-tests/table_locations-itest.cc@474
PS1, Line 474: EQ(14,
> nit: can just use ASSERT_FALSE
Done
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/integration-tests/table_locations-itest.cc@442
PS2, Line 442: TEST_F(TableLocationsTest, TestRangeSpecificHashing) {
> We chatted about this over a call, but for posterity, it'd be nice to test
will add this in next iteration.
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/master/catalog_manager.cc@1714
PS2, Line 1714: PartitionSchema::HashBucketSchemas hash_bucket_schemas;
: if (j < req.range_hash_schemas_size()) {
: const auto& hash_schemas_pb = req.range_hash_schemas(j++);
: if (!hash_schemas_pb.hash_schemas().empty()) {
: RETURN_NOT_OK(
: PartitionSchema::ExtractHashBucketSchemasFromPB(schema,
: hash_schemas_pb.hash_schemas(),
: &hash_bucket_schemas));
: }
: }
> IMO the intermixed parsing of 'range_hash_schemas' and 'ops' makes this pre
good catch, will move out 'range_hash_schema' logic out of for loop for 'ops'.
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/master/master.proto@504
PS2, Line 504: optional PartitionSchemaPB partition_schema = 7;
> nit: can you also comment how this is used if 'range_hash_schemas' is speci
Done
http://gerrit.cloudera.org:8080/#/c/16859/2/src/kudu/master/master.proto@505
PS2, Line 505: repeated PerRangeHashBucketSchemasPB range_hash_schemas = 12;
> nit: add docs to how this should be populated, its ordering constraints wit
Done
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Dec 2020 00:11:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 3:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/common/partition.cc@417
PS3, Line 417:
: if (!split_rows.empty()) {
: for (const auto& hash_schemas : range_hash_schemas) {
: if (!hash_schemas.empty()) {
: return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be "
: "populated at the same time.");
: }
: }
: }
:
: if (!range_hash_schemas.empty() && range_bounds.size() != range_hash_schemas.size()) {
: return Status::InvalidArgument("The number of range bounds does not match the number of user"
: "supplied hash schemas.");
: }
> Do we have a partition-test case in which split rows is non-empty and range
nope, I can add one. Should I add another one at a higher level (in table_locations-itest for example) for more test coverage?
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/integration-tests/table_locations-itest.cc@216
PS3, Line 216: auto* partition_schema_pb = req.mutable_partition_schema();
> Just FYI, this renders req.has_partition_schema() = true
yeh I thought about this and had an if check there stating if (!table_hash_schema.empty()). Don't think it should cause any issues given the way it's currently written but I will add it again to be more restrictive.
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/integration-tests/table_locations-itest.cc@459
PS3, Line 459: w}
> nit: add a space
Done
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/integration-tests/table_locations-itest.cc@473
PS3, Line 473: table wide
> nit: capitalize "Table-wide"
Done
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/integration-tests/table_locations-itest.cc@473
PS3, Line 473: user
> nit: these are all "user" schemas. "per-range" would be more accurate.
Done
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/integration-tests/table_locations-itest.cc@491
PS3, Line 491: // Tablets returned in sorted order by partition key range.
: EXPECT_EQ(string("\0\0\0\0" "\0\0\0\0" "a" "\0\0", 11),
: resp.tablet_locations(0).partition().partition_key_start());
: EXPECT_EQ(string("\0\0\0\0" "\0\0\0\1" "a" "\0\0", 11),
: resp.tablet_locations(1).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\0" "c" "\0\0", 7),
: resp.tablet_locations(2).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\0" "e" "\0\0", 7),
: resp.tablet_locations(3).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\1" "\0\0\0\0" "a" "\0\0", 11),
: resp.tablet_locations(4).partition().partition_key_start());
: EXPECT_EQ(string("\0\0\0\1" "\0\0\0\1" "a" "\0\0", 11),
: resp.tablet_locations(5).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\1" "c" "\0\0", 7),
: resp.tablet_locations(6).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\1" "e" "\0\0", 7),
: resp.tablet_locations(7).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\2" "\0\0\0\0" "a" "\0\0", 11),
: resp.tablet_locations(8).partition().partition_key_start());
: EXPECT_EQ(string("\0\0\0\2" "\0\0\0\1" "a" "\0\0", 11),
: resp.tablet_locations(9).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\2" "c" "\0\0", 7),
: resp.tablet_locations(10).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\2" "e" "\0\0", 7),
: resp.tablet_locations(11).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\3" "\0\0\0\0" "a" "\0\0", 11),
: resp.tablet_locations(12).partition().partition_key_start());
: EXPECT_EQ(string("\0\0\0\3" "\0\0\0\1" "a" "\0\0", 11),
: resp.tablet_locations(13).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\3" "c" "\0\0", 7),
: resp.tablet_locations(14).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\3" "e" "\0\0", 7),
: resp.tablet_locations(15).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\4" "c" "\0\0", 7),
: resp.tablet_locations(16).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\4" "e" "\0\0", 7),
: resp.tablet_locations(17).partition().partition_key_start());
:
: EXPECT_EQ(string("\0\0\0\5" "c" "\0\0", 7),
: resp.tablet_locations(18).partition().partition_key_start());
> nit: it would be easier to read and extend as:
Done
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/master/master.proto@507
PS3, Line 507: per user's request
> nit: remove?
Done
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/master/master.proto@507
PS3, Line 507: per
> nit: for
Done
http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/master/master.proto@508
PS3, Line 508: Only populated when user creates range bounds and not split rows, populated in the same order.
> nit: we can be more specific and refer to other fields. Also mind filling t
Done
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 16 Dec 2020 00:34:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master] Range specific hashing at table creation time.
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time.
......................................................................
Patch Set 6:
(5 comments)
I think this patch looks OK given it's scoped to handling CreateTable requests. That said, there are some serious limitations that are worth calling out and addressing in subsequent patches.
http://gerrit.cloudera.org:8080/#/c/16859/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16859/6//COMMIT_MSG@12
PS6, Line 12:
While this appears to create the correct partitions, it looks like the underlying tablets won't get correct partition schemas. I am OK with doing that in the next patch, but it's worth calling that out as a limitation here.
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc@1157
PS4, Line 1157: struct PartitionData {
> The only reason I can think of now is concerning the metadata. Currently, t
Ah, thanks for calling this out. I looked through the rest of the create table path, and it seems like there's a bit more that needs to be done before we're done with the master. I left some comments in relevant code.
The work can be in a later patch, but it's probably also worth mentioning the limitations of this patch in its commit message.
http://gerrit.cloudera.org:8080/#/c/16859/5/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16859/5/src/kudu/common/partition-test.cc@1218
PS5, Line 1218: if (d1.range_key_start == d2.range_key_start) {
: return d1.partition_key_start < d2.partition_key_start;
: }
: return d1.range_key_start < d2.range_key_start;
> They're all unique, but if I don't check for the range_key_start then they
Ah I forgot for a second that the comparator here is for sorting and not for uniqueness in a set :)
http://gerrit.cloudera.org:8080/#/c/16859/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16859/6/src/kudu/master/catalog_manager.cc@1852
PS6, Line 1852: partition_schema
We should also pass 'range_hash_schemas' here. As is, looks like we're dropping per-range metadata on the floor, meaning our PartitionPBs might not make sense. As you mentioned in the other thread, since we don't have more info in our Partition class and PartitionSchema classes, we're missing out on using the correct hash buckets. e.g. if we specify a 10 hashes for a range when the default hash buckets is 1, we'd have a bunch of PartitionPBs that point at buckets [0-9] and fail when we realize that our hash bucket values don't match our partition schema. This would probably show itself if we tried to write to or scan from a table. We'd probably also see this if we were to try examining the tablets on the tablet servers and checking for their partition schemas.
http://gerrit.cloudera.org:8080/#/c/16859/6/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/16859/6/src/kudu/master/master.proto@178
PS6, Line 178: // The table's partitioning schema.
: optional PartitionSchemaPB partition_schema = 9;
Should also include the PerRangeHashBucketSchemasPB here.
--
To view, visit http://gerrit.cloudera.org:8080/16859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d
Gerrit-Change-Number: 16859
Gerrit-PatchSet: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 17 Dec 2020 03:07:20 +0000
Gerrit-HasComments: Yes