Marton Greber 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 8: (5 comments) 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: 1 nit: After reading the descriptive comment above the test, I had to look up PlaceTabletReplicas() to make sure that the first argument is the RF. For readability sake would it make sense to create a variable to store the RF? (the same thing for all PlaceTabletReplicas() calls below - don't wanna spam all of them with a comment) http://gerrit.cloudera.org:8080/#/c/19931/8/src/kudu/master/placement_policy-test.cc@1058 PS8, Line 1058: 1 nit: and if I understand correctly, here you could assert against the above introduced RF variable. Right? http://gerrit.cloudera.org:8080/#/c/19931/8/src/kudu/master/placement_policy-test.cc@1145 PS8, Line 1145: range({ {"b1", 500} }, 500); : TabletNumByRangeAndTable range1 nit: maybe in such cases where you have multiple ranges you could number all the variables e.g. in this case: range1 range2 http://gerrit.cloudera.org:8080/#/c/19931/8/src/kudu/master/placement_policy-test.cc@1210 PS8, Line 1210: range({ {"b1", 250} }, 250); : TabletNumByRangeAndTable range1({ {"a1", 375} }, 375); : TabletNumByRangeAndTable range2 nit: range1 range2 range3 http://gerrit.cloudera.org:8080/#/c/19931/8/src/kudu/master/placement_policy-test.cc@1242 PS8, Line 1242: vector<string> tservers_no_load = {"ts0", "ts1", "ts2"}; : for (const auto& tserver : tservers_no_load) { : ASSERT_LE(684, stats[tserver]); : ASSERT_GE(689, stats[tserver]); : } : ASSERT_LE(434, stats["ts3"]); : ASSERT_GE(440, stats["ts3"]); : ASSERT_LE(309, stats["ts4"]); : ASSERT_GE(313, stats["ts4"]); : ASSERT_LE(184, stats["ts5"]); : ASSERT_GE(188, stats["ts5"]); Are these bounds empirical, or calculated. If the later, could you please add some elaborative comments? The reason I'm asking is because I checked it for flakyness and there was a time when it failed. Repro: ../../build-support/dist_test.py loop -n 1000 -- ./bin/placement_policy-test --stress_cpu_th reads=16 Result link: http://dist-test.cloudera.org/job?job_id=root.1687689632.26110 "... [ RUN ] PlacementPolicyTest.PlaceTabletReplicasWithRangesAndTablesWithDoubleTServers ../../src/kudu/master/placement_policy-test.cc:1252: Failure Expected: (188) >= (stats["ts5"]), actual: 188 vs 189 [ FAILED ] PlacementPolicyTest.PlaceTabletReplicasWithRangesAndTablesWithDoubleTServers (91 ms) [----------] 21 tests from PlacementPolicyTest (1633 ms total) [----------] Global test environment tear-down [==========] 21 tests from 1 test suite ran. (1633 ms total) [ PASSED ] 20 tests. [ FAILED ] 1 test, listed below: [ FAILED ] PlacementPolicyTest.PlaceTabletReplicasWithRangesAndTablesWithDoubleTServers 1 FAILED TEST" -- 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: 8 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: Sun, 25 Jun 2023 10:48:36 +0000 Gerrit-HasComments: Yes
