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


Fix it, then Ship it!




Would love to commit this asap, but we *need* to get the registry tests in with 
these changes. I'd like to at least do a quick pass over the registry tests 
first so a) we know we're testing the critical changes, and b) we have 
confidence that the tests will land in the same release as the endpoint.


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

    Oops, looks like you copied my typo. Sorry.
    s/updated/update/



src/master/master.cpp (lines 1549 - 1552)
<https://reviews.apache.org/r/41681/#comment184482>

    How about:
    "The registry contains dynamic weights, so static weights should be 
ignored. We must neutralize (set to 1.0) any weights statically set in 
`--weights` that are not explicitly overridden by the registry."
    or
    "Before recovering weights from the registry, the allocator was already 
initialized with `--weights`, so here we need to reset (to 1.0) weights in the 
allocator that are not overridden by the registry."



src/master/master.hpp (lines 1040 - 1042)
<https://reviews.apache.org/r/41681/#comment184486>

    So if I try to update N roles in one request, but I am not authorized to 
update 1, then I would get back a simple boolean "Forbidden" response, rather 
than a pointer to which role was unauthorized? I guess the best thing the 
client/user can do then is just try to update each role individually until one 
is rejected.
    Is there some way we could give back a more helpful error message with the 
Forbidden response?



src/master/master.cpp (lines 1546 - 1547)
<https://reviews.apache.org/r/41681/#comment184498>

    This log message will be easier to read if you wrap the flags value in 
single-quotes, so it becomes:
    ```
    ... flag '" << flags.weights.get()
    << "', and ...
    ```



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

    Why do you need a utils::copy() if `registry_weights` is not const, and 
will not be used after this?



src/master/master.cpp (lines 1571 - 1572)
<https://reviews.apache.org/r/41681/#comment184502>

    Let's add a comment that "The allocator was already updated with the flag 
values on startup."



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

    How about renaming this to `validatedWeightInfos`? Both this and 
`weightInfos` are for updating, but only one of these is validated.



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

    Why not just s/updateWeightInfos/weightInfos/?


- Adam B


On March 7, 2016, 4:19 a.m., Yongqiao Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41681/
> -----------------------------------------------------------
> 
> (Updated March 7, 2016, 4:19 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 
> 84d2cb3fbff3fbc7c3854d6eec5a3a55ad5760f8 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/authorizer/local/authorizer.hpp 
> c7321c276d566eca6a91f45c546468bea1b0da15 
>   src/authorizer/local/authorizer.cpp 
> a1486bd042e1d59e5ac99c2619fb3228c37b9788 
>   src/master/http.cpp 8276baa538eb4d2aaf54cc1aa516bffaadacc4dd 
>   src/master/master.hpp ea26670e6c6c67314406fded510e8fdd46053dc8 
>   src/master/master.cpp 57ff4a39039f573b8586bc03f873f97826b97f6f 
>   src/master/registry.proto 9958f9c2bdb785390fca2f292b65d5a9310434d5 
>   src/master/weights_handler.cpp PRE-CREATION 
>   src/tests/mesos.hpp d36840f6e23e5823332de53061bf6852330bdf0b 
>   src/tests/mesos.cpp e0f641c6828833de13a0a233e39ff6dc3f343d5c 
> 
> 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 | python -mjson.tool
> {
>     "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 
> "[{\"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 | python -mjson.tool
> {
>     "roles": [
>         {
>             "frameworks": [],
>             "name": "*",
>             "resources": {
>                 "cpus": 0,
>                 "disk": 0,
>                 "mem": 0
>             },
>             "weight": 1.0
>         },
>         {
>             "frameworks": [],
>             "name": "role1",
>             "resources": {
>                 "cpus": 0,
>                 "disk": 0,
>                 "mem": 0
>             },
>             "weight": 1.8
>         },
>         {
>             "frameworks": [],
>             "name": "role2",
>             "resources": {
>                 "cpus": 0,
>                 "disk": 0,
>                 "mem": 0
>             },
>             "weight": 1.0
>         },
>         {
>             "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 | python -mjson.tool
> {
>     "roles": [
>         {
>             "frameworks": [],
>             "name": "*",
>             "resources": {
>                 "cpus": 0,
>                 "disk": 0,
>                 "mem": 0
>             },
>             "weight": 1.0
>         },
>         {
>             "frameworks": [],
>             "name": "role1",
>             "resources": {
>                 "cpus": 0,
>                 "disk": 0,
>                 "mem": 0
>             },
>             "weight": 1.8
>         },
>         {
>             "frameworks": [],
>             "name": "role2",
>             "resources": {
>                 "cpus": 0,
>                 "disk": 0,
>                 "mem": 0
>             },
>             "weight": 1.0
>         },
>         {
>             "frameworks": [],
>             "name": "role3",
>             "resources": {
>                 "cpus": 0,
>                 "disk": 0,
>                 "mem": 0
>             },
>             "weight": 3.4
>         }
>     ]
> }
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Reply via email to