mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 28824: Fixed the two level sorting in the allocator.
Date Wed, 10 Dec 2014 22:50:52 GMT

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



include/mesos/resources.hpp
<https://reviews.apache.org/r/28824/#comment107383>

    What does unreserved(role) mean?



include/mesos/resources.hpp
<https://reviews.apache.org/r/28824/#comment107385>

    Do you need to comment on whether star (unreserved) resources will be in the hashmap or
not? Looking at you implementation below, seems that you also include star resources.



include/mesos/resources.hpp
<https://reviews.apache.org/r/28824/#comment107389>

    Looking at you code below, consider adding a function which returns unreserved resources.
    
    ```
    Resources unreserved() const;
    ```



src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/28824/#comment107391>

    No need for the temp variable?
    ```
    roleSorter->allocated(role, used.unreserved());
    ```



src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/28824/#comment107398>

    Should we add a comment here that we are assuming all used resources by the framework
are in either star role or framework's role. It gets complicated once we start to introduce
multiple roles for a framework and optimistic offer.



src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/28824/#comment107399>

    Ditto. No need for the temp var.



src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/28824/#comment107404>

    Ditto. No need for the temp var.



src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/28824/#comment107405>

    Ditto.



src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/28824/#comment107406>

    Ditto. Add a note about allocated resources are in either star role or framework's role.



src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/28824/#comment107407>

    Ditto.



src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/28824/#comment107408>

    Ditto.



src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/28824/#comment107434>

    This looks like a bug. You shouldn't consider star role resources here, right?



src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/28824/#comment107438>

    Hum, shouldn't you use foreachpair above for 'reservations' so that you don't need to
do it here again?



src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/28824/#comment107441>

    You probably wanna break after that since all the reservation for that role on the slave
have been allocated, right?



src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/28824/#comment107442>

    ```
    Resources unreserved = slaves[slaveId].available.unreserved();
    ```


- Jie Yu


On Dec. 8, 2014, 10:11 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28824/
> -----------------------------------------------------------
> 
> (Updated Dec. 8, 2014, 10:11 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-2176
>     https://issues.apache.org/jira/browse/MESOS-2176
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Per MESOS-2176:
> 
> (1) The top level role sorter always excludes reserved resources.
> (2) The intra-role framework sorter always includes both reserved and unreserved resources.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp 10777a62492e4a3333d764e0f75c064694e054d1 
>   src/common/resources.cpp 535a0eab6377b9ae63c960cdb05978647f667d5e 
>   src/master/hierarchical_allocator_process.hpp f18346f435a16d1e6243315bffa00fabd164e310

>   src/master/sorter.hpp aabdd0d3755be2534e2f6cae13976277ca39deb1 
>   src/tests/hierarchical_allocator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/28824/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>


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