mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 40469: Update Allocator interface to support dynamic roles
Date Thu, 10 Dec 2015 10:46:44 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40469/#review109706
-----------------------------------------------------------


This looks like just the interface change; where's the (default/reference) implementation?
Justify/delete the removeRole call.
Consider (the lack of) backwards-compatibility for your allocator module API change.


include/mesos/master/allocator.hpp (line 409)
<https://reviews.apache.org/r/40469/#comment169320>

    When is removeRole necessary? There aren't likely to be so many roles that we need to
worry about saving memory by clearing out inactive roles.
    If there are no frameworks registered with this role, it is inactive and won't affect
allocator decisions.
    If an admin wants to unspecify a weight for this user, they could just set it back to
the default '1.0'



include/mesos/master/allocator.hpp (lines 412 - 418)
<https://reviews.apache.org/r/40469/#comment169356>

    This is a breaking API change for the allocator, and any implementers of allocator modules
will not only have to recompile their modules against the newer version, but will also have
to implement this new function.
    
    I wonder if it would be better to not make this a pure virtual function and instead have
a default noop implementation, so module authors can recompile without error, and add dynamic
role support to their own allocator at their leisure.
    
    Either way, we'll need to make an announcement to the dev@ list and put a note in the
upgrades doc.



src/master/allocator/mesos/hierarchical.cpp (lines 1057 - 1058)
<https://reviews.apache.org/r/40469/#comment169358>

    Seems like there's a lot more missing here. Which subsequent review actually implements
the new functions? Perhaps that patch should be merged into this one so we have a functional
patch to commit at once.


- Adam B


On Dec. 7, 2015, 9:20 p.m., Yong Qiao Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40469/
> -----------------------------------------------------------
> 
> (Updated Dec. 7, 2015, 9:20 p.m.)
> 
> 
> Review request for mesos, Adam B, Guangya Liu, Qian Zhang, and Jian Qiu.
> 
> 
> Bugs: MESOS-3956
>     https://issues.apache.org/jira/browse/MESOS-3956
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Update Allocator interface to support dynamic roles
> 
> 
> Diffs
> -----
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd

>   src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416

>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
> 
> Diff: https://reviews.apache.org/r/40469/diff/
> 
> 
> Testing
> -------
> 
> Make check successfully.
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>


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