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