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


Getting close. Just get rid of the special 1.0 behavior and I'll give it a 
final review.


src/master/http.cpp (line 1297)
<https://reviews.apache.org/r/41681/#comment176365>

    s/update/updates the/



src/master/master.hpp (lines 1556 - 1561)
<https://reviews.apache.org/r/41681/#comment176366>

    We got rid of this excecption in the allocator. Shouldn't we store 
explicitly set 1.0s in the registry and show them in /roles too?



src/master/weights_handler.cpp (lines 143 - 145)
<https://reviews.apache.org/r/41681/#comment176367>

    Remove, unnecessarily complex.



include/mesos/authorizer/authorizer.hpp (lines 211 - 212)
<https://reviews.apache.org/r/41681/#comment176373>

    s/can update/is allowed to update/
    s/the specified roles/every specified role/



include/mesos/authorizer/authorizer.proto (line 130)
<https://reviews.apache.org/r/41681/#comment176372>

    `UpdateRoleWeights`? In case we end up allowing weights on frameworks too. 
Else we could use `optional Entity roles` and later add something like 
`optional Entity frameworkId` and possibly an ObjectType enum.



src/master/http.cpp (line 1294)
<https://reviews.apache.org/r/41681/#comment176377>

    s/weight/weights/



src/master/http.cpp (line 1297)
<https://reviews.apache.org/r/41681/#comment176381>

    s/update weight/updates weights/



src/master/master.hpp (line 1577)
<https://reviews.apache.org/r/41681/#comment176384>

    Can it ever return anything but `true`? Are there any error conditions at 
all?



src/master/master.cpp (lines 1556 - 1559)
<https://reviews.apache.org/r/41681/#comment176388>

    Remove. Unnecessary complexity. (and then we won't need the 
`utils::copy(weights)` above)



