You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Thomas Marshall <tw...@gmail.com> on 2012/07/12 21:19:03 UTC

Review Request: Round Robin Allocator

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

Review request for mesos and Benjamin Hindman.


Description
-------

Added a RoundRobinAllocator class which can be used with an AllocatorProcess to implement round robin allocations at either the per user or per framework level.

Also added tests and command line options for RoundRobinAllocator.

This patch depends on the series of allocator patches currently pending review. The patch immediately preceding this one in the series is:
https://reviews.apache.org/r/5910/


This addresses bug MESOS-221.
    https://issues.apache.org/jira/browse/MESOS-221


Diffs
-----

  src/master/allocator.hpp 12f31db 
  src/master/allocator.cpp PRE-CREATION 
  src/master/allocator_process.hpp PRE-CREATION 
  src/master/allocator_process.cpp PRE-CREATION 
  src/master/flags.hpp c0eb61f 
  src/master/simple_allocator_process.hpp PRE-CREATION 
  src/tests/allocator_process_tests.cpp PRE-CREATION 
  src/tests/allocator_tests.cpp 610826b 

Diff: https://reviews.apache.org/r/5917/diff/


Testing
-------


Thanks,

Thomas Marshall

Re: Review Request: Round Robin Allocator

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5917/#review10463
-----------------------------------------------------------


just some quick comments so you don't have to wait on benh forever :)


src/master/round_robin_sorter.cpp
<https://reviews.apache.org/r/5917/#comment22319>

    list removal is O(N), does this happen frequently?



src/master/round_robin_sorter.cpp
<https://reviews.apache.org/r/5917/#comment22317>

    s/insert/remove ?



src/master/round_robin_sorter.cpp
<https://reviews.apache.org/r/5917/#comment22320>

    again, this is O(N)
    
    we could convert the hashset to hashmap<string, iterator> which would allow us to do use erase() which is O(1) and preserves all other iterators
    push_back also preserves iterators (but you should triple check this).
    
    I don't really know the call frequency on these functions, so maybe this is moot.



src/master/round_robin_sorter.cpp
<https://reviews.apache.org/r/5917/#comment22324>

    if we've got an iterator to the lastClient (using the map I mentioned) then we can build the shifted list much easier as well:
    
    list<string> result(it, order.end());
    result.insert(order.begin(), it);
    return result;
    
    P.S. These can be const iterators, no?


- Ben Mahler


