Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18515 )
Change subject: [master] KUDU-2671: Range specific hashing during table alter op. ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/18515/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/18515/4/src/kudu/master/catalog_manager.cc@2609 PS4, Line 2609: PartitionSchema partition_schema; : RETURN_NOT_OK(PartitionSchema::FromPB( : l.data().pb.partition_schema(), schema, &partition_schema)); > Maybe this can be a different patch, but the new field 'ranges_with_hash_sc Indeed -- I'm not sure I see where the corresponding run-time structures are being updated to reflect adding a new range with custom hash schema. http://gerrit.cloudera.org:8080/#/c/18515/4/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/18515/4/src/kudu/master/master-test.cc@952 PS4, Line 952: char * nit: our styling guide dictates that the asterisk should stick to the type http://gerrit.cloudera.org:8080/#/c/18515/4/src/kudu/master/master-test.cc@963 PS4, Line 963: vector<HashSchema> range_hash_schemas{{{{"key"}, 3, 0}}}; nit: add 'const' or maybe even get rid of the variable inlining the value at line 968? http://gerrit.cloudera.org:8080/#/c/18515/4/src/kudu/master/master-test.cc@964 PS4, Line 964: HashSchema table_wide_hash_schema{{{"key"}, 2, 0}}; ditto http://gerrit.cloudera.org:8080/#/c/18515/4/src/kudu/master/master-test.cc@988 PS4, Line 988: auto& nit: const auto& ? http://gerrit.cloudera.org:8080/#/c/18515/4/src/kudu/master/master-test.cc@1008 PS4, Line 1008: LOG(INFO) << "Altering Table"; Does it help in automated testing if keeping this INFO log line? If not, consider removing this after debugging/troubleshooting is complete for this test scenario. http://gerrit.cloudera.org:8080/#/c/18515/4/src/kudu/master/master-test.cc@1011 PS4, Line 1011: ASSERT_STR_CONTAINS(resp.error().status().DebugString(), : "varying number of hash dimensions per range is not yet supported"); > Can you add another test case where a range partition with a custom hash sc +1 -- To view, visit http://gerrit.cloudera.org:8080/18515 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea9e3317d172c9ae76662c44b21fca9a4819930a Gerrit-Change-Number: 18515 Gerrit-PatchSet: 4 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Tue, 24 May 2022 22:20:26 +0000 Gerrit-HasComments: Yes
