mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 55870: Update the allocator to handle frameworks with multiple roles.
Date Tue, 31 Jan 2017 06:24:25 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message