----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55870/#review163516 -----------------------------------------------------------
include/mesos/allocator/allocator.hpp (lines 89 - 92) <https://reviews.apache.org/r/55870/#comment234977> What is the reason for changing this interface? We should think hard before making it too specific to requirements from single features such as `MULTI_ROLE`. Propagating internal assumptions such as "only allocations towards the same role in one `Resources`" across module boundaries really limits room for future developments. In this case both our allocator and the master rely on above assumption; it should be possible for them to adjust passed resources on interfaces, e.g., by creating datastructure like you proposed here internally themself (they could use the same helper function). That way we could avoid tech debt spilling into interfaces. include/mesos/allocator/allocator.hpp (line 264) <https://reviews.apache.org/r/55870/#comment234979> Do we need this detail of our allocator implementation in the generic interface? include/mesos/allocator/allocator.hpp (lines 333 - 335) <https://reviews.apache.org/r/55870/#comment234980> Internal detail? src/master/allocator/mesos/hierarchical.cpp (lines 685 - 690) <https://reviews.apache.org/r/55870/#comment234978> Is this detailed knowledge of `Resource` internals necessary here? Below we use the same adjusted `operation` regardless of its type. src/master/allocator/mesos/hierarchical.cpp (lines 703 - 704) <https://reviews.apache.org/r/55870/#comment234976> Lets try to put this code into the caller (e.g., `Master::_accept`). It seems that would introduce less current and future breakage in custom allocator modules. - Benjamin Bannier On Jan. 25, 2017, 10:55 p.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55870/ > ----------------------------------------------------------- > > (Updated Jan. 25, 2017, 10:55 p.m.) > > > Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael > Park. > > > Bugs: MESOS-6635 > https://issues.apache.org/jira/browse/MESOS-6635 > > > Repository: mesos > > > Description > ------- > > The allocator now sets `Resource.AllocationInfo` when performing > an allocation, and the interface is updated to expose allocations > for each role within a framework. > > The allocator also assumes that `Resource.AllocationInfo` is set > for inbound resources that are allocated (e.g. when adding an agent, > when adding a framework, when recovering resources). Note however, > that the necessary changes to the master and agent to enforce this > will be done via separate patches. > > > Diffs > ----- > > include/mesos/allocator/allocator.hpp > 558594983beb6ff49c1fddf875ba29c1983b1288 > src/master/allocator/mesos/allocator.hpp > 8e0f37a5eedd4d71d765991c0039b49c96f0ca53 > src/master/allocator/mesos/hierarchical.hpp > 9b66c23f26b37c02ed850c06c4518ea99078b02d > src/master/allocator/mesos/hierarchical.cpp > c2211be7458755aeb91ef078e4bfe92ac474044a > src/master/master.hpp 8e8a9037af11cf95961b6498540a0fd486ed091b > src/master/master.cpp 73159328ce3fd838e02eba0e6a30cf69efc319ba > src/tests/allocator.hpp 1f9261d1e7afa027bebc07915ba3e871a58e6558 > > Diff: https://reviews.apache.org/r/55870/diff/ > > > Testing > ------- > > Adjustments to existing tests are split out into a subsequent patch. > > New tests for frameworks having multiple roles will be added as a follow up. > > > Thanks, > > Benjamin Mahler > >
