Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/19931 )
Change subject: KUDU-3476: Make replica placement range and table aware ...................................................................... Patch Set 7: (10 comments) This rev addressed minor nits and implemented the change of approach suggested by Alexey to store ranges with their table in case of multiple tables with same ranges. The next rev will include more testing coverage. http://gerrit.cloudera.org:8080/#/c/19931/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19931/6//COMMIT_MSG@7 PS6, Line 7: KUDU-3476 > nit: Could you add the link to design document either in the jira descripti Added to jira. http://gerrit.cloudera.org:8080/#/c/19931/6//COMMIT_MSG@20 PS6, Line 20: > In addition to a unit test, it would be great to add some sort of a more 'i Working on an end to end test now, it'll be as part of the next rev. http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/catalog_manager.cc@375 PS6, Line 375: false > Would it make sense to enable this by default so give this maximum exposure It'll be enabled for appropriate tests. Good point on the latter, I'll keep it disabled for now but will change it to true when the feature is ready to merge. http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/catalog_manager.cc@5024 PS6, Line 5024: if (FLAGS_enable_range_replica_placement) { : Partition partition; : if (tablet_->metadata().state().pb.has_partition()) { : const auto& tablet_partition = tablet_->metadata().state().pb.partition(); : Partition::FromPB(tablet_partition, &partition); : } : range_key_start = partition.begin().range_key(); : VLOG(1) << Substitute("range_key_start is set to $1", > Add some logs here. For when range_key_start and table_id are populated as Done http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/catalog_manager.cc@6099 PS6, Line 6099: dimension = tablet->metadata().state().pb.dimension_label(); : } : if (FLAGS_enable_range_replica_placement) { : Partition partition; : if (tablet->metadata().state().pb.has_partition()) { : const auto& tablet_partition = tablet->metadata().state().pb.partition(); : Partition::FromPB(tablet_partition, &partition); : } > Same comment as above. Done http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/placement_policy-test.cc File src/kudu/master/placement_policy-test.cc: http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/placement_policy-test.cc@68 PS6, Line 68: TabletNumByRangeAndTableMap > Aren't those supposed to be attributed to a table, and only then to a range Yup good point, changed the representation of ranges everywhere to include the table it belongs to in case multiple tables have ranges with the same start keys. http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/placement_policy-test.cc@1040 PS6, Line 1040: const vector<LocationInfo> cluster_info = { > Don't you need to enable enable_range_replica_placement here and at other l Not for these tests, these are unit tests. The flag will need to be set for any test that exercises the code path in catalog manager, so it will be need to be set for the end to end tests. http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/placement_policy-test.cc@1277 PS6, Line 1277: ASSERT_LE(141, stats["ts5"]); > I guess it would be nice to cover at least the following scenarios as well. Along with the end to end test, it'll be a part of the next rev. http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/placement_policy.cc File src/kudu/master/placement_policy.cc: http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/placement_policy.cc@382 PS6, Line 382: const optional<string>& dimension, : const optional<string>& range_key > Here and everywhere range_key_start and table_id is used, you could assert That flag is set in catalog_manager, so I wouldn't have access to that flag here. http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/placement_policy.cc@385 PS6, Line 385: const set<shared_ptr<TSDescriptor>>& excluded) const { : if (range_key_start && table_id) { : TSDescriptorVector ts_choices; : auto choices_size = ts_descs.size() - excluded.size(); : ReservoirSample(ts_descs, choices_size, excluded, rng_, &ts_choices); : DCHECK_EQ(ts_choices.size(), choices_size); : if (ts_choices.size() > 1) { : return PickTabletServer(ts_choices, range_key_start.value(), table_id.value(), rng_); : } : if (ts_choices.size() == 1) { : return ts_choices.front(); : } : return > nit: Fix indentation indent is consistent with other methods, it may look weird because of the indent for the parameters. -- To view, visit http://gerrit.cloudera.org:8080/19931 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9caeb8d5547e946bfeb152a99e1ec034c3fa0a0f Gerrit-Change-Number: 19931 Gerrit-PatchSet: 7 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Ádám Bakai <[email protected]> Gerrit-Comment-Date: Wed, 21 Jun 2023 06:28:15 +0000 Gerrit-HasComments: Yes
