Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17694 )
Change subject: KUDU-2671 remove semantically duplicate range_hash_schemas ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/17694/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17694/2//COMMIT_MSG@12 PS2, Line 12: This change doesn't break any compatibility : because the corresponding client side which uses the removed field : hasn't yet been released. Probably a gap in my knowledge but could you further expand on this? You mean the client side changes aren't part of an official release (e.g. 1.15.0, 1.14.0) so compatibility is not broken right? http://gerrit.cloudera.org:8080/#/c/17694/2/src/kudu/integration-tests/table_locations-itest.cc File src/kudu/integration-tests/table_locations-itest.cc: http://gerrit.cloudera.org:8080/#/c/17694/2/src/kudu/integration-tests/table_locations-itest.cc@197 PS2, Line 197: for (const auto& bound : bounds) { Shouldn't we also have an encoder pointing to partition_schema_pb.range_bounds() and add the bounds to them if 'range_hash_schema' is defined? http://gerrit.cloudera.org:8080/#/c/17694/2/src/kudu/integration-tests/table_locations-itest.cc@216 PS2, Line 216: auto* partition_schema_pb = req.mutable_partition_schema(); nit: can remove this line, defined at L202. http://gerrit.cloudera.org:8080/#/c/17694/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17694/2/src/kudu/master/catalog_manager.cc@1865 PS2, Line 1865: for (auto i = 0; i < ps.range_hash_schemas_size(); ++i) { : PartitionSchema::HashBucketSchemas hash_bucket_schemas; : RETURN_NOT_OK(PartitionSchema::ExtractHashBucketSchemasFromPB( : schema, ps.range_hash_schemas(i).hash_schemas(), &hash_bucket_schemas)); : range_hash_schemas.emplace_back(std::move(hash_bucket_schemas)); : } We already decode the per range hash schemas from PartitionSchemaPB.range_hash_schemas in PartitionSchema::FromPB() earlier in this method. Do you think we can just directly use `hash_schemas` from the field `ranges_with_hash_schemas` from the partition_schema object rather than decoding this protobuf field again? http://gerrit.cloudera.org:8080/#/c/17694/2/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/17694/2/src/kudu/master/master-test.cc@630 PS2, Line 630: auto* partition_schema_pb = req.mutable_partition_schema(); nit: can move this to before for loop of bounds, then can point the encoder at `partition_schema_pb` instead of `req.mutable_partition_schema` on L624. http://gerrit.cloudera.org:8080/#/c/17694/2/src/kudu/master/master-test.cc@921 PS2, Line 921: "must specify only primary key columns for hash bucket partition components"); Can we add another range bound in the test to trigger the error "The number of range bounds does not match the number of per range hash schemas"? Just want to keep the coverage of this error, as there is already an explicit test to check for hash schemas on non primary key columns. http://gerrit.cloudera.org:8080/#/c/17694/2/src/kudu/master/master-test.cc@1151 PS2, Line 1151: FLAGS_default_num_replicas = 1; nit: Is this worth adding to the other tests as well where only one replica is enough? -- To view, visit http://gerrit.cloudera.org:8080/17694 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icde3d0b0870fd3a3941fcc91602993ae7ad46266 Gerrit-Change-Number: 17694 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Fri, 23 Jul 2021 19:50:43 +0000 Gerrit-HasComments: Yes
