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 Fri, 08 Jan 2016 10:36:55 GMT

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


Here's my first full pass over this patch. Couldn't find anything major except the PUT vs.
POST question.
I'll admit I was surprised to see in your testing section that `role1` showed up in `/roles`
even though it has a default weight. Can you explain why that happens and if it was intended
behavior?


include/mesos/authorizer/authorizer.hpp (lines 192 - 197)
<https://reviews.apache.org/r/41681/#comment174090>

    Delete everything in this comment after the first sentence "Used to verify... specific
roles." We recently removed some redundancy in these comments, and I think you're still following
the old pattern.



include/mesos/authorizer/authorizer.hpp (lines 199 - 201)
<https://reviews.apache.org/r/41681/#comment174091>

    "@param request `ACL::UpdateWeights` packing all the parameters needed to verify if the
given principal is allowed to update the weight of the specified roles.



include/mesos/authorizer/authorizer.hpp (line 204)
<https://reviews.apache.org/r/41681/#comment174092>

    s/however//



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

    s/roles.../roles to update./



src/CMakeLists.txt (lines 173 - 178)
<https://reviews.apache.org/r/41681/#comment174093>

    'weights' goes after 'validation' alphabetically



src/Makefile.am (lines 580 - 583)
<https://reviews.apache.org/r/41681/#comment174094>

    'weights' after 'validation'



src/master/http.cpp (lines 1103 - 1104)
<https://reviews.apache.org/r/41681/#comment174095>

    POST or PUT?
    "... and updates the weight for the specified roles."



src/master/http.cpp (lines 1115 - 1117)
<https://reviews.apache.org/r/41681/#comment174096>

    "Like /quota, we should also add query logic for /weights to keep consistent. Then /roles
no longer needs to show weight information."
    
    Create a separate follow-up JIRA for this. We don't need to get to it for MVP, but we
can revisit it later.



src/master/master.hpp (lines 1037 - 1039)
<https://reviews.apache.org/r/41681/#comment174097>

    Just wondering if we really want the authorize() interface to take multiple roles, or
just a single role at a time.



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

    Please add a space before parentheses like "value (1.0)"



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

    s/remove/remove it/



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

    s/is/are/



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

    s/Ignore/Ignoring/



src/master/master.cpp (lines 1471 - 1475)
<https://reviews.apache.org/r/41681/#comment174104>

    Do you still want to update the allocator (push_back) with 1.0 weights? Are those even
possible if we're recovering all this from the registry? Maybe we should just CHECK.



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

    Is `weights` loaded with the contents of `--weights` first?
    If there's a role in `--weights` that was since erased from the registry (weight=1.0),
wouldn't `weights` still have the cmd-line value after fresh recovery?



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

    s/boostrap cluster/bootstrapping the cluster/



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

    This last line is false, since the agents of the cluster know nothing about the weights.
A newly elected master will recover the weights from the registry.



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

    Why did we choose PUT instead of POST. Everything else uses POST.



src/master/weights_handler.cpp (lines 60 - 61)
<https://reviews.apache.org/r/41681/#comment174112>

    You can squeeze these two onto the same line.



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

    We don't use the 'Str' abbreviation in Mesos. How about just `weights`?



src/master/weights_handler.cpp (lines 81 - 83)
<https://reviews.apache.org/r/41681/#comment174115>

    Wow, you should have `using google::protobuf::RepeatedPtrField;` up top so you can shorten
this.



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

    Why the `_`? Why not just `weightInfo`?



src/master/weights_handler.cpp (lines 97 - 100)
<https://reviews.apache.org/r/41681/#comment174117>

    Why the ostringstream here?



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

    Can you also show the weight as well, so they know what weight value is invalid?



src/master/weights_handler.cpp (lines 111 - 112)
<https://reviews.apache.org/r/41681/#comment174120>

    Please put these two on the same line. In fact, it looks like you're wrapping at 52chars
for some reason. Wrap at 80, please.



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

    Can you s/Option<string>::none()/None()/?



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

    // Update the registry and acknowledge the request.



src/master/weights_handler.cpp (lines 150 - 151)
<https://reviews.apache.org/r/41681/#comment174128>

    How about an `else` instead of a `continue`?



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

    "to update weights for roles..."


- Adam B


On Jan. 5, 2016, 6:15 p.m., Yongqiao Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41681/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2016, 6:15 p.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 f61613948b7b5c5c2118f1782a0c5f8ff7e8e057 
>   include/mesos/authorizer/authorizer.proto 7b729e19484d92be48bbde4dff2c833a4109936e

>   src/CMakeLists.txt 8169fe4f1a0eda5c2e2b30e22a7346478420a9a0 
>   src/Makefile.am e08e86724abaa3023f0483aa222354c95d4d3817 
>   src/authorizer/local/authorizer.hpp 1563c11709c2612350354690b50ceb33d2720f98 
>   src/authorizer/local/authorizer.cpp 1d135fb6906c7050a788cbac9ca2d8164ff064ef 
>   src/master/http.cpp deb0c8f9852dc0eec1e8c0ff35c684f982e8b110 
>   src/master/master.hpp 1cc5531de70bdc0bdce9835c4930dc774406fac9 
>   src/master/master.cpp 40ce3e17fca88da689128bcf5d35fdddc396c011 
>   src/master/registry.proto 9958f9c2bdb785390fca2f292b65d5a9310434d5 
>   src/master/weights_handler.cpp PRE-CREATION 
>   src/tests/mesos.hpp a4811b0d8dee33ff2ca4968f532ce64b7ea95249 
>   src/tests/mesos.cpp f4b0f82449c4b6a2b4b7b7f14518714485d5a13a 
> 
> Diff: https://reviews.apache.org/r/41681/diff/
> 
> 
> Testing
> -------
> 
> Make & Make check successfully!
> 
> $ (./mesos-master.sh --ip=127.0.0.1 --work_dir=/Users/yqwyq/tmp/mesos-master  >>
/tmp/mesos-master.log 2>&1 &)
> $ curl -d weights="[{\"weight\":1.0,\"role\":\"role1\"},{\"weight\":8.0,\"role\":\"role2\"}]"
-X PUT http://localhost: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
>         }, 
>         {
>             "frameworks": [ ], 
>             "name": "role2", 
>             "resources": {
>                 "cpus": 0, 
>                 "disk": 0, 
>                 "mem": 0
>             }, 
>             "weight": 8
>         }
>     ]
> }
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>


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