mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 49616: Add suppression benchmark.
Date Wed, 06 Jul 2016 23:19:13 GMT

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




src/tests/hierarchical_allocator_tests.cpp (lines 3572 - 3575)
<https://reviews.apache.org/r/49616/#comment206488>

    We should use the templatized test fixture.
    
    s/TEST_F/TEST_P/ and use `GetParam()`.



src/tests/hierarchical_allocator_tests.cpp (line 3587)
<https://reviews.apache.org/r/49616/#comment206502>

    We actually don't need this count and it's misleading: it's not the count of offers. It's
the count of frameworks that are receiving offers in this allocation round.
    
    To get the offer count we can use `offers.size()`.



src/tests/hierarchical_allocator_tests.cpp (lines 3609 - 3616)
<https://reviews.apache.org/r/49616/#comment206506>

    We can simply:
    
    ```
    cout << "Using " << slaveCount << " agents and "
         << frameworkCount << " frameworks" << endl;
    
    vector<SlaveInfo> slaves(slaveCount);
    vector<FrameworkInfo> frameworks(frameworkCount);
    ```



src/tests/hierarchical_allocator_tests.cpp (line 3648)
<https://reviews.apache.org/r/49616/#comment206492>

    Nit: `s/count/i/`, we don't seem to use `count` in any special way and `i` is the standard
variable name in a for loop.



src/tests/hierarchical_allocator_tests.cpp (lines 3650 - 3651)
<https://reviews.apache.org/r/49616/#comment206494>

    Comment:
    
    // Recover resources with no filters because we want to test the effect of suppression
alone.



src/tests/hierarchical_allocator_tests.cpp (line 3654)
<https://reviews.apache.org/r/49616/#comment206495>

    Comment:
    
    // Wait for all declined offers to be processed so the stopwatch only measures the allocation
time.



src/tests/hierarchical_allocator_tests.cpp (line 3660)
<https://reviews.apache.org/r/49616/#comment206496>

    This `{` shouldn't be necessary.



src/tests/hierarchical_allocator_tests.cpp (line 3669)
<https://reviews.apache.org/r/49616/#comment206499>

    Instead of calling it a `round` we could be more explicit:
    
    "allocate() took 1.491969secs to make 2000 offers with 103 out of 200 frameworks suppressing
offers"


- Jiang Yan Xu


On July 5, 2016, 2:48 p.m., Jacob Janco wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49616/
> -----------------------------------------------------------
> 
> (Updated July 5, 2016, 2:48 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5781
>     https://issues.apache.org/jira/browse/MESOS-5781
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> - Useful for high framework clusters which carry
>   many suppressed frameworks that are considered
>   during allocation.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp 0498cd5e54b0e4b87a767585a77699653aa52179

> 
> Diff: https://reviews.apache.org/r/49616/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message