----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51027/#review146094 -----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp (line 216) <https://reviews.apache.org/r/51027/#comment212548> Change the comment to: ``` // Allocate resources from the allocation candidates. ``` src/master/allocator/mesos/hierarchical.hpp (lines 219 - 220) <https://reviews.apache.org/r/51027/#comment212546> Kill this. We don't define this method anymore. src/master/allocator/mesos/hierarchical.hpp (lines 222 - 223) <https://reviews.apache.org/r/51027/#comment212547> We need to change this per the comments below. src/master/allocator/mesos/hierarchical.hpp (lines 382 - 383) <https://reviews.apache.org/r/51027/#comment212516> We should add comments for these members. src/master/allocator/mesos/hierarchical.cpp (lines 782 - 786) <https://reviews.apache.org/r/51027/#comment212538> Here it's appropriate, particularly given the comment above, that we run allocation synchronously. Add a comment similar to the one in `setQuota` on why we do it synchronously. src/master/allocator/mesos/hierarchical.cpp (line 1107) <https://reviews.apache.org/r/51027/#comment212539> s/explicitly/synchronously/ src/master/allocator/mesos/hierarchical.cpp (lines 1109 - 1113) <https://reviews.apache.org/r/51027/#comment212535> Here it's appropriate, particularly given the comment above, that we run allocation synchronously. src/master/allocator/mesos/hierarchical.cpp (line 1136) <https://reviews.apache.org/r/51027/#comment212540> s/explicitly/synchronously/ src/master/allocator/mesos/hierarchical.cpp (lines 1138 - 1142) <https://reviews.apache.org/r/51027/#comment212536> Ditto. src/master/allocator/mesos/hierarchical.cpp (lines 1177 - 1181) <https://reviews.apache.org/r/51027/#comment212537> Ditto. src/master/allocator/mesos/hierarchical.cpp (line 1209) <https://reviews.apache.org/r/51027/#comment212485> We need some comments here. ``` We run the allocation synchronously here instead of dispatching it so we don't delay a batched allocation if the allocator is backed up. TODO: This still does not guarantee that at least one allocation is run per batch interval. Consider having the allocator run allocations synchronously when handling events if no allocation is run for the past batch interval. ``` src/master/allocator/mesos/hierarchical.cpp (line 1214) <https://reviews.apache.org/r/51027/#comment212541> src/master/allocator/mesos/hierarchical.cpp (line 1227) <https://reviews.apache.org/r/51027/#comment212522> Here we can just rename the method we call as: `_allocate()`. Note that now that we keep track of a member `allocationCandidates` we don't have to pass them around. If we don't pass them around, we can't use the argument as what distinguishes the overloaded `allocate()` methods. I think it's pretty natural to have `void _allocate();` as a private helper. src/master/allocator/mesos/hierarchical.cpp (line 1582) <https://reviews.apache.org/r/51027/#comment212544> Similarly here, we don't need the argument in `deallocate()`. - Jiang Yan Xu On Aug. 17, 2016, 11:12 a.m., Jacob Janco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51027/ > ----------------------------------------------------------- > > (Updated Aug. 17, 2016, 11:12 a.m.) > > > Review request for mesos, Benjamin Mahler, James Peach, Klaus Ma, and Jiang > Yan Xu. > > > Bugs: MESOS-3157 > https://issues.apache.org/jira/browse/MESOS-3157 > > > Repository: mesos > > > Description > ------- > > - Triggered allocations dispatch allocate() only > if there is no pending allocation in the queue. > - Allocation candidates are accumulated and only > cleared when enqueued allocations are processed. > > - carrying over work from https://reviews.apache.org/r/41658/ and added the > previous reviewers > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 > src/master/allocator/mesos/hierarchical.cpp > 234ef98529964a0b6d3f132426a6c8ccbb1263ee > > Diff: https://reviews.apache.org/r/51027/diff/ > > > Testing > ------- > > make check > > note: check without filters depends on https://reviews.apache.org/r/51028 > > With new benchmark https://reviews.apache.org/r/49617: > Sample output without 51027: > [ RUN ] > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22 > Using 10000 agents and 3000 frameworks > Added 3000 frameworks in 57251us > Added 10000 agents in 3.21345353333333mins > allocator settled after 1.61236038333333mins > [ OK ] > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22 > (290578 ms) > > Sample output with 51027: > [ RUN ] > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22 > Using 10000 agents and 3000 frameworks > Added 3000 frameworks in 39817us > Added 10000 agents in 3.22860541666667mins > allocator settled after 25.525654secs > [ OK ] > SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.FrameworkFailover/22 > (220137 ms) > > > Thanks, > > Jacob Janco > >
