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