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 9: (8 comments) http://gerrit.cloudera.org:8080/#/c/19931/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19931/6//COMMIT_MSG@20 PS6, Line 20: > Working on an end to end test now, it'll be as part of the next rev. Done http://gerrit.cloudera.org:8080/#/c/19931/8/src/kudu/master/placement_policy-test.cc File src/kudu/master/placement_policy-test.cc: http://gerrit.cloudera.org:8080/#/c/19931/8/src/kudu/master/placement_policy-test.cc@1057 PS8, Line 1057: > nit: After reading the descriptive comment above the test, I had to look up Good point, went ahead and did that for all new test cases along with adding a const int for number of tablet servers. http://gerrit.cloudera.org:8080/#/c/19931/8/src/kudu/master/placement_policy-test.cc@1058 PS8, Line 1058: = > nit: and if I understand correctly, here you could assert against the above Done http://gerrit.cloudera.org:8080/#/c/19931/8/src/kudu/master/placement_policy-test.cc@1063 PS8, Line 1063: ++stats[ts_uuid]; > More flaky results("../../build-support/dist_test.py loop -n 1000 -- ./bin/ Addressed below. http://gerrit.cloudera.org:8080/#/c/19931/8/src/kudu/master/placement_policy-test.cc@1145 PS8, Line 1145: : > nit: maybe in such cases where you have multiple ranges you could number al Done http://gerrit.cloudera.org:8080/#/c/19931/8/src/kudu/master/placement_policy-test.cc@1210 PS8, Line 1210: s3"]); : } : } > nit: Done http://gerrit.cloudera.org:8080/#/c/19931/8/src/kudu/master/placement_policy-test.cc@1242 PS8, Line 1242: map<string, int> stats; : for (auto i = 0; i < 1000; ++i) { : TSDescriptorVector result; : ASSERT_OK(policy.PlaceTabletReplicas(nReplicationFactor, nullopt, nullopt, nullopt, &result)); : ASSERT_EQ(nReplicationFactor, result.size()); : for (const auto& ts : result) { : const auto& ts_uuid = ts->permanent_uuid(); : ++stats[ts_uuid]; : } : } : ASSERT_EQ(kNumServers, stats. > Are these bounds empirical, or calculated. If the later, could you please a Hey, thanks for pointing this out. I went ahead and fixed the flakiness shown by your dist-test runs. For some of these test cases, we're not expected to see the same value for replicas per tserver each time. What's more important is that it's within the expected range. I suppose to be safe and avoid future flakiness I can expand each range by more than we need at the moment. http://gerrit.cloudera.org:8080/#/c/19931/8/src/kudu/master/placement_policy-test.cc@1273 PS8, Line 1273: ASSERT_OK(policy.PlaceTabletRep > More flaky results("../../build-support/dist_test.py loop -n 1000 -- ./bin/ Addressed above. -- 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: 9 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: Tue, 27 Jun 2023 04:48:34 +0000 Gerrit-HasComments: Yes
