mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 69821: Enforced minimal allocatable resources in the hierarchical allocator.
Date Tue, 05 Feb 2019 16:38:50 GMT


> On Feb. 4, 2019, 9:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2041-2042 (original), 2075-2076 (patched)
> > <https://reviews.apache.org/r/69821/diff/5/?file=2123824#file2123824line2084>
> >
> >     Hm.. isn't the framework capability stripping messing with our break condition?

Hmm, it seems like it, yes. This should be an issue already now where we `break` in even more
scenarios. This should be fixed outside of this change. I created https://issues.apache.org/jira/browse/MESOS-9554.

Dropping here.


> On Feb. 4, 2019, 9:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2058-2060 (original), 2092-2094 (patched)
> > <https://reviews.apache.org/r/69821/diff/5/?file=2123824#file2123824line2101>
> >
> >     We lost the comment here about why it's safe to break? It still seems relevant

Hmm, not sure what you are after. My patch `continue`s instead of `break`s. The way the code
is structured now we cannot `break` here, but instead must iterate over all frameworks. We
could `break` if we'd e.g., made `allocatable` independent of framework settings like before
(e.g., by computing a minimal allocatable resources given all framework information), but
we'd likely reject many allocation decision last minute that way in the same spot where we
currently check filters. The code as proposed here looks simpler to me.

Can we drop this?


> On Feb. 4, 2019, 9:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2064-2070 (original), 2096-2102 (patched)
> > <https://reviews.apache.org/r/69821/diff/5/?file=2123824#file2123824line2107>
> >
> >     I'm left confused by the two checks now that they both continue, and I think
the comment is now inaccurate and confusing? It is written based on break vs continue

I went over all the comments around resource emptiness and `allocatable` checks.


> On Feb. 4, 2019, 9:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2090-2091 (original), 2122-2125 (patched)
> > <https://reviews.apache.org/r/69821/diff/5/?file=2123824#file2123824line2133>
> >
> >     It seems more readable if this is a member function of `Framework`

In that case we'd have to pass in the allocator's `minAllocatableResources` (either here,
or if they could change here). I'd suggest to keep this as is.

Dropping for now, feel free to reopen if you think this requires more discussion.


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69821/#review212523
-----------------------------------------------------------


On Feb. 5, 2019, 5:38 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69821/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2019, 5:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9523
>     https://issues.apache.org/jira/browse/MESOS-9523
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch modifies the hierarchical allocator to take
> framework-specified minimal allocatable resources into account.
> 
> While previously the allocator was inspecting the minimal allocatable
> resources specified in its global options, it can now also inspects
> framework-specific resource requirements. With that frameworks can e.g.,
> configure resource requirements above the default minimal allocatable
> resource, or opt into receiving resources considered too small to be
> allocatable by the allocator in its default behavior.
> 
> For that we change the hierarchical allocator's `allocatable` function
> to be framework and role-specific. As that does in some places not allow
> us to abort iterating over candidate resource consumers like before an
> additional check on whether any resources are left in an allocation
> cycle is added as a break condition.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.hpp 1420c2638786d85f7b04379e5d79e59990c3e6cf

>   src/master/allocator/mesos/hierarchical.cpp bb9a9c95979f36c0564af5b3babb1c43077a363b

> 
> 
> Diff: https://reviews.apache.org/r/69821/diff/8/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> File Attachments
> ----------------
> 
> Ratio new/old timings
>   https://reviews.apache.org/media/uploaded/files/2019/01/31/d76189de-8882-4aff-956b-090dab729358__new_over_old.png
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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