mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 55870: Update the allocator to handle frameworks with multiple roles.
Date Mon, 30 Jan 2017 16:12:21 GMT

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


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