Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11549 )
Change subject: [rebalancer] location-aware rebalancer (part 1/3) ...................................................................... Patch Set 4: (24 comments) http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc File src/kudu/tools/placement_policy_util-test.cc: http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@51 PS3, Line 51: TestTableInfo > nit: I think it'd be fair to remove the "Info" suffix from all of these str Maybe, we can leave it as is for now. Once this patch is closer to the to-be-committed state, I could make some renamings like that. I added TODO to address that. http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@58 PS3, Line 58: TabletServerLocationInfo > nit: Likewise, I think this could just be called "TestLocation" or somethin ditto http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@64 PS3, Line 64: TabletServerReplicasInfo > nit: Likewise, I think this could just be called "TestTabletServer" or some ditto http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@69 PS3, Line 69: Describing > nit: Describes. Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@74 PS3, Line 74: // All available tablet servers and their locations. > IIUC this collection is more akin to all the locations. The locations merel Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@77 PS3, Line 77: // Distribution of tablet replicas among tablet servers. > Likewise, it seems like this is best described as the collection of tablet Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@81 PS3, Line 81: : the reference to compare the results with > Remove. Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@119 PS3, Line 119: repilca_num_by_ts_id > replica_num_by_ts_id Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@126 PS3, Line 126: tests > nit: "this type of test" or "these types of tests" Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@129 PS3, Line 129: this tests > these tests Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@130 PS3, Line 130: true, true > Would you mind adding a comment with the parameter name? Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@132 PS3, Line 132: TabletReplicaInfo{ replica_info.ts_id, false, true } > Ditto. Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@151 PS3, Line 151: bool operator==(const vector<PlacementPolicyViolationInfo>& lhs, : const vector<PlacementPolicyViolationInfo>& rhs) > TBH hiding the sorting behind a custom vector<T> == is too magic for me. Co Yup, that's too magical and gross. I ended up building auxiliary maps for comparison between the reference and actual results (resembles the first option you proposed). http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@197 PS3, Line 197: bool operator==(const vector<Rebalancer::ReplicaMove>& lhs, : const vector<Rebalancer::ReplicaMove>& rhs) { > Likewise, so maybe a templated type synonym...you could implement compariso ditto http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.h File src/kudu/tools/placement_policy_util.h: http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.h@32 PS3, Line 32: replica > replicas Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.h@33 PS3, Line 33: table > tables Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.h@33 PS3, Line 33: tablet > tablets Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.h@40 PS3, Line 40: bool is_voter; > nit: Can you be a leader but not a follower? Would it make more sense to re That a good one! Thanks. http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.h@45 PS3, Line 45: for CAS-like change of Raft configs > Super-nit: Begin comment with capital letter and end with . I thought that was for full-line comments only, not in-lined comments like this one. Done. http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.cc File src/kudu/tools/placement_policy_util.cc: http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.cc@82 PS3, Line 82: > Although we disallow creating such a table by default, I think to handle re I updated this piece to handle RF=2*N and RF=2*N+1 separately, because in case of RF=2*N loosing at least N replicas means loosing the majority. Thank you for pointing at this! http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.cc@129 PS3, Line 129: lets_info, > conform Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.cc@128 PS3, Line 128: : const auto& tablet_info = FindOrDie(tablets_info.tablets_info, tablet_id); : const auto& ts_at_location = Fin > Looking at this I thought "isn't this just a special case of a more general Ack. http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.cc@298 PS3, Line 298: t > extra 'c' Done http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/rebalancer.h File src/kudu/tools/rebalancer.h: http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/rebalancer.h@247 PS3, Line 247: //friend class KsckResultsToClusterLocationInfoTest > Extra? Done -- To view, visit http://gerrit.cloudera.org:8080/11549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1cdc3b57a782e6d043b1853600c97fe1f8630347 Gerrit-Change-Number: 11549 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 19 Oct 2018 14:24:36 +0000 Gerrit-HasComments: Yes
