Alexey Serbin 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 me Right. The point is: there aren't clients released which rely on that functionality. In addition, we will introduce a special feature flag to signal that a server supports ranges with custom hash partitions, and the new client will require that flag. So, the new and updated client which uses this new PB convention will be able to tell whether a server supports the new semantics. 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_bou As I can see, tests scenarios in this file do not have coverage for corresponding code paths. However, I added the encoding for the bounds, as pointed out. I guess we can add test scenarios with corresponding coverage later on. 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. Good catch! Done 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_h Mahesh and I discussed this topic offline, and the consensus was that PartitionSchema::CreatePartitions() should be updated to make its signature fit to the current use cases, where the information accumulated during PartitionSchema::FromPB() should be used (e.g., removing `range_hash_schemas` from the list of parameters of the PartitionSchema::CreatePartitions() is one of those things to do). So, I think it makes sense to implement the change that Mahesh suggested here as a part of the refactoring described above. 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 Good point! Done 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 Good point! Done 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 Yep, it's possible, but I don't think doing that in this particular changelist is the best option. I'd rather do so in a separate changelist. -- 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: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Tue, 27 Jul 2021 03:28:07 +0000 Gerrit-HasComments: Yes
