Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11207 )
Change subject: [location_awareness] replica selection honors placement policy ...................................................................... Patch Set 3: (30 comments) http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG@7 PS3, Line 7: [master] > nit: Consider making the label [location_awareness] to be consistent with t Done http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG@9 PS3, Line 9: placement policy > Would you mind adding another sentence explaining the single policy that is Done http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG@10 PS3, Line 10: > nit: One space. Done http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG@13 PS3, Line 13: Added > nit: This patch also adds a few... Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h@524 PS3, Line 524: std::string, int > Please add a short comment explaining what the key and value types are. I removed this since it moved into the PlacementPolicy class. http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h@880 PS3, Line 880: online > nit: I think the standard term in "live". Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h@880 PS3, Line 880: grouped by location : // (as specified by 'ltd') > What is 'ltd'? Looks like this comment is stale. Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h@887 PS3, Line 887: PlacementPolicy* > nit: Naively I expected this to be a cref because something called a Placem Ah, that's because of the random generator pointer being the member of the PlacementPolicy class. Maybe a better approach would be passing the generator as a parameter in all those methods since the Placement policy just keeps the raw pointer anyway. As of now I added 'mutable' specified for the rng_ raw pointer member. http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h File src/kudu/master/placement_policy.h: http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@39 PS3, Line 39: placing > place Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@39 PS3, Line 39: at proper tablet servers > on tablet servers according to some rules. Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@40 PS3, Line 40: class PlacementPolicy { > Please add another sentence with a brief description of the specific policy Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@49 PS3, Line 49: destructed > "destroyed", or say that 'rng' must outlive of the PlacementPolicy. Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@82 PS3, Line 82: // TODO (aserbin): add the reference to the document once it's in the repo > nit: End with . Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@100 PS3, Line 100: is > Remove extra word. Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@108 PS3, Line 108: : it's in constructor and cached > I think if we make this member const it enforces these two things. Ah, good idea. http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc File src/kudu/master/placement_policy.cc: http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@51 PS3, Line 51: > a Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@51 PS3, Line 51: > the Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@51 PS3, Line 51: repilcas > replicas Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@52 PS3, Line 52: of > Remove extra 'of'. Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@55 PS3, Line 55: unordered_map<string, int> > Is this a ReplicaLocationsInfo type? Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@57 PS3, Line 57: DCHECK(it != ltd.end()); : if (it == ltd.end()) { : return numeric_limits<double>::max(); : } > Is there a reason we shouldn't always crash if we break the invariant that Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@62 PS3, Line 62: DCHECK(!ts_descriptors.empty()); > Same thing here. Shouldn't every location we consider here have at least on Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@81 PS3, Line 81: unordered_map<string, int> > This too is a ReplicaLocationsInfo, right? Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@167 PS3, Line 167: ts_num_ = accumulate( : ltd_.begin(), ltd_.end(), 0, : [](size_t val, const LocationToDescriptorsMap::value_type& elem) { : return val + elem.second.size(); : }); : DCHECK_EQ(ts_num_, descs.size()); > Should ts_num_ be const and set in the initializer list? BuildLocationToDes Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@188 PS3, Line 188: if (it == ltd_.end()) { : return Status::IllegalState( : Substitute("'$0': no info on tablet servers at location", loc)); : } > This should be impossible, right? We should at least DCHECK on it, if so. Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@201 PS3, Line 201: unordered_map<string, int> > Ditto on the type alias. Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@203 PS3, Line 203: "" > It's a little pedantic, but could you add a comment saying it's ok to use a Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@254 PS3, Line 254: avalailable > available Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@256 PS3, Line 256: table > tablet Done http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@264 PS3, Line 264: break; > This loop combo is a little tricky. It took me a bit of thinking to see why Done -- 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: 3 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 24 Aug 2018 03:27:26 +0000 Gerrit-HasComments: Yes
