mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 57110: Updated the master to handle frameworks that changes its roles.
Date Sun, 05 Mar 2017 20:37:45 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message