src/master/master.cpp (line 1562)
<https://reviews.apache.org/r/41681/#comment176389>

    s/if(/if (/



src/master/master.cpp (line 1563)
<https://reviews.apache.org/r/41681/#comment176391>

    s/log //



src/master/master.cpp (line 1565)
<https://reviews.apache.org/r/41681/#comment176390>

    s/updateWeightInfos/weightInfos/?



src/master/master.cpp (line 1566)
<https://reviews.apache.org/r/41681/#comment176392>

    foreachpair? Then you don't have to get `weights[role]`



src/master/master.cpp (line 1572)
<https://reviews.apache.org/r/41681/#comment176393>

    Should this also `allocator->updateWeights(weightInfos);`?



src/master/registry.proto (line 81)
<https://reviews.apache.org/r/41681/#comment176394>

    s/non-default//



src/master/weights_handler.cpp (line 93)
<https://reviews.apache.org/r/41681/#comment176396>

    s/can not/cannot/



src/master/weights_handler.cpp (lines 113 - 115)
<https://reviews.apache.org/r/41681/#comment176395>

    Why are you reconstructing a roles `string` out of the weightInfos instead 
of just using a `vector`? Does `authorize()` really require a comma-delimited 
string?
    
    What I was proposing before was that you call `authorize(principal, role)` 
for each weightInfo.role, and fail if any of those fails.


- Adam B


On Jan. 18, 2016, 6:09 a.m., Yongqiao Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41681/
> -----------------------------------------------------------
> 
> (Updated Jan. 18, 2016, 6:09 a.m.)
> 
> 
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
> 
> 
> Bugs: MESOS-4214
>     https://issues.apache.org/jira/browse/MESOS-4214
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduce HTTP endpoint /weights for updating weight.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authorizer/authorizer.hpp 
> 5ee3c7afadd131802c93febbb6b4dbad069c2d81 
>   include/mesos/authorizer/authorizer.proto 
> 84727e66dd14be9a25705ab1141e92eee72fae50 
>   src/CMakeLists.txt 39a23df3227a4f524ea0d408dc894fa5bbab7d10 
>   src/Makefile.am d23e35001078a86775bd9b76baa207ecb9dab7e1 
>   src/authorizer/local/authorizer.hpp 
> c7321c276d566eca6a91f45c546468bea1b0da15 
>   src/authorizer/local/authorizer.cpp 
> c1db9c2131ea8fbf835278203a240f108c6372c5 
>   src/master/http.cpp 34a70ee50553492fc8c3947497ab5922f4379d72 
>   src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 
>   src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
>   src/master/registry.proto 9958f9c2bdb785390fca2f292b65d5a9310434d5 
>   src/master/weights_handler.cpp PRE-CREATION 
>   src/tests/mesos.hpp 3d9ebc6c9dc3cd1be02dc3771fbd847386907fac 
>   src/tests/mesos.cpp 365ebe8335c37bfdb983a5424d4c995fa9b76a22 
> 
> Diff: https://reviews.apache.org/r/41681/diff/
> 
> 
> Testing
> -------
> 
> Make & Make check successfully!
> 
> $ (./mesos-master.sh --ip=127.0.0.1 --work_dir=/tmp/mesos-master  
> --weights="role1=4.2,role2=3.1" --authenticate_http 
> --credentials=/opt/credentials.json  >> /tmp/mesos-master.log 2>&1 &)
> $ curl http://localhost:5050/roles
> {
>     "roles": [
>         {
>             "frameworks": [ ], 
>             "name": "*", 
>             "resources": {
>                 "cpus": 0, 
>                 "disk": 0, 
>                 "mem": 0
>             }, 
>             "weight": 1
>         }, 
>         {
>             "frameworks": [ ], 
>             "name": "role1", 
>             "resources": {
>                 "cpus": 0, 
>                 "disk": 0, 
>                 "mem": 0
>             }, 
>             "weight": 4.2
>         }, 
>         {
>             "frameworks": [ ], 
>             "name": "role2", 
>             "resources": {
>                 "cpus": 0, 
>                 "disk": 0, 
>                 "mem": 0
>             }, 
>             "weight": 3.1
>         }
>     ]
> }
> 
> - Test update:
> $ curl --user framework1:secret_string1 --data 
> weights="[{\"weight\":1.8,\"role\":\"role1\"},{\"weight\":1.0,\"role\":\"role2\"},{\"weight\":3.4,\"role\":\"role3\"}]"
>  -X PUT http://127.0.0.1:5050/weights
> $ curl http://localhost:5050/roles
> {
>     "roles": [
>         {
>             "frameworks": [ ], 
>             "name": "*", 
>             "resources": {
>                 "cpus": 0, 
>                 "disk": 0, 
>                 "mem": 0
>             }, 
>             "weight": 1
>         }, 
>         {
>             "frameworks": [ ], 
>             "name": "role1", 
>             "resources": {
>                 "cpus": 0, 
>                 "disk": 0, 
>                 "mem": 0
>             }, 
>             "weight": 1.8
>         }, 
>         {
>             "frameworks": [ ], 
>             "name": "role3", 
>             "resources": {
>                 "cpus": 0, 
>                 "disk": 0, 
>                 "mem": 0
>             }, 
>             "weight": 3.4
>         }
>     ]
> }
> 
> - Test recovuery:
> $ ps -ef | grep mesos-master
> 501 56292     1   0  6:18PM ttys001    0:00.31 
> /Users/yqwyq/Desktop/mesos/build/src/.libs/mesos-master --ip=127.0.0.1 
> --work_dir=/tmp/mesos-master --weights=role1=4.2,role2=3.1 
> --authenticate_http --credentials=/opt/credentials.json
> $ kill -9 56292
> 
> $ (./mesos-master.sh --ip=127.0.0.1 --work_dir=/tmp/mesos-master  
> --weights="role1=4.2,role2=3.1,role6=9.0" --authenticate_http 
> --credentials=/opt/credentials.json  >> /tmp/mesos-master.log 2>&1 &)
> $ curl http://localhost:5050/roles
> {
>     "roles": [
>         {
>             "frameworks": [ ], 
>             "name": "*", 
>             "resources": {
>                 "cpus": 0, 
>                 "disk": 0, 
>                 "mem": 0
>             }, 
>             "weight": 1
>         }, 
>         {
>             "frameworks": [ ], 
>             "name": "role1", 
>             "resources": {
>                 "cpus": 0, 
>                 "disk": 0, 
>                 "mem": 0
>             }, 
>             "weight": 1.8
>         }, 
>         {
>             "frameworks": [ ], 
>             "name": "role3", 
>             "resources": {
>                 "cpus": 0, 
>                 "disk": 0, 
>                 "mem": 0
>             }, 
>             "weight": 3.4
>         }
>     ]
> }
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Reply via email to