Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11549 )
Change subject: [rebalancer] location-aware rebalancer (part 1/3) ...................................................................... Patch Set 3: (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 structs. But I also don't care if you leave it just to save the time it would take to make the change. 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 something. 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 something. http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@69 PS3, Line 69: Describing nit: Describes. 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 merely reference the tablet servers by id. 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 servers, which of course know what replicas they host. 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. 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 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" http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util-test.cc@129 PS3, Line 129: this tests these tests 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? TabletReplicaInfo{ replica_info.ts_id, /*is_leader=*/true, /*is_voter=*/true } 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. 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. Could we use a sorted map of tablet id -> violation instead? Or maybe a typedef to make a synonym for vector<PlacementPolicyViolationInfo>, then implement == for that? 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 comparison and then implement the unsorted == once and reduce the amount of code. 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 http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.h@33 PS3, Line 33: tablet tablets http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.h@33 PS3, Line 33: table tables 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 replace the 4 states of (bool, bool) with three states of something like enum class ReplicaRole { LEADER, FOLLOWER_VOTER, FOLLOWER_NONVOTER, }; This would also make the constructor calls in the test and other places where TabletReplicaInfo is constructed easier to read without referring back to the definition. TabletReplicaInfo { replica_info.ts_id, ReplicaRole::FOLLOWER_VOTER } 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 . 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: rep_factor > 1 Although we disallow creating such a table by default, I think to handle replication factor 2 tables this should be rep_factor > 2, since a 2x replicated tablet also must have a majority in a single location if it has any replicas in a location. http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.cc@129 PS3, Line 129: conforming conform http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.cc@128 PS3, Line 128: // If a tablet has odd replication factor and total number of locations is 2, : // it's impossible to make its replica distribution conforming with the : // placement policy constraints. Looking at this I thought "isn't this just a special case of a more general rule?" but I think it's the right thing because the other cases should more or less not really happen, as they require a high replication factor, so letting them fall through in the general logic below is fine. http://gerrit.cloudera.org:8080/#/c/11549/3/src/kudu/tools/placement_policy_util.cc@298 PS3, Line 298: c extra 'c' 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? -- 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: 3 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 16 Oct 2018 21:32:20 +0000 Gerrit-HasComments: Yes
