> On Aug. 23, 2016, 9:26 a.m., Alexander Rukletsov wrote: > > src/master/allocator/mesos/hierarchical.cpp, lines 1194-1195 > > <https://reviews.apache.org/r/51027/diff/2/?file=1481820#file1481820line1194> > > > > `delay` already contains `dispatch`, so under "synchronously" you > > actually mean double dispatch. > > Jiang Yan Xu wrote: > I originally suggested putting this comment inside `batch()` and directly > above `allocate()` so it's more clear what `synchronously` applies to: > calling `allocate()` without dispatching. > > *delay* in this sentence doesn't mean the `delay()` call but in the > literal sense. To make it more clear, how about we say: > > ``` > // We run the allocation synchronously here instead of dispatching it > **again** so > // a batched allocation **doesn't lag behind further** if the allocator > is backed up. > ``` > > Note the asterisks are just to emphasize the changes.
This reads better! > On Aug. 23, 2016, 9:26 a.m., Alexander Rukletsov wrote: > > src/master/allocator/mesos/hierarchical.cpp, lines 276-279 > > <https://reviews.apache.org/r/51027/diff/2/?file=1481820#file1481820line276> > > > > Probably extract this snippet in a function, e.g. > > `conditionalAllocate()`? > > Jiang Yan Xu wrote: > I thought about it but was struggling to find a short and clear method > name. So to describe the function in a full sentense it's "dispatch an > allocate() call if the condition `!allocationPending` is met". I think > `conditionalAllocate()` is OK but not great, it's not clear what the > condition is and not clear about the dispatch. I agree it's worth doing if we > can abstract this out without needing to explain what each line of a 4-line > method is doing in the comment... Let's focus on _why_ instead of _how_. You want to ensure that one and only one event allocation happens after the event. Dispatch+flag is _how_ you achieve this, which may change in future. Here are some suggestions: - `ensureAllocate` - `ensureAllocation` - `allocateOnce` Side note: "unique dispatch", i.e. a dispatch that succeeds only if there are no identical messages in the actor's mailbox, looks like something we may need in other places. Maybe we should consider adding such functionality into libprocess. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51027/#review146499 ----------------------------------------------------------- On Aug. 23, 2016, 8:49 a.m., Jacob Janco wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51027/ > ----------------------------------------------------------- > > (Updated Aug. 23, 2016, 8:49 a.m.) > > > Review request for mesos, Benjamin Mahler, Guangya Liu, 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. > - Batched allocations are handled synchronously. > > - Carrying over work from https://reviews.apache.org/r/41658/ and added the > previous reviewers > - Specifically, this patch introduces the boolean flag pendingAllocation, > which when set on event > triggered allocations, will prevent additional no-op allocations: the flag > is cleared when > the enqueued allocation is processed, subsequent event triggered > allocations will update a set > of allocation candidates rather than dispatching an additional allocate(). > > > 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 > >