On Aug. 16, 2012, 11:34 p.m., Thomas Marshall wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5917/
> -----------------------------------------------------------
> 
> (Updated Aug. 16, 2012, 11:34 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Description
> -------
> 
> Added a RoundRobinAllocator class which can be used with an AllocatorProcess to implement round robin allocations at either the per user or per framework level.
> 
> Also added tests and command line options for RoundRobinAllocator.
> 
> This patch depends on the series of allocator patches currently pending review. The patch immediately preceding this one in the series is:
> https://reviews.apache.org/r/6665/
> 
> 
> This addresses bug MESOS-221.
>     https://issues.apache.org/jira/browse/MESOS-221
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am b0cb6cc 
>   src/master/allocator.cpp PRE-CREATION 
>   src/master/flags.hpp f66f0a3 
>   src/master/round_robin_sorter.hpp PRE-CREATION 
>   src/master/round_robin_sorter.cpp PRE-CREATION 
>   src/tests/sorter_tests.cpp PRE-CREATION 
>   src/tests/utils.hpp caf5926 
> 
> Diff: https://reviews.apache.org/r/5917/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Thomas Marshall
> 
>

Re: Review Request: Round Robin Allocator

Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5917/
-----------------------------------------------------------

(Updated Aug. 21, 2012, 9:50 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
-------

Updated to latest review.


Description
-------

Added a RoundRobinAllocator class which can be used with an AllocatorProcess to implement round robin allocations at either the per user or per framework level.

Also added tests and command line options for RoundRobinAllocator.

This patch depends on the series of allocator patches currently pending review. The patch immediately preceding this one in the series is:
https://reviews.apache.org/r/6665/


This addresses bug MESOS-221.
    https://issues.apache.org/jira/browse/MESOS-221


Diffs (updated)
-----

  src/Makefile.am aaceee3 
  src/master/allocator.cpp 3f82291 
  src/master/flags.hpp 6f450d5 
  src/master/round_robin_sorter.hpp PRE-CREATION 
  src/master/round_robin_sorter.cpp PRE-CREATION 
  src/tests/sorter_tests.cpp 2b00a7f 
  src/tests/utils.hpp 83d5daa 

Diff: https://reviews.apache.org/r/5917/diff/


Testing
-------


Thanks,

Thomas Marshall

Re: Review Request: Round Robin Allocator

Posted by Thomas Marshall <tw...@gmail.com>.

> On Aug. 20, 2012, 9:22 p.m., Ben Mahler wrote:
> > src/master/round_robin_sorter.cpp, line 62
> > <https://reviews.apache.org/r/5917/diff/2-3/?file=141721#file141721line62>
> >
> >     like above, use the iterator to erase
> >     
> >     also, remove from the clients map here too?

The purpose of the clients map is to differentiate clients that have been deactivated and not removed, so removing it here would defeat the point.


> On Aug. 20, 2012, 9:22 p.m., Ben Mahler wrote:
> > src/master/round_robin_sorter.cpp, line 46
> > <https://reviews.apache.org/r/5917/diff/2-3/?file=141721#file141721line46>
> >
> >     use the iterator map for O(1) removal :)
> >     order.erase(clients[client])
> >     
> >     you may want to CHECK/ASSERT that client is in the map, I don't yet know mesos style for that

We use CHECK().


- Thomas


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


On Aug. 21, 2012, 9:50 p.m., Thomas Marshall wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5917/
> -----------------------------------------------------------
> 
> (Updated Aug. 21, 2012, 9:50 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Description
> -------
> 
> Added a RoundRobinAllocator class which can be used with an AllocatorProcess to implement round robin allocations at either the per user or per framework level.
> 
> Also added tests and command line options for RoundRobinAllocator.
> 
> This patch depends on the series of allocator patches currently pending review. The patch immediately preceding this one in the series is:
> https://reviews.apache.org/r/6665/
> 
> 
> This addresses bug MESOS-221.
>     https://issues.apache.org/jira/browse/MESOS-221
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am aaceee3 
>   src/master/allocator.cpp 3f82291 
>   src/master/flags.hpp 6f450d5 
>   src/master/round_robin_sorter.hpp PRE-CREATION 
>   src/master/round_robin_sorter.cpp PRE-CREATION 
>   src/tests/sorter_tests.cpp 2b00a7f 
>   src/tests/utils.hpp 83d5daa 
> 
> Diff: https://reviews.apache.org/r/5917/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Thomas Marshall
> 
>

Re: Review Request: Round Robin Allocator

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5917/#review10550
-----------------------------------------------------------



src/master/round_robin_sorter.cpp
<https://reviews.apache.org/r/5917/#comment22565>

    this can be done in the initializer rather than the constructor body



src/master/round_robin_sorter.cpp
<https://reviews.apache.org/r/5917/#comment22566>

    pre-increment instead



src/master/round_robin_sorter.cpp
<https://reviews.apache.org/r/5917/#comment22575>

    use the iterator map for O(1) removal :)
    order.erase(clients[client])
    
    you may want to CHECK/ASSERT that client is in the map, I don't yet know mesos style for that



src/master/round_robin_sorter.cpp
<https://reviews.apache.org/r/5917/#comment22577>

    we may want to be doing some CHECKs or something to assert that client is indeed in clients
    
    otherwise, the [] operator will actually insert an entry into the map



src/master/round_robin_sorter.cpp
<https://reviews.apache.org/r/5917/#comment22576>

    like above, use the iterator to erase
    
    also, remove from the clients map here too?



src/master/round_robin_sorter.cpp
<https://reviews.apache.org/r/5917/#comment22578>

    pre-increment



src/master/round_robin_sorter.cpp
<https://reviews.apache.org/r/5917/#comment22563>

    I think we can kill these loops with:
    
    list<string> ret(nextClient, order.end());
    ret.insert(ret.end(), order.begin(), nextClient);
    
    or
    
    list<string> ret(order.begin(), nextClient);
    ret.insert(ret.begin(), nextClient, order.end());



src/master/round_robin_sorter.cpp
<https://reviews.apache.org/r/5917/#comment22568>

    just as an fyi for any other loops / iterator incrementing, we pre-increment iterators



src/tests/sorter_tests.cpp
<https://reviews.apache.org/r/5917/#comment22579>

    test empty lists?


- Ben Mahler


On Aug. 17, 2012, 11:17 p.m., Thomas Marshall wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5917/
> -----------------------------------------------------------
> 
> (Updated Aug. 17, 2012, 11:17 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Description
> -------
> 
> Added a RoundRobinAllocator class which can be used with an AllocatorProcess to implement round robin allocations at either the per user or per framework level.
> 
> Also added tests and command line options for RoundRobinAllocator.
> 
> This patch depends on the series of allocator patches currently pending review. The patch immediately preceding this one in the series is:
> https://reviews.apache.org/r/6665/
> 
> 
> This addresses bug MESOS-221.
>     https://issues.apache.org/jira/browse/MESOS-221
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am b0cb6cc 
>   src/master/allocator.cpp PRE-CREATION 
>   src/master/flags.hpp f66f0a3 
>   src/master/round_robin_sorter.hpp PRE-CREATION 
>   src/master/round_robin_sorter.cpp PRE-CREATION 
>   src/tests/sorter_tests.cpp PRE-CREATION 
>   src/tests/utils.hpp caf5926 
> 
> Diff: https://reviews.apache.org/r/5917/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Thomas Marshall
> 
>

Re: Review Request: Round Robin Allocator

Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5917/
-----------------------------------------------------------

(Updated Aug. 17, 2012, 11:17 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
-------

Updated to Ben M's review.


Description
-------

Added a RoundRobinAllocator class which can be used with an AllocatorProcess to implement round robin allocations at either the per user or per framework level.

Also added tests and command line options for RoundRobinAllocator.

This patch depends on the series of allocator patches currently pending review. The patch immediately preceding this one in the series is:
https://reviews.apache.org/r/6665/


This addresses bug MESOS-221.
    https://issues.apache.org/jira/browse/MESOS-221


Diffs (updated)
-----

  src/Makefile.am b0cb6cc 
  src/master/allocator.cpp PRE-CREATION 
  src/master/flags.hpp f66f0a3 
  src/master/round_robin_sorter.hpp PRE-CREATION 
  src/master/round_robin_sorter.cpp PRE-CREATION 
  src/tests/sorter_tests.cpp PRE-CREATION 
  src/tests/utils.hpp caf5926 

Diff: https://reviews.apache.org/r/5917/diff/


Testing
-------


Thanks,

Thomas Marshall

Re: Review Request: Round Robin Allocator

Posted by Thomas Marshall <tw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5917/
-----------------------------------------------------------

(Updated Aug. 16, 2012, 11:34 p.m.)


Review request for mesos and Benjamin Hindman.


Changes
-------

Updated to all the changes further up in the review pipeline.


Description (updated)
-------

Added a RoundRobinAllocator class which can be used with an AllocatorProcess to implement round robin allocations at either the per user or per framework level.

Also added tests and command line options for RoundRobinAllocator.

This patch depends on the series of allocator patches currently pending review. The patch immediately preceding this one in the series is:
https://reviews.apache.org/r/6665/


This addresses bug MESOS-221.
    https://issues.apache.org/jira/browse/MESOS-221


Diffs (updated)
-----

  src/Makefile.am b0cb6cc 
  src/master/allocator.cpp PRE-CREATION 
  src/master/flags.hpp f66f0a3 
  src/master/round_robin_sorter.hpp PRE-CREATION 
  src/master/round_robin_sorter.cpp PRE-CREATION 
  src/tests/sorter_tests.cpp PRE-CREATION 
  src/tests/utils.hpp caf5926 

Diff: https://reviews.apache.org/r/5917/diff/


Testing
-------


Thanks,

Thomas Marshall