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 57111: Updated the allocator to handle frameworks that change its roles.
Date Sat, 04 Mar 2017 01:58:16 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57111/#review167882
-----------------------------------------------------------




src/master/allocator/mesos/hierarchical.hpp
Lines 426-429 (original), 426-429 (patched)
<https://reviews.apache.org/r/57111/#comment239845>

    Ditto suggestion from previous review for updating this comment, and also removing the
notion of "active" here.



src/master/allocator/mesos/hierarchical.cpp
Lines 429-432 (original), 394-400 (patched)
<https://reviews.apache.org/r/57111/#comment239864>

    Ditto from last review, could we simplify with a - operator in stout/set.hpp?
    
    ```
    const set<string> removedRoles = oldRoles - newRoles;
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 417-423 (patched)
<https://reviews.apache.org/r/57111/#comment239865>

    Ditto from last review, could we simplify with a - operator in stout/set.hpp?
    
    ```
    const set<string> addedRoles = newRoles - oldRoles;
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 426-429 (patched)
<https://reviews.apache.org/r/57111/#comment239883>

    Hm.. this was a bit tough to grasp for me, how about something like:
    
    ```
    // Add the framework to the role. It's possible that we're already
    // tracking this role for the framework because a framework can
    // unsubscribe from a role while it still has resources allocated
    // to the role.
    ```



src/master/allocator/mesos/hierarchical.cpp
Line 458 (original), 463 (patched)
<https://reviews.apache.org/r/57111/#comment239886>

    Can you commit this separately?



src/master/allocator/mesos/hierarchical.cpp
Lines 470-473 (patched)
<https://reviews.apache.org/r/57111/#comment239895>

    Maybe something a bit more general, since this isn't the only case where this is possible
and that might confuse the reader. It's also possible when there is no partition involved,
when the master is re-registering an agent.
    
    E.g.
    
    ```
    // The framework has resources allocated to this role but it may
    // or may not be subscribed to the role. Either way, we need to
    // track the framework under the role.
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 1065-1071 (patched)
<https://reviews.apache.org/r/57111/#comment239912>

    Hm.. maybe we can use "start tracking" and "stop tracking" in these comments since "adding"
"removing" from the role seems a little less clear to me. E.g.
    
    ```
    Stop tracking the framework under this role if it's no longer subscribed and no longer
has resources allocated to the role.
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 1066 (patched)
<https://reviews.apache.org/r/57111/#comment239896>

    s/no/no longer any/



src/master/allocator/mesos/hierarchical.cpp
Lines 2186-2187 (patched)
<https://reviews.apache.org/r/57111/#comment239855>

    Subscribe *or have resources allocated to this role*, right?



src/master/allocator/mesos/hierarchical.cpp
Lines 2191 (patched)
<https://reviews.apache.org/r/57111/#comment239861>

    Do you want a CHECK in front of this insert, since you seem to be check guarding every
other insert in this function?



src/master/allocator/mesos/hierarchical.cpp
Lines 2218 (patched)
<https://reviews.apache.org/r/57111/#comment239862>

    This condition isn't quite right anymore, maybe something like:
    
    ```
    If no more frameworks are subscribed to this role or have resources allocated to this
role, ...
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 2228 (patched)
<https://reviews.apache.org/r/57111/#comment239849>

    Did you want to use an `.at()` here?


- Benjamin Mahler


On Feb. 27, 2017, 10:20 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57111/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2017, 10:20 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6627
>     https://issues.apache.org/jira/browse/MESOS-6627
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.hpp d33306745a7287b750cb4a5242c7527369d58d65

>   src/master/allocator/mesos/hierarchical.cpp eeb44fe89d4bfd26900b11833c1182157e5c7e5c

> 
> 
> Diff: https://reviews.apache.org/r/57111/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Michael Park
> 
>


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