----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50172/#review143251 -----------------------------------------------------------
Ship it! The commit summary and description seem to not describe the intent of this patch, which was to fix the issue of allocating ports outside of the agent's port range. E.g. commit message: ```` Fixed an issue with port allocation in the sorter benchmarks. Previously the port allocation was not aware of the port range that the agent had available. This updates the allocation of fragmented port ranges to fragment *from* the range of ports available on the agent. ``` src/tests/sorter_tests.cpp (lines 562 - 566) <https://reviews.apache.org/r/50172/#comment209028> Let's put this example up in the documentation for this function? Then the fragmentation technique is clear to the caller and anyone reading the documentation can figure it out. src/tests/sorter_tests.cpp (lines 567 - 581) <https://reviews.apache.org/r/50172/#comment209029> Maybe the following is simpler? ``` for (size_t i = 0; i < numRanges; ++i) { Value::Range* range = ranges.add_range(); range->set_begin(bounds.begin() + (i * 2)); range->set_end(range->begin()); } // Make sure the last range covers the end of the bounds. if (!ranges.range().empty()) { ranges.mutable_range()->rbegin()->set_end(bounds.end()); } ``` src/tests/sorter_tests.cpp (line 570) <https://reviews.apache.org/r/50172/#comment209030> Why start instead of begin? src/tests/sorter_tests.cpp (line 587) <https://reviews.apache.org/r/50172/#comment209027> We should probably do a s/make/create/ to be consistent with the similar helpers that we had looked at in your other review? - Benjamin Mahler On July 21, 2016, 1:43 a.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50172/ > ----------------------------------------------------------- > > (Updated July 21, 2016, 1:43 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > This patch added three new functions to sorter benchmark test to > generate a \`ports\` resource which falls into a specifed ports > range bounds. > > 1) makePorts: Creates a "ports(*)" resource for the given ranges. > 2) fragment: Fragments the given range bounds into a number of > subranges. > 3) makeRange: Returns the 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 > >
