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 4: (11 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. > Right. The point is: there aren't clients released which rely on that func Sounds good, thanks for the clarification. http://gerrit.cloudera.org:8080/#/c/17694/4/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: http://gerrit.cloudera.org:8080/#/c/17694/4/src/kudu/common/partition-test.cc@1142 PS4, Line 1142: n nit: capitalize "not" and add period to end of sentence to be uniform with other comments on these code blocks. http://gerrit.cloudera.org:8080/#/c/17694/4/src/kudu/common/partition-test.cc@1151 PS4, Line 1151: Status::InvalidArgument nit: Status::InvalidArgument() http://gerrit.cloudera.org:8080/#/c/17694/4/src/kudu/common/partition-test.cc@1163 PS4, Line 1163: Status::InvalidArgument nit: Status::InvalidArgument() http://gerrit.cloudera.org:8080/#/c/17694/4/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/17694/4/src/kudu/common/partition.cc@426 PS4, Line 426: if (bounds_with_hash_schemas->empty()) { : return Status::OK(); : } nit: Is this just for readability purposes to make this code more explicit? If this field is empty, the for loop below is skipped anyways. 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: auto* partition_schema_pb = req.mutable_partition_schema(); > As I can see, tests scenarios in this file do not have coverage for corresp Ack http://gerrit.cloudera.org:8080/#/c/17694/2/src/kudu/integration-tests/table_locations-itest.cc@216 PS2, Line 216: hash_schema_pb->set_seed(hash_schema.seed); > Good catch! Ack 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: // 'range_hash_schemas' parameters: update its signature : // to remove the extra parameter and rely on its : // 'ranges_with_hash_schemas_' member field; the path in : // CatalogManager::ApplyAlterPartitioningSteps() involving : // CreatePartitions() should be updated correspondingly. : c > Mahesh and I discussed this topic offline, and the consensus was that Parti Ack 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: for (const auto& hash_schemas : range_hash_schema) { > Good point! Ack http://gerrit.cloudera.org:8080/#/c/17694/2/src/kudu/master/master-test.cc@921 PS2, Line 921: Status s = CreateTable(kTableName, kTableSchema, {}, > Good point! Ack http://gerrit.cloudera.org:8080/#/c/17694/2/src/kudu/master/master-test.cc@1151 PS2, Line 1151: { ColumnSchema("key", INT32), ColumnSchema("int32_val", INT32) }, 1); > Yep, it's possible, but I don't think doing that in this particular changel Ack -- 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: 4 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 18:24:25 +0000 Gerrit-HasComments: Yes
