----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35664/#review88582 -----------------------------------------------------------
Looks good, is it easy to add a test for the bug? src/master/allocator/sorter/drf/sorter.cpp (lines 134 - 150) <https://reviews.apache.org/r/35664/#comment141175> Might be a bit clearer if all the updates to 'total' are done together, and all the updates to 'allocations' are done together? Seems a bit odd to me to break them up by scalar-ness, but up to you. src/master/allocator/sorter/drf/sorter.cpp (lines 215 - 216) <https://reviews.apache.org/r/35664/#comment141172> Whoops, the old scalars from this slave needs to be subtracted, and the new scalars added. Mind adding a test that can catch this? src/master/allocator/sorter/drf/sorter.cpp (lines 295 - 299) <https://reviews.apache.org/r/35664/#comment141161> Now we have a `names()` method on Resources, so we don't need to manually construct the name set here anymore :) src/master/allocator/sorter/drf/sorter.cpp (line 302) <https://reviews.apache.org/r/35664/#comment141168> Maybe we want to use the new `get` method which returns a `Resources` filtered by the provided name? Might make the comment above more obvious, that we have to loop over multiple `Resource` objects for each name, if we move it down, like this: ``` foreach (const string& name, total.scalars.names()) { double total = 0.0; // NOTE: Scalar resources may be spread across multiple // 'Resource' objects. E.g. persistent volumes. foreach (const Resource& resource, total.scalars.get(name)) { total += resource.scalar().value(); } if (total > 0.0) { ... } } ``` Ditto for computing `allocation` below. Seems a bit non-intuitive that the call to `get` is aggregating across `Resource` objects. One would guess that it is just pulling out the scalar by that name. - Ben Mahler On June 19, 2015, 9:09 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35664/ > ----------------------------------------------------------- > > (Updated June 19, 2015, 9:09 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Bugs: MESOS-2891 > https://issues.apache.org/jira/browse/MESOS-2891 > > > Repository: mesos > > > Description > ------- > > Improved the performance of DRF sorter by caching the scalars. > > > Diffs > ----- > > src/master/allocator/sorter/drf/sorter.hpp > 35dc1a4d0b5e61b26a07c2c53583d75896aff27c > src/master/allocator/sorter/drf/sorter.cpp > c5f4caf68aff26a9d4809eff11df98d0137aa401 > > Diff: https://reviews.apache.org/r/35664/diff/ > > > Testing > ------- > > make check > > With this patch: > > [==========] Running 6 tests from 1 test case. > [----------] Global test environment set-up. > [----------] 6 tests from SlaveCount/HierarchicalAllocator_BENCHMARK_Test > [ RUN ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/0 > Added 1000 slaves in 668.950475ms > [ OK ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/0 (844 > ms) > [ RUN ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/1 > Added 5000 slaves in 3.335592974secs > [ OK ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/1 (4244 > ms) > [ RUN ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/2 > Added 10000 slaves in 6.675837646secs > [ OK ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/2 (8527 > ms) > [ RUN ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/3 > Added 20000 slaves in 13.411382999secs > [ OK ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/3 > (17177 ms) > [ RUN ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/4 > Added 30000 slaves in 20.012000768secs > [ OK ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/4 > (25567 ms) > [ RUN ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/5 > Added 50000 slaves in 33.377255617secs > [ OK ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/5 > (42960 ms) > [----------] 6 tests from SlaveCount/HierarchicalAllocator_BENCHMARK_Test > (99346 ms total) > > [----------] Global test environment tear-down > [==========] 6 tests from 1 test case ran. (99452 ms total) > [ PASSED ] 6 tests. > > Baseline (pre MESOS-2373) > > [==========] Running 6 tests from 1 test case. > [----------] Global test environment set-up. > [----------] 6 tests from SlaveCount/HierarchicalAllocator_BENCHMARK_Test > [ RUN ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/0 > Added 1000 slaves in 492.832376ms > [ OK ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/0 (602 > ms) > [ RUN ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/1 > Added 5000 slaves in 2.432903553secs > [ OK ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/1 (2991 > ms) > [ RUN ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/2 > Added 10000 slaves in 4.866961208secs > [ OK ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/2 (6014 > ms) > [ RUN ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/3 > Added 20000 slaves in 9.736516799secs > [ OK ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/3 > (12067 ms) > [ RUN ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/4 > Added 30000 slaves in 14.599861839secs > [ OK ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/4 > (18011 ms) > [ RUN ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/5 > Added 50000 slaves in 24.30015991secs > [ OK ] SlaveCount/HierarchicalAllocator_BENCHMARK_Test.AddSlave/5 > (30097 ms) > [----------] 6 tests from SlaveCount/HierarchicalAllocator_BENCHMARK_Test > (69784 ms total) > > [----------] Global test environment tear-down > [==========] 6 tests from 1 test case ran. (69820 ms total) > [ PASSED ] 6 tests. > > > Thanks, > > Jie Yu > >
