----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47274/#review133388 -----------------------------------------------------------
Okay, almost there. We should be good after one more pass. src/master/master.hpp (line 1022) <https://reviews.apache.org/r/47274/#comment197763> Do you still need `request`? src/master/quota_handler.cpp (line 421) <https://reviews.apache.org/r/47274/#comment197755> Since we are adding support for authorizing GET now, we need to adjust wording in `QUOTA_HELP()` in "master/http.cpp" (and run "./support/generate-endpoint-help.py" to update the docs). It would be great to say that if a user GETs /quota, the results can be silently filtered. src/master/quota_handler.cpp (lines 427 - 428) <https://reviews.apache.org/r/47274/#comment197758> Mind writing a concise comment on why you need this copy? src/master/quota_handler.cpp (line 430) <https://reviews.apache.org/r/47274/#comment197756> Feel free to kill this blank line. src/master/quota_handler.cpp (line 434) <https://reviews.apache.org/r/47274/#comment197792> Could you please restore the TODO from the previous version of the patch? Also your comment // Create a list of authorization actions for each role we may return. // TODO(alexr): Batch these actions once we have BatchRequest in authorizer. src/master/quota_handler.cpp (line 435) <https://reviews.apache.org/r/47274/#comment197760> How about s/getQuotaByRoleAuthorized/authorizedRoles? src/master/quota_handler.cpp (line 443) <https://reviews.apache.org/r/47274/#comment197762> Any reason you don't say `const list<bool>&`? Also maybe s/authorized/authorizedRolesCollected ? src/master/quota_handler.cpp (line 452) <https://reviews.apache.org/r/47274/#comment197772> s/authorized/authorizedRoles ? src/master/quota_handler.cpp (line 461) <https://reviews.apache.org/r/47274/#comment197767> Feel free to use `auto` here. Also, `quotaInfoIt` can be a better name. src/master/quota_handler.cpp (line 462) <https://reviews.apache.org/r/47274/#comment197778> If you rename `authorized` to `authorizedRoles`, you can then rename `result` to `authorized`. I'd say `if (authorized)` is more descriptive. src/master/quota_handler.cpp (lines 467 - 468) <https://reviews.apache.org/r/47274/#comment197781> Blank line, please. src/master/quota_handler.cpp (line 495) <https://reviews.apache.org/r/47274/#comment197780> We separate implementations with two blank lines. Thanks! src/master/quota_handler.cpp (line 504) <https://reviews.apache.org/r/47274/#comment197783> Now in retrospect it looks like it was a mediocre idea to use `LOG(INFO)` in this case. We may have to revisit our policy around log levels one day. src/master/quota_handler.cpp (lines 516 - 517) <https://reviews.apache.org/r/47274/#comment197782> Let's add a blank line for consistency with other `authorize*()` functions in this file. src/master/quota_handler.cpp (lines 518 - 520) <https://reviews.apache.org/r/47274/#comment197793> Two blanks here, please! - Alexander Rukletsov On May 13, 2016, 6:48 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47274/ > ----------------------------------------------------------- > > (Updated May 13, 2016, 6:48 p.m.) > > > Review request for mesos, Adam B and Alexander Rukletsov. > > > Bugs: MESOS-5336 > https://issues.apache.org/jira/browse/MESOS-5336 > > > Repository: mesos > > > Description > ------- > > Authorize what quota can be seen by GET_QUOTA_BY_ROLE. > > > Diffs > ----- > > include/mesos/authorizer/acls.proto > 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d > include/mesos/authorizer/authorizer.proto > 32492a59ad95df3bb673ec42321518f86c11af59 > src/authorizer/local/authorizer.cpp > e95435327bb3b6f447e814b8657bce8084535346 > src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 > src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 > src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c > src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 > > Diff: https://reviews.apache.org/r/47274/diff/ > > > Testing > ------- > > Unit test. > > > Thanks, > > Zhitao Li > >
