----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49571/#review144048 -----------------------------------------------------------
I would suggest you take a look at https://reviews.apache.org/r/49616 to see more comments from Ben Mahler. src/tests/hierarchical_allocator_tests.cpp (line 84) <https://reviews.apache.org/r/49571/#comment210010> new line here src/tests/hierarchical_allocator_tests.cpp (line 252) <https://reviews.apache.org/r/49571/#comment210011> new line here src/tests/hierarchical_allocator_tests.cpp (line 265) <https://reviews.apache.org/r/49571/#comment210012> new line here src/tests/hierarchical_allocator_tests.cpp (lines 3483 - 3484) <https://reviews.apache.org/r/49571/#comment210013> s/unsigned/size_t src/tests/hierarchical_allocator_tests.cpp (lines 3487 - 3489) <https://reviews.apache.org/r/49571/#comment210014> Do we really need this as we already using `Clock::pause();`, I think that you can just kill this. I'm planning to do a clean up for other benchmark tests, such as `DeclineOffers`, `ResourceLabels` etc. src/tests/hierarchical_allocator_tests.cpp (line 3498) <https://reviews.apache.org/r/49571/#comment210015> 1) The braces go on the next line. 2) How about s/OfferedResources/Allocation/ ? This would match the pattern used in the HierarchicalAllocatorTestBase. src/tests/hierarchical_allocator_tests.cpp (lines 3510 - 3513) <https://reviews.apache.org/r/49571/#comment210017> first and second is not clear, what about the following? ``` foreachpair (const SlaveID& slaveId, const Resources& r, resources) { Offer offer; offer.frameworkId = frameworkId; offer.slaveId = slaveId; offer.resources = r; offers.push_back(std::move(offer)); } ``` I would also prefer that you update line 3504 to ``` vector<Allocation> allocations; ``` Then the first code section will be `allocations` but not `offers` since offers is a master concept but not allocator concept. src/tests/hierarchical_allocator_tests.cpp (line 3515) <https://reviews.apache.org/r/49571/#comment210016> The `offerCount` was already removed from other benchmark tests and we are using `offers.size()` instead. src/tests/hierarchical_allocator_tests.cpp (line 3518) <https://reviews.apache.org/r/49571/#comment210019> We tend to declare variables close to where they are needed, seems this can be moved down to where we create the slaves below? src/tests/hierarchical_allocator_tests.cpp (line 3540) <https://reviews.apache.org/r/49571/#comment210021> I prefer rename this as variable as `allocation` src/tests/hierarchical_allocator_tests.cpp (line 3549) <https://reviews.apache.org/r/49571/#comment210022> s/unsigned/size_t src/tests/hierarchical_allocator_tests.cpp (lines 3550 - 3552) <https://reviews.apache.org/r/49571/#comment210020> move this out of the loop src/tests/hierarchical_allocator_tests.cpp (line 3578) <https://reviews.apache.org/r/49571/#comment210023> we actually have one more port allocated but not `a random port`, and also the allocator do not know task, I would suggest that we only keep `// Add some used resources on each slave.` or just remove the comments here. src/tests/hierarchical_allocator_tests.cpp (line 3589) <https://reviews.apache.org/r/49571/#comment210024> s/unsigned/size_t src/tests/hierarchical_allocator_tests.cpp (line 3590) <https://reviews.apache.org/r/49571/#comment210025> Can you avoid auto here? It would be more readable to have the type, and we only use 'auto' when the type is very verbose or is clear using only local reasonsing src/tests/hierarchical_allocator_tests.cpp (line 3597) <https://reviews.apache.org/r/49571/#comment210026> kill this src/tests/hierarchical_allocator_tests.cpp (line 3599) <https://reviews.apache.org/r/49571/#comment210027> kill this brace, I think it is not needed src/tests/hierarchical_allocator_tests.cpp (line 3604) <https://reviews.apache.org/r/49571/#comment210028> s/background/batch - Guangya Liu On 七月 28, 2016, 10:46 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49571/ > ----------------------------------------------------------- > > (Updated 七月 28, 2016, 10:46 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-5771 > https://issues.apache.org/jira/browse/MESOS-5771 > > > Repository: mesos > > > Description > ------- > > Allocations test has the following configurations: > (1) REGULAR: Offers from every slave have regular resources. > (2) SHARED: Offers from every slave include a shared resource. > (3) REGULAR: Offers from every alternate slave contain only regular > resources; and offers from every other alternate slave contains > a shared resource. > > > Diffs > ----- > > src/tests/hierarchical_allocator_tests.cpp > bb6947fcecb5b78047e98d10fc1278c612a69548 > > Diff: https://reviews.apache.org/r/49571/diff/ > > > Testing > ------- > > All tests passed. > > Allocations benchmark test results > ================================== > Support of shared resources has a small impact on runtime performance in > allocations. Also, there is no visible impact in performance when shared > resources are added in the tests. > > With the patch (and no shared resources) > ---------------------------------------- > round 0 allocate took 3.19704secs to make 200 offers > round 50 allocate took 3.240605secs to make 200 offers > round 100 allocate took 3.227024secs to make 200 offers > round 150 allocate took 3.225281secs to make 200 offers > round 199 allocate took 3.26036secs to make 200 offers > > With the patch (and shared resources on all agents) > --------------------------------------------------- > round 0 allocate took 3.279115secs to make 200 offers > round 50 allocate took 3.273396secs to make 200 offers > round 100 allocate took 3.278509secs to make 200 offers > round 150 allocate took 3.275959secs to make 200 offers > round 199 allocate took 3.278151secs to make 200 offers > > With the patch (and shared resources on alternate agents) > --------------------------------------------------------- > round 0 allocate took 3.251739secs to make 200 offers > round 50 allocate took 3.263777secs to make 200 offers > round 100 allocate took 3.263079secs to make 200 offers > round 150 allocate took 3.263114secs to make 200 offers > round 199 allocate took 3.236228secs to make 200 offers > > Based on HEAD, with all regular resources (no shared resources in HEAD > supported) > --------------------------------------------------------------------------------- > round 0 allocate took 2.925681secs to make 200 offers > round 50 allocate took 2.922036secs to make 200 offers > round 100 allocate took 2.909337secs to make 200 offers > round 150 allocate took 2.914093secs to make 200 offers > round 199 allocate took 2.923762secs to make 200 offers > > > Thanks, > > Anindya Sinha > >
