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 41681: Introduce HTTP endpoint /weights for updating weight.
Date Wed, 20 Jan 2016 09:51:23 GMT

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


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