This is an automatically generated e-mail. To reply, visit:

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)

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

include/mesos/authorizer/authorizer.proto (line 62)


src/authorizer/local/authorizer.cpp (lines 219 - 229)

    Ditto (move up before set quota).

src/master/master.hpp (line 1013)

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

src/master/master.hpp (line 1024)

    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)

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

src/master/quota_handler.cpp (lines 449 - 450)

    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)

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

src/master/quota_handler.cpp (lines 459 - 470)

    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)

    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)

    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)

    Blank line

src/tests/master_quota_tests.cpp (line 1301)

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

- 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

Reply via email to