Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11549 )
Change subject: [rebalancer] location-aware rebalancer (part 1/n) ...................................................................... Patch Set 6: (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 in this context frequently use a name in single quotes to refer to parameter names. http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.h@111 PS6, Line 111: // At the bigger picture, reinstating the placement policy is about finding : // tablet replicas to be marked with the REPLACE attribute. Once the REPLACE : // attribute is set on a replica, the catalog manager will do the rest to move : // the replica elsewhere in accordance with the placement policies. nit: Not sure this is need here. 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 http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@53 PS6, Line 53: Kick nit: Replace http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@58 PS6, Line 58: nit: DCHECK(replica_to_replace) http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@71 PS6, Line 71: // If a tablet has odd replication factor and total number of locations is 2, : // it's impossible to make its replica distribution conform with the : // placement policy constraints. It doesn't matter how many replicas there are. With 2 locations it's impossible not to have at least half of the replicas in one location. http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@100 PS6, Line 100: consensus::MajoritySize(table_info.replication_factor) <= : loc_replica_count + 1 nit: Reverse the order of this inequality. I think it makes it easier to read. 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. http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@143 PS6, Line 143: if (ts_id_candidates.empty()) { : return Status::IllegalState(Substitute( : "no replicas found to remove from location '$0' to fix placement " : "policy violation for tablet $1", tablet_id, location)); : } If we don't find any replicas in the location, that means there's some significant bug and we should just crash, I think. http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@159 PS6, Line 159: CHECK(it != servers_by_replica_num.crend()); I think this is overkill given the check on L143. http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@158 PS6, Line 158: auto it = servers_by_replica_num.crbegin(); : CHECK(it != servers_by_replica_num.crend()); : if (it->second == ts_id_leader_replica) { : // Prefer non-leader replicas for the replica-to-remove candidates: removing : // a leader replica might require currently active clients to reconnect. : ++it; : } : if (it == servers_by_replica_num.crend()) { : DCHECK_EQ(1, servers_by_replica_num.size()); : --it; : } Can we structure this as a loop? It took me 30 seconds of looking at this to get it and I think it would've been more natural to me as a loop like string replica_to_replace_id; for ts : server in decreasing order of replica num: replica_to_replace_id = ts.id if not leader: break check replica_to_replace_id isn't empty (should crash i think) I think that's the same logic. http://gerrit.cloudera.org:8080/#/c/11549/6/src/kudu/tools/placement_policy_util.cc@296 PS6, Line 296: loosing losing -- 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: 6 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Mon, 22 Oct 2018 19:48:11 +0000 Gerrit-HasComments: Yes
