----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31667/#review83669 -----------------------------------------------------------
Ship it! Looks good, just some minor notes below. Went over these with Michael so I'll make the updates for him. src/master/allocator/sorter/drf/sorter.cpp <https://reviews.apache.org/r/31667/#comment134685> Hm.. since we're guaranteed to just re-insert on the next line, let's not bother with this? src/master/allocator/sorter/drf/sorter.cpp <https://reviews.apache.org/r/31667/#comment134686> Maybe we should make it more clear about why we're even talking about safety in the first place? Might not be clear to the reader that Resources::sum is known to be problematic for non-scalars: ``` // NOTE: Summation is incorrect for non-scalars, but since we // only care about scalar resources, this is safe. ``` src/master/allocator/sorter/sorter.hpp <https://reviews.apache.org/r/31667/#comment134680> Thanks! src/master/allocator/sorter/sorter.hpp <https://reviews.apache.org/r/31667/#comment134682> #include <stout/hashmap.hpp> for this? src/tests/sorter_tests.cpp <https://reviews.apache.org/r/31667/#comment134687> Let's break this line consistently with the one below, just to make it more readable :) src/tests/sorter_tests.cpp <https://reviews.apache.org/r/31667/#comment134688> Would be nice to also validate that there is only 1 entry in the map :) src/tests/sorter_tests.cpp <https://reviews.apache.org/r/31667/#comment134689> How about MultipleSlaves and UpdateMultipleSlaves? Since this is these are the only tests now with multiple slaves :) src/tests/sorter_tests.cpp <https://reviews.apache.org/r/31667/#comment134690> Let's check the size too? src/tests/sorter_tests.cpp <https://reviews.apache.org/r/31667/#comment134700> EXPECT_EQ wants the expected value as the first argument (to print expected vs actual), so let's flip the arguments here src/tests/sorter_tests.cpp <https://reviews.apache.org/r/31667/#comment134692> This says construct an offer operation, but you just flatten? Seems like we should go through the apply() operation code here. src/tests/sorter_tests.cpp <https://reviews.apache.org/r/31667/#comment134691> Let's check the size too? src/tests/sorter_tests.cpp <https://reviews.apache.org/r/31667/#comment134701> Ditto here, let's flip the arguments. - Ben Mahler On May 10, 2015, 12:21 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31667/ > ----------------------------------------------------------- > > (Updated May 10, 2015, 12:21 a.m.) > > > Review request for mesos, Alexander Rukletsov and Ben Mahler. > > > Bugs: MESOS-2373 > https://issues.apache.org/jira/browse/MESOS-2373 > > > Repository: mesos > > > Description > ------- > > `Sorter` changes: > > - Augmented `add`, `remove`, `allocated`, `unallocated`, `update` with > `SlaveID`. > - `allocation` returns `hashmap<SlaveID, Resources>`. > > `DRFSorter` changes: > > - `allocations` is updated from `hashmap<std::string, Resources>` to > `hashmap<std::string, hashmap<SlaveID, Resources>>`. > - `resources` is updated from `Resources` to `hashmap<SlaveID, Resources>`. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > 09adced9d8712b3eeda885d598443791186890db > src/master/allocator/sorter/drf/sorter.hpp > 4366710d6530b784aa5094813328d0e338239ba0 > src/master/allocator/sorter/drf/sorter.cpp > 2f69f384b95ff20d3ee429a4570a8cffa74d8e8b > src/master/allocator/sorter/sorter.hpp > e2efb27b11dbea42dd73f81e5db0d6d2b0a6034b > src/tests/sorter_tests.cpp 42442353afe7bd3d1a5b43992f8ae191ac19bdcd > > Diff: https://reviews.apache.org/r/31667/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Michael Park > >
