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

Reply via email to