> On Jan. 30, 2017, 8:12 a.m., Benjamin Bannier wrote:
> > include/mesos/allocator/allocator.hpp, lines 89-92
> > <https://reviews.apache.org/r/55870/diff/1/?file=1615402#file1615402line89>
> >
> > 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.
I can sympathize with this position, although it doesn't seem terrible to me
that
the notion of multi-role which is being introduced to Mesos (rather than a
specific allocator)
changes the allocator API.
But it doesn't seem obvious to me why it should be `hashmap<string,
hashmap<SlaveID, Resources>>`,
as opposed to `hashmap<SlaveID, hashmap<string, Resources>>`. I tried to look
through the code
briefly to see if it happens to make the code a lot simpler or something, but I
don't really see one..
It seems to me like the pattern I see in a few places could be used here
without changing the API also.
```cpp
const hashmap<SlaveID, Resources>& resources;
foreachpair (const SlaveID& slaveId, const Resources& resources) {
foreachpair (const string& role,
const Resources& resources,
resources.allocation()) {
// ...
}
}
```
I'm more-so trying to understand what the reasoning is here.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55870/#review163516
-----------------------------------------------------------
On Jan. 25, 2017, 1: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, 1: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
>
>
