----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57110/#review167948 -----------------------------------------------------------
Couple of high level suggestions: (1) Looks like some patches could be pulled out of this to unclutter the diff? (e.g. the 'activeRoles' and 'source' renames) (2) The use of "subscribe" in the helpers seems confusing, see below. src/master/master.hpp Lines 1785-1786 (original), 1789-1793 (patched) <https://reviews.apache.org/r/57110/#comment239932> It looks like we should have pulled out this rename to into a separate patch to have this be a bit cleaner, oh well. src/master/master.hpp Lines 2248-2250 (patched) <https://reviews.apache.org/r/57110/#comment239934> Hm.. well this sounds misleading, since it seems to read as we're subscribing the framework to the role which means offers will be sent, but that's not the case? Shall we call this something different? Like `trackRole`? src/master/master.hpp Lines 2282-2286 (patched) <https://reviews.apache.org/r/57110/#comment239937> This seems like another spot of confusion with the use of "subscribed" in the helpers introduced in this patch. Here I would expect `roles.count(role) == 0` to be equivalent to `!isSubscribed(role)`. Also, calling `unsubscribe(role)` here sounds equivalent to the framework having removed its role, but it's just stopping to track allocation for the role? src/master/master.hpp Lines 2408-2419 (patched) <https://reviews.apache.org/r/57110/#comment239938> I guess given the phrasing here, using the word "track" might be less confusing for these helpers.. e.g. ``` if (!executorInfo.resources().empty()) { std::string role = executorInfo.resources().begin()->allocation_info().role(); if (!trackingAllocation(role)) { trackAllocation(role); } } ``` src/master/master.hpp Lines 2438-2440 (patched) <https://reviews.apache.org/r/57110/#comment239939> We probably want to use consistent language here, "remove ourself from the role" implies the other locations should say "add ourself to the role" but since adding and removing seems a bit vague, this case might be clearer as "stop tracking the allocation for the role". src/master/master.hpp Line 2423 (original), 2467 (patched) <https://reviews.apache.org/r/57110/#comment239943> Do you want to pull this 'source' to 'newInfo' rename patch out to unclutter this diff? src/master/master.hpp Lines 2652-2654 (patched) <https://reviews.apache.org/r/57110/#comment239940> Per the other suggestions, perhaps something like: ``` bool trackingAllocationToRole(const std::string& role) const; void trackAllocationToRole(const std::string& role); void stopTrackingAllocationToRole(const std::string& role); ``` src/master/master.hpp Lines 2760 (patched) <https://reviews.apache.org/r/57110/#comment239942> Brace on the next line src/master/master.cpp Lines 381-412 (patched) <https://reviews.apache.org/r/57110/#comment239941> Braces on the next line src/master/master.cpp Lines 401-412 (patched) <https://reviews.apache.org/r/57110/#comment239944> Since this is effectively `stopTrackingAllocationToRole`, should we be CHECKing the invariants? i.e. that there is no more allocation and that we're not subscribed to the role src/master/quota_handler.cpp Line 135 (original), 135 (patched) <https://reviews.apache.org/r/57110/#comment239933> Not yours, but do you want commit a change to use .at here? - Benjamin Mahler On March 5, 2017, 3:55 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57110/ > ----------------------------------------------------------- > > (Updated March 5, 2017, 3:55 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-6627 > https://issues.apache.org/jira/browse/MESOS-6627 > > > Repository: mesos > > > Description > ------- > > Updated the master to handle frameworks that changes its roles. > > > Diffs > ----- > > src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d > src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 > src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 > src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a > src/master/weights_handler.cpp d8047f2354322ec18b008afe44f507069b36a71f > > > Diff: https://reviews.apache.org/r/57110/diff/7/ > > > Testing > ------- > > > Thanks, > > Michael Park > >
