----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51027/#review162229 -----------------------------------------------------------
Fix it, then Ship it! The logic looks good to me, just a few minor comments. src/master/allocator/mesos/hierarchical.hpp (lines 221 - 229) <https://reviews.apache.org/r/51027/#comment233533> I think it's sufficient here to just say: ``` // Allocate resources from the specified agents. ``` The rest of the comment seems to be a re-iteration of the logic inside the implementations. Do we need to say that the other two call into this? Or how exactly we do the batching? IMHO it's likely to go stale over time and the reader can read the implementation to see exactly how it works. We could add something like: ``` // Allocate any allocatable resources from all known agents, // the allocation is deferred and batched with other // allocation requests. process::Future<Nothing> allocate(); ``` But it seems unnecessary given the caller just needs to understand that it is asynchronous (i.e. returns a Future). src/master/allocator/mesos/hierarchical.hpp (lines 232 - 236) <https://reviews.apache.org/r/51027/#comment233536> Run seems a bit generic. How about `_allocate()` and `__allocate()` to make it clear it's the first `_allocate()` continuation? src/master/allocator/mesos/hierarchical.cpp (lines 1267 - 1274) <https://reviews.apache.org/r/51027/#comment233544> This change introduces an additional trip through the allocator's queue after the allocation completes and before the delay is called. This would avoid it: ``` auto pid = self(); // TODO: Use process::loop for the allocation loop. allocate() .onAny([pid]() { delay(allocationInterval, self(), &Self::batch); } ``` Not sure if this was intentional or not. src/master/allocator/mesos/hierarchical.cpp (lines 1301 - 1305) <https://reviews.apache.org/r/51027/#comment233545> This seems pretty self explanatory, no need for this comment IMHO. The example is likely to go stale. src/master/allocator/mesos/hierarchical.cpp (lines 1308 - 1309) <https://reviews.apache.org/r/51027/#comment233546> Why the newline? - Benjamin Mahler On Jan. 12, 2017, 6:55 p.m., Jacob Janco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51027/ > ----------------------------------------------------------- > > (Updated Jan. 12, 2017, 6:55 p.m.) > > > Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus > Ma, and Jiang Yan Xu. > > > Bugs: MESOS-6904 > https://issues.apache.org/jira/browse/MESOS-6904 > > > 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. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > a6424d624864155e1c87a28a63b784512c5c8722 > src/master/allocator/mesos/hierarchical.cpp > 91b1ec43940a788459f045ca4a4b82d4e8373bca > > Diff: https://reviews.apache.org/r/51027/diff/ > > > Testing > ------- > > make check with the filters below > > Broken tests: > - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028 > - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028 > - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028 > - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028 > - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in > 51028 > - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534 > - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), > fix in 51621 > > > Thanks, > > Jacob Janco > >
