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


I'm not sure exactly why you needed to move RoleInfo out of allocator.proto. 
The RoleInfo we use for the allocator module API doesn't need to be (and 
perhaps shouldn't be) the same class that we use to display role information 
like weights in the HTTP endpoint, or even the same class that we persist in 
the registry. There may be some future role metadata we want to set in the HTTP 
endpoint that doesn't need to be passed on to the allocator (e.g. 
Role.description), and maybe even something we set that doesn't need to be 
persisted. I don't actually think we need 3 or 4 separate RoleInfo-like 
protobufs, but I want us to think about how each of these APIs could grow apart 
in their notion of "important role metadata". Is there a real need to move the 
RoleInfo protobuf?


include/mesos/master/allocator.proto (line 19)
<https://reviews.apache.org/r/40431/#comment169315>

    Shouldn't this file have `java_package` and `java_outer_classname` just 
like the other protos?
    Looks like isolator.proto and oversubscription.proto are missing it too. 
Would you mind creating a separate patch to fix that?



include/mesos/role/role.proto (line 21)
<https://reviews.apache.org/r/40431/#comment169316>

    Why change the package? Couldn't this still be in `mesos.master`? Then you 
wouldn't have to change all the other files.


- 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/40431/
> -----------------------------------------------------------
> 
> (Updated Dec. 7, 2015, 9:20 p.m.)
> 
> 
> Review request for mesos, Adam B, Guangya Liu, Qian Zhang, and Jian Qiu.
> 
> 
> Bugs: MESOS-3944
>     https://issues.apache.org/jira/browse/MESOS-3944
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   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 c0d77c745eb5b12dd6d9d7afaba7e820f8d848ef 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/40431/diff/
> 
> 
> Testing
> -------
> 
> 1. Make Check successfully;
> 
> 2. $ curl http://9.110.48.168:5050/roles
> {"roles":[{"frameworks":[],"name":"*","resources":{"cpus":0,"disk":0,"mem":0},"weight":1.0}]}
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Reply via email to