Ashwani Raina 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 6: (7 comments) This is the first pass. I need some more time to go through the changes completely to be able to give more valuable feedback. Meanwhile, you can address the comments added so far. 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 description or here for posterity. 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 to tests and deployments? I am fine with keeping it disabled as long as there is a plan to enable it for testing, etc. Also, when released to customers, if this is disabled by default, chances are that many of the customers would simply go ahead with old way of placement without knowing about existence of range aware placement capability in the first place. 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(); : table_id = tablet_->metadata().state().pb.table_id(); Add some logs here. For when range_key_start and table_id are populated as well as when there is no value entry added. http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/catalog_manager.cc@6099 PS6, Line 6099: 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(); : table_id = tablet->metadata().state().pb.table_id(); Same comment as above. 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@1040 PS6, Line 1040: TEST_F(PlacementPolicyTest, PlaceTabletReplicasWithRangesExtremeCase) { Don't you need to enable enable_range_replica_placement here and at other location below where new tests are added? 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>& range_key_start, : const optional<string>& table_id, Here and everywhere range_key_start and table_id is used, you could assert that these are null if enable_range_replica_placement is false. http://gerrit.cloudera.org:8080/#/c/19931/6/src/kudu/master/placement_policy.cc@385 PS6, Line 385: 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 nullptr; : } else { nit: Fix indentation -- 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: 6 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: Fri, 16 Jun 2023 21:31:11 +0000 Gerrit-HasComments: Yes
