Re: Review Request 68591: Added allocator benchmark for non-homogeneous framework profiles.

[Meng Zhu <mz...@mesosphere.io>]

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



Can you post the test result?


src/tests/hierarchical_allocator_benchmarks.cpp
Lines 290 (patched)
<https://reviews.apache.org/r/68591/#comment292163>

    "measures the allocator performance" -- can you elaborate?
    
    One of the pain points I got from reading the benchmark code is that it is not clear what is being tested and what is the figure of merits.
    
    Let's make a good model here and future readers will be grateful :)
    
    Please also update the commit description along with it.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 357-358 (patched)
<https://reviews.apache.org/r/68591/#comment292166>

    Move this TODO to the frameworkProfile struct.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 360 (patched)
<https://reviews.apache.org/r/68591/#comment292167>

    Use `at` for readonly access



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 365-370 (patched)
<https://reviews.apache.org/r/68591/#comment292168>

    I suggest moving the `--` and `++` to the loop body for readibility.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 367 (patched)
<https://reviews.apache.org/r/68591/#comment292170>

    "&" --> "&&"
    
    I think the compiler will complain in this case. Did you forget to send out the most recent commit?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 379 (patched)
<https://reviews.apache.org/r/68591/#comment292175>

    s/process:://
    
    Also, why we need to `settle` here? Can you add some comments?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 382 (patched)
<https://reviews.apache.org/r/68591/#comment292169>

    why do we need to settle here? Can you add some comments?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 389-397 (patched)
<https://reviews.apache.org/r/68591/#comment292174>

    The logic here is surprising. When we advance the clock, it will generate offers but since we haven't processed anything yet, I do not expect to see the "cout" log line.
    
    Also, I believe if we fix the pause bug I mentioned in the previous patch, the first log line will look like "Launched 0 tasks", how about:
    
    while() {
      advance clock
      settle
      process the result
      cout the result
    }


- Meng Zhu


On Aug. 31, 2018, 3:14 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68591/
> -----------------------------------------------------------
> 
> (Updated Aug. 31, 2018, 3:14 p.m.)
> 
> 
> Review request for mesos, Meng Zhu and Till Toenshoff.
> 
> 
> Bugs: MESOS-9187
>     https://issues.apache.org/jira/browse/MESOS-9187
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This tests measures allocation performance with non-uniform framework
> characteristics. Each framework profile launches a different number of
> tasks with different task sizes.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68591/diff/1/
> 
> 
> Testing
> -------
> 
> Make check with the new test.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>