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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 04 May 2020 21:13:40 +0000 Gerrit-HasComments: Yes
