mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 51027: Track allocation candidates to bound allocator.
Date Wed, 24 Aug 2016 10:38:12 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message