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