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 47274: Authorize what quota can be seen by GET_QUOTA_BY_ROLE.
Date Fri, 13 May 2016 14:17:41 GMT

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



Looks good. Could you please make this patch self-contained, i.e., remove dependency on /r/47222?

Did you manually test that authorization works? I mean setup ACLs, start a master, force-set
some quotas and check that they are being properly filtered? Could you please do it and describe
in the "testing done" section?


include/mesos/authorizer/acls.proto (line 206)
<https://reviews.apache.org/r/47274/#comment197375>

    Let's move this line up, before `SetQuota`



include/mesos/authorizer/authorizer.proto (line 62)
<https://reviews.apache.org/r/47274/#comment197376>

    Ditto.



src/authorizer/local/authorizer.cpp (lines 219 - 229)
<https://reviews.apache.org/r/47274/#comment197377>

    Ditto (move up before set quota).



src/master/master.hpp (line 1013)
<https://reviews.apache.org/r/47274/#comment197380>

    This can be simply named `authorizeGetQuota` after you remove coarse-grained authz.



src/master/master.hpp (line 1024)
<https://reviews.apache.org/r/47274/#comment197383>

    Once you remove coarse-grained authz, you can prepare the authz action in `status()` and
hence you don't need `principal` in `_status()`, right?



src/master/quota_handler.cpp (lines 441 - 447)
<https://reviews.apache.org/r/47274/#comment197389>

    Do we really need this copy? Or we can directly use `master->quotas`?



src/master/quota_handler.cpp (lines 449 - 450)
<https://reviews.apache.org/r/47274/#comment197385>

    Please blank line between comment sections, e.g.:
    ```
    // The quick, brown fox jumps over a lazy dog. DJs flock by when MTV
    // ax quiz prog. Junk MTV quiz graced by fox whelps. Bawds jog, flick
    // quartz, vex nymphs.
    //
    // NOTE: Waltz, bad nymph, for quick jigs vex! Fox nymphs grab quick-jived
    // waltz. Brick quiz whangs jumpy veldt fox. Bright vixens jump; fowl quack.
    //
    // TODO(zhitao): Quick wafting zephyrs vex bold Jim. Quick zephyrs blow,
    // vexing daft Jim. Sex-charged fop blew my junk TV quiz. How quickly daft
    // jumping zebras vex. Two driven jocks help fax my big quiz.
    ```



src/master/quota_handler.cpp (line 454)
<https://reviews.apache.org/r/47274/#comment197388>

    We indent by 4 spaces if the previous line was a function call ending by '('



src/master/quota_handler.cpp (lines 459 - 470)
<https://reviews.apache.org/r/47274/#comment197384>

    Let's make it a continuation. Since you'll be moving this code to `status()`, you can
reuse `_status()` for continuation.



src/master/quota_handler.cpp (line 463)
<https://reviews.apache.org/r/47274/#comment197387>

    How about a comment here:
    ```
    // Create an entry (including role and resources) for each quota,
    // except those filtered out based on the authorizer's response.
    ```



src/master/quota_handler.cpp (line 464)
<https://reviews.apache.org/r/47274/#comment197386>

    If we eliminate intermediate vector, we will be looping thorugh hashmap and list simultaneously.
I can't think of a nice way to do it; we should probably use `for` with iterators.



src/master/quota_handler.cpp (line 519)
<https://reviews.apache.org/r/47274/#comment197382>

    Blank line



src/tests/master_quota_tests.cpp (line 1301)
<https://reviews.apache.org/r/47274/#comment197391>

    "previous authorized quota"? Do you want to say "previously requested quota"?


- Alexander Rukletsov


On May 12, 2016, 12:48 a.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47274/
> -----------------------------------------------------------
> 
> (Updated May 12, 2016, 12:48 a.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/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
> 
>


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