----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69821/#review212679 -----------------------------------------------------------
Looks good, just some minor comments. I suppose you'll need to rebase this off my updated patch (which I hope to get to today). src/master/allocator/mesos/hierarchical.cpp Lines 1886-1888 (patched) <https://reviews.apache.org/r/69821/#comment298529> Ditto the comment left below for the equivalent check in the second loop src/master/allocator/mesos/hierarchical.cpp Lines 2110-2112 (patched) <https://reviews.apache.org/r/69821/#comment298528> Hm.. why bother with this? Seems correct without it and this doesn't seem to save much in terms of cost? It also seems like an orthogonal change? It seems ok to add this one and the one above, but seems cleaner to do that in a follow up change that measures the performance impact? The quota one does seem to potentially save some expensive work src/master/allocator/mesos/hierarchical.cpp Line 2125 (original), 2138-2141 (patched) <https://reviews.apache.org/r/69821/#comment298527> Let's not bother with this additional check? If `toAllocate` is empty, we'll continue from the check below (allocatable always is false for no resources), followed by the top of this loop breaking because there's nothing left for the role. So this only saves having to execute a few lines at the top of the loop? Doesn't seem worth it? - Benjamin Mahler On Feb. 8, 2019, 11:32 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69821/ > ----------------------------------------------------------- > > (Updated Feb. 8, 2019, 11:32 a.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/9/ > > > 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 > >
