Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11549 )
Change subject: [rebalancer] location-aware rebalancer (part 1/n) ...................................................................... Patch Set 7: (12 comments) http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.h File src/kudu/tools/placement_policy_util.h: http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.h@101 PS6, Line 101: "best candidate" > nit: Change this to being surrounded by double quotes ", since I and others Done http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.h@111 PS6, Line 111: const TabletsPlacementInfo& placement_info, : const ClusterLocalityInfo& locality_info, : const std::vector<PlacementPolicyViolationInfo>& violations_info, : std::vector<Rebalancer::ReplicaMove>* replicas_to_remove); > nit: Not sure this is need here. Done http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc File src/kudu/tools/placement_policy_util.cc: http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@51 PS6, Line 51: > a Done http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@53 PS6, Line 53: Repl > nit: Replace Done http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@58 PS6, Line 58: DCHECK(replica_to_replace); > nit: DCHECK(replica_to_replace) Done http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@71 PS6, Line 71: : // If a total number of locations is 2, it's impossible to make its replica : // distribution conform with the > It doesn't matter how many replicas there are. With 2 locations it's imposs Right. http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@100 PS6, Line 100: loc_replica_count + 1 >= : consensus::MajoritySi > nit: Reverse the order of this inequality. I think it makes it easier to re Done http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@106 PS6, Line 106: CHECK_NE(location, loc) > nit: Maybe add a failure note. Done http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@143 PS6, Line 143: : CHECK(!ts_id_candidates.empty()) << Substitute( : "no replicas found to remove from location '$0' to fix placement policy " : "violation for tablet $1", location, tablet_id); : > If we don't find any replicas in the location, that means there's some sign That's a good observation. http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@159 PS6, Line 159: replica_id = elem.second; > I think this is overkill given the check on L143. Done http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@158 PS6, Line 158: for (const auto& elem : servers_by_replica_num) { : replica_id = elem.second; : // Prefer non-leader replicas for the replica-to-remove candidates: removing : // a leader replica might require currently active clients to reconnect. : if (replica_id != ts_id_leader_replica) { : break; : } : } : CHECK(!replica_id.empty()); : : * > Can we structure this as a loop? It took me 30 seconds of looking at this t Interesting: to me the loop is harder to comprehend. However, I changed it to be a loop. http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@296 PS6, Line 296: max_rep > losing 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: 7 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: Tue, 23 Oct 2018 05:26:18 +0000 Gerrit-HasComments: Yes
