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

Reply via email to