Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11207 )
Change subject: [location_awareness] replica selection honors placement policy ...................................................................... Patch Set 7: (21 comments) Just small things and I think this is good to go. http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG@16 PS7, Line 16: avaialable available http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG@17 PS7, Line 17: load replicas http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@878 PS7, Line 878: Select required number tablet servers to place replicas for the given newly : // created table Howabout "Select the tablet servers where the given newly-created tablet's replica will be placed"? http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@880 PS7, Line 880: is are http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@882 PS7, Line 882: The nit: Remove http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@3391 PS7, Line 3391: for (const auto& ts_desc : ts_descs) { : if (IsRaftConfigMember(ts_desc->permanent_uuid(), cstate_.committed_config())) { : InsertOrDie(&existing, ts_desc); : } : } It isn't very important because this code path isn't hot and the number of TS will be small, but wouldn't it be better to iterate over the UUIDs of the peers in the config to populate the set, rather than filtering all TS using the config? The difference is that 'ts_descs' excludes presumed dead TS, so if we changed how 'existing' is populated it might contain presumed dead TS. This should be harmless since we'd never propose to use a presumed dead TS since our options come from 'ts_descs'. http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@4336 PS7, Line 4336: RETURN_NOT_OK(policy.PlaceTabletReplicas(nreplicas, &descriptors)); Can we add the tablet ID and table name to the status returned here? http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@4345 PS7, Line 4345: // TODO(unknown): this is temporary, we will use only UUIDs nit: I don't think this is temporary anymore :). If there's no JIRA related to this I feel like we can drop it. http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h File src/kudu/master/placement_policy.h: http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h@41 PS7, Line 41: the Remove http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h@73 PS7, Line 73: of Remove http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc File src/kudu/master/placement_policy.cc: http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@55 PS7, Line 55: a the http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@62 PS7, Line 62: deltas: information location of replicas that not yet placed Stale? http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@63 PS7, Line 63: ltd Out of order w.r.t the function parameters. http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@74 PS7, Line 74: auto num_live_replicas = accumulate( : ts_descriptors.begin(), ts_descriptors.end(), 0, : [](int val, const shared_ptr<TSDescriptor>& desc) { : return val + desc->num_live_replicas(); : }); : const auto liit = locations_info.find(location); : if (liit != locations_info.end()) { : num_live_replicas += liit->second; : } Is this computing R then adding R to it, so we return 2x the load? http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@107 PS7, Line 107: > Should we (D?)CHECK that > doesn't happen? http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@115 PS7, Line 115: to of http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@115 PS7, Line 115: for Remove http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@125 PS7, Line 125: if (location_per_load.empty()) { : // If placing an additional replica to any location will make it containing : // the majority of replilcas of the same tablet, place the replica into : // one of the less loaded locations (see below). : location_per_load.swap(location_per_load_majority); : } If this happens and the RF is 2k, then each location must have k replicas, which means there's a contradiction as we actually don't have another replica to place. If this happens and the RF is 2k + 1, then each location must have k replicas, so there's at most two locations. Does that sound right? If so I'd explain how special a case this is in the comment rather than making this situation appear so general. http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@147 PS7, Line 147: if (rng) We don't require rng to be non-null? Elsewhere you required it to outlive the PlacementPolicy instance using it, so I feel like we can CHECK on rng in this function. http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@141 PS7, Line 141: if (distance < 2) { : return it->second; : } : : // In case if multiple location candidates, select a random one : // if RNG is available. : if (rng) { : std::advance(it, rng->Uniform(distance)); : } This can be handled uniformly, right? http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@241 PS7, Line 241: //auto loc = SelectLocation(all_locations, {}, ltd_, location_info, rng_); Leftover bit of code. -- To view, visit http://gerrit.cloudera.org:8080/11207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 05 Sep 2018 16:30:54 +0000 Gerrit-HasComments: Yes
