You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Yong Qiao Wang <yq...@cn.ibm.com> on 2015/12/07 11:12:06 UTC
Re: Review Request 40469: Update Allocator interface to support
dynamic roles
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40469/
-----------------------------------------------------------
(Updated Dec. 7, 2015, 10:12 a.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 (updated)
-----
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
Re: Review Request 40469: Update Allocator interface to support
dynamic roles
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40469/#review109297
-----------------------------------------------------------
Patch looks great!
Reviews applied: [40431, 40469]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 8, 2015, 5:20 a.m., Yong Qiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40469/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 5:20 a.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
>
>
Re: Review Request 40469: Update Allocator interface to support
dynamic roles
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
> On Dec. 10, 2015, 10:46 a.m., Adam B wrote:
> > 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.
Refer to some other Epic(such as quota), they always split the changes into some smaller bits, so I also follow up them and want to implement these functions in another sepratated JIRA MESOS-3943, I think it is easier to reivew. I will post another patch for the implementation, is it OK?
For backwards-compatibility, I have considered this issue before, but I think Mesos does not reach to 1.0 so interfaces changes is resonable, and also refer to the quota implementation, the functions setQuota()/removeQuota() also be defined to pure virtual functions. I think we should keep consistence.
> On Dec. 10, 2015, 10:46 a.m., Adam B wrote:
> > include/mesos/master/allocator.hpp, line 409
> > <https://reviews.apache.org/r/40469/diff/4/?file=1155784#file1155784line409>
> >
> > 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'
For Dynamic Roles, I think it is make sence to provide a way to let cluster operator to remove a role due to the corresponding way is provided to add a role by /roles endpoint. But for Implicit Roles, this is non-necessary, I will update this patch to remove this function.
> On Dec. 10, 2015, 10:46 a.m., Adam B wrote:
> > include/mesos/master/allocator.hpp, lines 412-418
> > <https://reviews.apache.org/r/40469/diff/4/?file=1155784#file1155784line412>
> >
> > 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.
I suggest to make this funciton as a pure virtual function and let some other allocator to implement it, my reasons as below:
1. Mesos does not reach to 1.0, so the interface changes are resonable.
2. Like quota configuration(those functions are also be defined as pure virtual funcs in allocator.hpp), Weight dynamic update also is an important functions, I think it should be required for any mesos cluster.
Of couse, It is great to define this function with a default noop implementation for backwards-compatibility, I just confused for the consistence like quota done(setQuota()/removeQuota()). OK, let me know your further comment.
> On Dec. 10, 2015, 10:46 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1057-1058
> > <https://reviews.apache.org/r/40469/diff/4/?file=1155787#file1155787line1057>
> >
> > 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.
I plan to implement these functions in JIRA MESOS-3943, If you think we should merge them together, I am happy to do that.
- Yongqiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40469/#review109706
-----------------------------------------------------------
On Dec. 8, 2015, 5:20 a.m., Yongqiao Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40469/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 5:20 a.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,
>
> Yongqiao Wang
>
>
Re: Review Request 40469: Update Allocator interface to support
dynamic roles
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 40469: Update Allocator interface to support
dynamic weight
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40469/
-----------------------------------------------------------
(Updated Dec. 11, 2015, 8:59 a.m.)
Review request for mesos, Adam B, Guangya Liu, Qian Zhang, and Jian Qiu.
Summary (updated)
-----------------
Update Allocator interface to support dynamic weight
Bugs: MESOS-3956
https://issues.apache.org/jira/browse/MESOS-3956
Repository: mesos
Description (updated)
-------
Currently, RoleInfo protobuf never be used for serialization, so I think we can remove it from allocator.proto, and define a struct to communicate between the allocator and master. But for role information display, then current serialization way(call modle(role*) in http.cpp) is not better, and we should define another RoleInfo protobuf for serialization. Refer to other components(such as quota), I propose to define role protobuf in a separated package rather than define it in mesos.proto.
Diffs (updated)
-----
include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0
include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c
include/mesos/role/role.hpp PRE-CREATION
include/mesos/role/role.proto PRE-CREATION
src/CMakeLists.txt 0d46043dd06007f2cba56626c82e8edd251cb8fb
src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e
src/master/allocator/mesos/allocator.hpp 97ee80726ad155917811265a983258b0165d3451
src/master/allocator/mesos/hierarchical.hpp 99c742906874c30c39c159e58a65277ade3c07fd
src/master/allocator/mesos/hierarchical.cpp 5da825a1d578a9ee40b4985378fddb3c5fb3b416
src/master/allocator/sorter/drf/sorter.hpp 050896e8b12cd4097ccd137d5284d6b39b0f06ab
src/master/allocator/sorter/drf/sorter.cpp 3a442f121f3a1505513877a5c78458a4b8d0a824
src/master/allocator/sorter/sorter.hpp 7be6b44a762fd62c2cd7f28b4dc4865a4587ed26
src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8
src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4
src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42
src/tests/hierarchical_allocator_tests.cpp e239b4746494fcc2b362a83afb634a2ce5e25f9b
Diff: https://reviews.apache.org/r/40469/diff/
Testing (updated)
-------
Make successfully.
Make check successfully.
Run test framework successfully
TODO: will add a test for role update.
Thanks,
Yongqiao Wang
Re: Review Request 40469: Update Allocator interface to support
dynamic roles
Posted by Yong Qiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40469/
-----------------------------------------------------------
(Updated Dec. 8, 2015, 5:20 a.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 (updated)
-----
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
Re: Review Request 40469: Update Allocator interface to support
dynamic roles
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40469/#review109238
-----------------------------------------------------------
Bad patch!
Reviews applied: [40431]
Failed command: ./support/apply-review.sh -n -r 40431
Error:
2015-12-08 01:04:32 URL:https://reviews.apache.org/r/40431/diff/raw/ [12531/12531] -> "40431.patch" [1]
Successfully applied: Move RoleInfo message out of allocator.proto
Currently role protobuf is defined in allocator.proto due to only the traditional DRF allocator uses roles as it’s first level of hierarchy, I think we should move it out and define it in a separated file as quota had in dynamic roles project, because role protobuf will also be used by master to persist.
Review: https://reviews.apache.org/r/40431
include/mesos/role/role.hpp:1: A license header should appear on the file's first line starting with '// Licensed'.: /**
Total errors found: 1
Checking 9 files
Failed to commit patch
- Mesos ReviewBot
On Dec. 7, 2015, 10:12 a.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, 10:12 a.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
>
>