mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Neil Conway <neil.con...@gmail.com>
Subject Re: Review Request 49375: Simplified DRFSorter to not track per-slave total resources.
Date Mon, 04 Jul 2016 08:59:01 GMT


> On July 4, 2016, 7:51 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 716-723
> > <https://reviews.apache.org/r/49375/diff/2/?file=1434830#file1434830line716>
> >
> >     Just nit: I think that here the `role sorters` should include both `roleSorter`
and `quotaRoleSorter`, so what about grouping the `sorters` into one code block as following?
> >     
> >     // Now, update the total resources in the role sorters by removing
> >     // the previous resources at this slave and adding the new resources.
> >     roleSorter->remove(slaveId, oldTotal);
> >     roleSorter->add(slaveId, updatedTotal.get());
> >     quotaRoleSorter->remove(slaveId, oldTotal.nonRevocable());
> >     quotaRoleSorter->add(slaveId, updatedTotal.get().nonRevocable());

Personally I think this is a bit easier to read with some whitespace. The comment about non-revocable
resources in the quota role sorter is also worth keeping.


- Neil


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


On July 1, 2016, 9:47 a.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49375/
> -----------------------------------------------------------
> 
> (Updated July 1, 2016, 9:47 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-5698
>     https://issues.apache.org/jira/browse/MESOS-5698
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The DRFSorter previously kept the total resources at each slave, along
> with the total quantity of resources in the cluster. The latter figure
> is what most of the clients of the sorter care about. In the few places
> where the previous coding was using the per-slave total resources, it is
> relatively easy to adjust the code to remove this dependency.
> 
> As part of this change, remove `total()` and `update(const SlaveID&
> slaveId, const Resources& resources)` from the Sorter interface. The
> former was unused; for the latter, code that used it can instead be
> rewritten to adjust the total resources in the cluster by first removing
> the previous resources at a slave and then adding the new resources.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 38381237fa6ceb3f21fd0d4b07d7c3787f0129df

>   src/master/allocator/sorter/drf/sorter.hpp 0aa1a71da4501a3b469d07538a043b4c1d74d688

>   src/master/allocator/sorter/drf/sorter.cpp 967290d4d1100208900b4b724422c3218abc23cb

>   src/master/allocator/sorter/sorter.hpp 5ce6bb82b0127257d97daf0cea6d1d0db405bf83 
>   src/tests/sorter_tests.cpp 6fc58829892dc0223140f1b47593a3e5853cace5 
> 
> Diff: https://reviews.apache.org/r/49375/diff/
> 
> 
> Testing
> -------
> 
> make check on OSX and recent Arch Linux.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


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