----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69821/#review212523 -----------------------------------------------------------
Can you split this change apart so that I can review more easily and we can land it faster? (1) The plumbing and storage of the information: this is an easy change that doesn't require much thought, it looks good and can be shipped. (2) The update to the allocation / allocatable logic: this is a harder change that warrants careful thought about the existing allocation code and I'd like to review it in isolation from the other changes in the current patch. Left some comments in the second stage that also apply to the first stage. (3) Tests: also a rather easy change and would be good to ship in isolation. Haven't review this yet (would prefer to review in isolation since the review load is too high in this patch). src/master/allocator/mesos/hierarchical.cpp Lines 2041-2042 (original), 2075-2076 (patched) <https://reviews.apache.org/r/69821/#comment298332> Hm.. isn't the framework capability stripping messing with our break condition? src/master/allocator/mesos/hierarchical.cpp Lines 2058-2060 (original), 2092-2094 (patched) <https://reviews.apache.org/r/69821/#comment298329> We lost the comment here about why it's safe to break? It still seems relevant src/master/allocator/mesos/hierarchical.cpp Lines 2064-2070 (original), 2096-2102 (patched) <https://reviews.apache.org/r/69821/#comment298330> 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 src/master/allocator/mesos/hierarchical.cpp Lines 2090-2091 (original), 2122-2125 (patched) <https://reviews.apache.org/r/69821/#comment298328> It seems more readable if this is a member function of `Framework` src/master/allocator/mesos/hierarchical.cpp Lines 2455 (patched) <https://reviews.apache.org/r/69821/#comment298327> We already have the `Framework` in hand whenever we call this (and that's what I would expect), so can we simplify this? If we pass the `Framework` we can avoid having the contains guard and framework lookup logic. src/master/framework.cpp Lines 504 (patched) <https://reviews.apache.org/r/69821/#comment298325> Ah, here it is, should be in the earlier review? - Benjamin Mahler On Feb. 4, 2019, 8:37 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69821/ > ----------------------------------------------------------- > > (Updated Feb. 4, 2019, 8:37 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 > src/tests/hierarchical_allocator_tests.cpp > cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee > > > Diff: https://reviews.apache.org/r/69821/diff/6/ > > > 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 > >
