mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 50172: Made `ports` resource configurable in sorter benchmark test.
Date Tue, 19 Jul 2016 03:13:50 GMT

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




src/tests/sorter_tests.cpp (lines 504 - 506)
<https://reviews.apache.org/r/50172/#comment208321>

    How about these functions:
    
    ```
    // Creates a "ports(*)" resource for the given ranges.
    Resources makePorts(const Ranges& ranges);
    
    // Fragments the given range into a number of subranges.
    // Returns an Error if 'numRanges' is too large.
    Try<Ranges> fragment(const Range& range, size_t numRanges);
    
    Range makeRange(uint64_t begin, uint64_t end);
    ```
    
    The caller will call `makePorts` with the result of `fragment`ing the range. This way,
the fragment function doesn't have to bundle the logic of



src/tests/sorter_tests.cpp (lines 505 - 506)
<https://reviews.apache.org/r/50172/#comment208314>

    The typical format is to wrap the arguments:
    
    ```
    static Try<Resource> fragment(
        const ::mesos::Value::Range& bounds,
        size_t numRanges)
    ```



src/tests/sorter_tests.cpp (lines 510 - 514)
<https://reviews.apache.org/r/50172/#comment208324>

    Based on this comment, the formula is wrong?
    
    10 / 2 + 1 = 6 but the max is 5?



src/tests/sorter_tests.cpp (line 515)
<https://reviews.apache.org/r/50172/#comment208325>

    Can you flip this check? It seems to read opposite of how it should: numRanges > maxRanges



src/tests/sorter_tests.cpp (lines 516 - 520)
<https://reviews.apache.org/r/50172/#comment208322>

    Hm.. maybe we can just keep this simple, in general we don't print the caller-available
information (bounds and numRanges in this case). Something like:
    
    "Requested more distinct ranges than possible"



src/tests/sorter_tests.cpp (lines 534 - 544)
<https://reviews.apache.org/r/50172/#comment208323>

    It doesn't seem like you need to special case step == 1, is this case possible? I'm pretty
confused by this code, I would expect the following behavior from fragment:
    
    ```
    [1-10], 1 -> [1-10]
    [1-10], 2 -> [1-4,6-10]
    [1-10], 3 -> [1-3,5-6,8-10]
    [1-10], 4 -> [1-2,4-5,7-8,10-10] // Multiple choices are possible
    [1-10], 5 -> [1-1,3-3,5-5,7-7,9-10] // Multiple choices are possible
    ```
    
    Or something very simple to implement and still easy to understand, like picking off single
ranges from the front always, this one has no ambiguity like the above:
    
    ```
    [1-10], 1 -> [1-10]
    [1-10], 2 -> [1-1,3-10]
    [1-10], 3 -> [1-1,3-3,5-10]
    [1-10], 4 -> [1-1,3-3,5-5,7-10]
    [1-10], 5 -> [1-1,3-3,5-5,7-7,9-10]
    ```
    
    But yours does the following from what I can tell:
    
    ```
    [1-10], 1 -> [1-2]
    [1-10], 2 -> [1-2,6-7]
    [1-10], 3 -> [1-2,4-5,7-8]
    [1-10], 4 -> [1-2,3-4,5-6,7-8] -> [1-8]       // Wrong?
    [1-10], 5 -> [1-2,3-4,5-6,7-8,9-10] -> [1-10] // Wrong?
    ```
    
    This looks wrong..? I'd say let's just implement the second suggestion I made here as
it's simple, and it is easy to understand.



src/tests/sorter_tests.cpp (line 564)
<https://reviews.apache.org/r/50172/#comment208309>

    There is no "ports" associated with this function, since it's just a Value::Range, not
a Resource. How about just removing this sentence?



src/tests/sorter_tests.cpp (line 632)
<https://reviews.apache.org/r/50172/#comment208310>

    Any reason you didn't use s/.get()./->/ here?



src/tests/sorter_tests.cpp (line 636)
<https://reviews.apache.org/r/50172/#comment208312>

    Why is it ok to use [1-10] when the agent only has [31000-32000]? Seems wrong?



src/tests/sorter_tests.cpp (line 638)
<https://reviews.apache.org/r/50172/#comment208311>

    Ditto here, why not use ->?


- Benjamin Mahler


On July 19, 2016, 1:39 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50172/
> -----------------------------------------------------------
> 
> (Updated July 19, 2016, 1:39 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch added two new functions to sorter benchmark test to
> generate a `ports` resource which falls into a specifed ports
> range bounds.
> 
> 1) fragment: Returns a "ports" resource with the number of ranges.
> 2) makeRange: Returns the "ports" range bounds.
> 
> 
> Diffs
> -----
> 
>   src/tests/sorter_tests.cpp b0e5ef8a55bbcca3553a221bd5691a9c801a04f7 
> 
> Diff: https://reviews.apache.org/r/50172/diff/
> 
> 
> Testing
> -------
> 
> ```
> ./bin/mesos-tests.sh --benchmark --gtest_filter="AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1"
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test
> [ RUN      ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1
> Using 1000 agents and 50 clients
> Added 50 clients in 872us
> Added 1000 agents in 26457us
> Added allocations for 1000 agents in 79697us
> Full sort of 50 clients took 1321us
> No-op sort of 50 clients took 28us
> [       OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1 (115 ms)
> [----------] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test (115 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (133 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>


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