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 Tue, 08 Mar 2016 08:07:28 GMT

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


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