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