mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 47399: New update_quotas ACL for both set and remove cases.
Date Tue, 17 May 2016 13:26:09 GMT

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



I've committed MESOS-5336, I believe you have to rebase this one.


include/mesos/authorizer/acls.proto (line 124)
<https://reviews.apache.org/r/47399/#comment197838>

    You don't have to put my name here : ). Feel free to put yours, if you like; it is your
patch. Here and everywhere.



include/mesos/authorizer/acls.proto (lines 165 - 182)
<https://reviews.apache.org/r/47399/#comment197839>

    Though it does not influence the thrust, it would be great if we maintain the same ordering
everywhere. I'd vote for keeping them after dynamic volumes, but feel free to put them at
the end as well. I care more about consistency.



src/authorizer/local/authorizer.cpp (lines 69 - 70)
<https://reviews.apache.org/r/47399/#comment197844>

    No period at the end of log messages.
    
    I see this is in sync with other log messages in this file, but we tend to put the space
in the beginning of the next line, so it's easier to spot when they are missing. You don't
need to change it here, so we stay locally consistent.



src/authorizer/local/authorizer.cpp (lines 195 - 198)
<https://reviews.apache.org/r/47399/#comment197994>

    How about
    ```
            // Deprecation case: If `update_quotas` is empty but
            // `set_quotas` or `remove_quotas` are not, "skip"
            // authorization here under assumption that the caller,
            // i.e., `QuotaHandler` checks `set_quotas`/`remove_quotas`.
    ```



src/authorizer/local/authorizer.cpp (line 462)
<https://reviews.apache.org/r/47399/#comment197993>

    I like the idea of validating ACLs on costruction. I'm thinking what else should we validate
here.



src/authorizer/local/authorizer.cpp (lines 468 - 469)
<https://reviews.apache.org/r/47399/#comment197992>

    Blank line.



src/master/quota_handler.cpp (line 557)
<https://reviews.apache.org/r/47399/#comment197995>

    No period, please. Also, wrap `role` in single quotes and sqush onto the previous line.



src/master/quota_handler.cpp (lines 566 - 567)
<https://reviews.apache.org/r/47399/#comment197997>

    Blank line, please.


- Alexander Rukletsov


On May 15, 2016, 4:30 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47399/
> -----------------------------------------------------------
> 
> (Updated May 15, 2016, 4:30 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-5155
>     https://issues.apache.org/jira/browse/MESOS-5155
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> New update_quotas ACL for both set and remove cases.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authorizer/acls.proto 9adae8c2a2e1b2ee4b9068ea50fcebb0544f2e5d 
>   include/mesos/authorizer/authorizer.proto 32492a59ad95df3bb673ec42321518f86c11af59

>   src/authorizer/local/authorizer.hpp d15d3a6c9f3d7c432e593cfe78cc48d672848d02 
>   src/authorizer/local/authorizer.cpp e95435327bb3b6f447e814b8657bce8084535346 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/tests/authorization_tests.cpp f50ac69c32d0551a63391d97f31559ff4f414011 
>   src/tests/master_quota_tests.cpp 9ce7e2e481706c74f080d54b6f58d57874a27e64 
> 
> Diff: https://reviews.apache.org/r/47399/diff/
> 
> 
> Testing
> -------
> 
> Unit tests.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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