You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org> on 2020/04/25 06:14:18 UTC

[kudu-CR] [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message 

Volodymyr Verovkin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15808


Change subject: [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message
......................................................................

[partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Added RangeBoundsPB protobuf message to describe range partitions with different hash partitioning schemas

Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
---
M src/kudu/client/client.cc
M src/kudu/client/table_alterer-internal.cc
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, 39 insertions(+), 19 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/15808/1
-- 
To view, visit http://gerrit.cloudera.org:8080/15808
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
Gerrit-Change-Number: 15808
Gerrit-PatchSet: 1
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15808

to look at the new patch set (#7).

Change subject: [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message
......................................................................

[partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Added RangeBoundsPB protobuf message to describe range partitions with
different hash partitioning schemas.

Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M src/kudu/client/client.cc
M src/kudu/client/table_alterer-internal.cc
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
9 files changed, 59 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/15808/7
-- 
To view, visit http://gerrit.cloudera.org:8080/15808
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
Gerrit-Change-Number: 15808
Gerrit-PatchSet: 7
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15808

to look at the new patch set (#2).

Change subject: [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message
......................................................................

[partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Added RangeBoundsPB protobuf message to describe range partitions with
different hash partitioning schemas

Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M src/kudu/client/client.cc
M src/kudu/client/table_alterer-internal.cc
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
9 files changed, 59 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/15808/2
-- 
To view, visit http://gerrit.cloudera.org:8080/15808
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
Gerrit-Change-Number: 15808
Gerrit-PatchSet: 2
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has abandoned this change. ( http://gerrit.cloudera.org:8080/15808 )

Change subject: [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message
......................................................................


Abandoned

Patches related to KUDU-2671 have been merged into upstream repo already, and this patch is no longer needed.
-- 
To view, visit http://gerrit.cloudera.org:8080/15808
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
Gerrit-Change-Number: 15808
Gerrit-PatchSet: 7
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>

[kudu-CR] [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15808

to look at the new patch set (#3).

Change subject: [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message
......................................................................

[partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Added RangeBoundsPB protobuf message to describe range partitions with
different hash partitioning schemas

Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M src/kudu/client/client.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/common/partition-test.cc
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
10 files changed, 93 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/15808/3
-- 
To view, visit http://gerrit.cloudera.org:8080/15808
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
Gerrit-Change-Number: 15808
Gerrit-PatchSet: 3
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Volodymyr Verovkin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15808 )

Change subject: [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message
......................................................................


Patch Set 7:

I have moved changes in src/kudu/common/partition.cc and src/kudu/common/partition-test.cc to part 1


-- 
To view, visit http://gerrit.cloudera.org:8080/15808
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
Gerrit-Change-Number: 15808
Gerrit-PatchSet: 7
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 May 2020 19:12:31 +0000
Gerrit-HasComments: No

[kudu-CR] [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15808 )

Change subject: [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message
......................................................................


Patch Set 5:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/15808/5/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
File java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java:

http://gerrit.cloudera.org:8080/#/c/15808/5/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java@271
PS5, Line 271:       RowOperationsPB rowops = new Operation.OperationsEncoder()
nit: prefer camel case for rowOps, per the GSG

https://google.github.io/styleguide/javaguide.html#s5.3-camel-case


http://gerrit.cloudera.org:8080/#/c/15808/5/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:

PS5: 
I think these changes are meant to be a part of the part 1 of this series.


http://gerrit.cloudera.org:8080/#/c/15808/5/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:

PS5: 
Same here.


http://gerrit.cloudera.org:8080/#/c/15808/5/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/15808/5/src/kudu/master/master.proto@481
PS5, Line 481: message RangeBoundsPB
nit: combine lines


http://gerrit.cloudera.org:8080/#/c/15808/5/src/kudu/master/master.proto@489
PS5, Line 489:     repeated uint32 hash_bucket_schemas_per_bound = 2;
             :     // HashBucketSchemaPB are flattened; for example [0],[1] -> 1st bound, [2] - 2nd bound
             :     repeated PartitionSchemaPB.HashBucketSchemaPB hash_bucket_schemas = 3;
nit: would it simplify anything to define a

 message HashBucketSchemasInRangePB {
   repeated PartitionSchemaPB.HashBucketSchemaPB hash_bucket_schemas = 1;
 }

 message RangeBoundsPB {
   required RowOperationsPB bounds = 1;
   repeated HashBucketSchemasInRangePB = 2;
 }

? Also maybe consider calling RangeBoundsPB something like RangeDefinitionPB instead? Since it's more than just the bounds.


http://gerrit.cloudera.org:8080/#/c/15808/5/src/kudu/master/master.proto@653
PS5, Line 653:   message AddRangePartition {
             :     // The lower and upper range bound for
             :     // the range partition to add.
             :     optional RangeBoundsPB range_bounds = 1;
             : 
             :     // The dimension label for the tablet. Used for dimension-specific placement
             :     // of the tablet's replicas.
             :     optional string dimension_label = 2;
             :   }
             :   message DropRangePartition {
             :     // The lower and upper range bound for
             :     // the range partition to drop.
             :     optional RangeBoundsPB range_bounds = 1;
             :   }
I don't think these changes are backwards compatible -- if an older client (which uses RowOperationsPB) were to try to operate on a new cluster (which uses RangeBoundsPB), won't there be a serialization error?

I think we may have to have a separate field for RangeBoundsPB and handle both it and the RowOperationsPB field, or move the `hash_bucket_schemas_per_bound` and `hash_bucket_schemas` fields into these *RangePartition messages. Same with CreateTableRequestPB.



-- 
To view, visit http://gerrit.cloudera.org:8080/15808
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
Gerrit-Change-Number: 15808
Gerrit-PatchSet: 5
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 04 May 2020 21:13:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15808 )

Change subject: [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/15808/1/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/15808/1/src/kudu/master/master.proto@482
PS1, Line 482: {
style nit: move the bracket to the line above like it's in other message definitions in this file.


http://gerrit.cloudera.org:8080/#/c/15808/1/src/kudu/master/master.proto@484
PS1, Line 484:     
style nit: shift here should be 2 spaces


http://gerrit.cloudera.org:8080/#/c/15808/1/src/kudu/master/master.proto@500
PS1, Line 500: RangeBoundsPB
Is this change backwards-compatible?



-- 
To view, visit http://gerrit.cloudera.org:8080/15808
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
Gerrit-Change-Number: 15808
Gerrit-PatchSet: 1
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 29 Apr 2020 06:25:30 +0000
Gerrit-HasComments: Yes

[kudu-CR] [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15808

to look at the new patch set (#4).

Change subject: [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message
......................................................................

[partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Added RangeBoundsPB protobuf message to describe range partitions with
different hash partitioning schemas

Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M src/kudu/client/client.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
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
11 files changed, 109 insertions(+), 48 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/15808/4
-- 
To view, visit http://gerrit.cloudera.org:8080/15808
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
Gerrit-Change-Number: 15808
Gerrit-PatchSet: 4
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15808

to look at the new patch set (#5).

Change subject: [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message
......................................................................

[partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Added RangeBoundsPB protobuf message to describe range partitions with
different hash partitioning schemas.

Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M src/kudu/client/client.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
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
11 files changed, 109 insertions(+), 48 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/15808/5
-- 
To view, visit http://gerrit.cloudera.org:8080/15808
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
Gerrit-Change-Number: 15808
Gerrit-PatchSet: 5
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message

Posted by "Volodymyr Verovkin (Code Review)" <ge...@cloudera.org>.
Volodymyr Verovkin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15808 )

Change subject: [partitioning] KUDU-2671 [part 2] Added RangeBoundsPB protobuf message
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15808/5/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java
File java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java:

http://gerrit.cloudera.org:8080/#/c/15808/5/java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java@271
PS5, Line 271:       RowOperationsPB rowOps = new Operation.OperationsEncoder()
> nit: prefer camel case for rowOps, per the GSG
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/15808
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic407d54970475fb1522b3801f62341d0ff1ccd2c
Gerrit-Change-Number: 15808
Gerrit-PatchSet: 7
Gerrit-Owner: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 May 2020 19:11:01 +0000
Gerrit-HasComments: Yes