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