----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69821/#review212421 -----------------------------------------------------------
The commit description makes it sound like the filter is using the **union** of the flag and the per-framework/role override: > This set is populated with any minimal allocatable resources specified in the > allocator's options and any framework's minimal allocatable resources. However, it should be an override (which is what the patch correctly does AFAICT). Probably, just using "override" in the explanation would make this very clear to the reader. IMO, we only really need the first paragraph of the description: > This patch modifies the hierarchical allocator to take framework-specified > minimal allocatable resources into account. We could make it a bit clearer: > This patch modifies the allocator to take the framework-specified minimum > allocatable resources into account. These act as an override of the existing > `min_allocatable_resources` flag. What more needs to be said? src/master/allocator/mesos/hierarchical.cpp Lines 1952-1968 (original), 1978-1987 (patched) <https://reviews.apache.org/r/69821/#comment298189> This makes it look like we should consolidate into the `isFiltered` function? src/master/allocator/mesos/hierarchical.cpp Line 2069 (original), 2082 (patched) <https://reviews.apache.org/r/69821/#comment298188> Now that this is a continue instead of a break, it seems they can be consolidated into 1 below? Also, can you post the benchmark numbers before and after this patch to make sure we're not accidentally regressing pretty badly on performance? src/master/allocator/mesos/hierarchical.cpp Lines 2400-2409 (patched) <https://reviews.apache.org/r/69821/#comment298186> Hm.. what's this? Are you suggesting that the allocatable check should be moved within isFiltered? (seems like a good idea to me) src/tests/hierarchical_allocator_tests.cpp Lines 2287-2289 (patched) <https://reviews.apache.org/r/69821/#comment298187> Can you pull the tests into a separate review to simplify reviewing? src/tests/hierarchical_allocator_tests.cpp Lines 2289 (patched) <https://reviews.apache.org/r/69821/#comment298190> FrameworkMinAllocatable src/tests/hierarchical_allocator_tests.cpp Lines 2336 (patched) <https://reviews.apache.org/r/69821/#comment298191> FrameworkEmptyMinAllocatable src/tests/hierarchical_allocator_tests.cpp Lines 2378 (patched) <https://reviews.apache.org/r/69821/#comment298192> Hm.. this doesn't quite look right to me, it seems if a framework is setting the `min_allocatable_resources` field, it should override, but that's not happening here? - Benjamin Mahler On Jan. 29, 2019, 4:20 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69821/ > ----------------------------------------------------------- > > (Updated Jan. 29, 2019, 4:20 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 options, it now inspects a dynamically > calculated set of minimal allocatable resources. This set is populated > with any minimal allocatable resources specified in the allocator's > options and any framework's minimal allocatable resources. For a > resource to be allocatable it needs to contain any of these minimal > requirements. > > If a framework does not specify minimal allocatable resource > requirements, its minimal requirements are set to the globally > configured option. > > We also adjust the allocator to take a framework's minimal resource > requirements into account when checking whether a resource can be > allocated to a particular framework. The check is performed at the same > time we check whether a framework filtered a particular resource. This > avoids offering resources to frameworks which the framework would never > have considered allocatable itself (e.g., given the global option of the > allocator). > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > ca1638390d89e2a81efd9d6d4a28b863c79723c4 > src/master/allocator/mesos/hierarchical.cpp > f1f3894058a8e3f008013cb269744bd36c0e31b3 > src/tests/hierarchical_allocator_tests.cpp > cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee > > > Diff: https://reviews.apache.org/r/69821/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
