----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22284/#review45180 -----------------------------------------------------------
Ship it! src/master/master.hpp <https://reviews.apache.org/r/22284/#comment79889> Ditto my comment on the previous review about saying validation fails for the invalid case: // Returns None if the framework is valid. // Returns a validation Error if the framework is invalid. // Returns a Failure if an authorization failure occurs. src/master/master.cpp <https://reviews.apache.org/r/22284/#comment79890> This could be re-used from your other change right? src/master/master.cpp <https://reviews.apache.org/r/22284/#comment79891> Remove period? src/master/master.cpp <https://reviews.apache.org/r/22284/#comment79892> Indentation is off here. src/master/master.cpp <https://reviews.apache.org/r/22284/#comment79893> Oh, now I see why you wanted to pass the failure message. :) src/master/master.cpp <https://reviews.apache.org/r/22284/#comment79894> Do we want to log the framework id here and below? src/master/master.cpp <https://reviews.apache.org/r/22284/#comment79897> Using "new" here and "another" below? src/master/master.cpp <https://reviews.apache.org/r/22284/#comment79898> "Master" src/master/master.cpp <https://reviews.apache.org/r/22284/#comment79900> For all these cases, rather than "before we are here" or "interim", how about a precise description: // This occurs when another authentication request arrived while validation was in progress. Does that seem like a more precise way to write these comments? src/tests/master_authorization_tests.cpp <https://reviews.apache.org/r/22284/#comment79904> Nice test!!! src/tests/master_authorization_tests.cpp <https://reviews.apache.org/r/22284/#comment79903> A bit confusing, how about calling the futures: 'authorize1' and 'authorize2'? Would read better in the AWAIT_ statements below. src/tests/master_authorization_tests.cpp <https://reviews.apache.org/r/22284/#comment79906> It seems like the duplicate tests should be grouped together and the removed tests should be grouped together. It is trivial to understand the 'DuplicateReregistration' test if reading it immediately after the 'DuplicateRegistration' test. src/tests/master_authorization_tests.cpp <https://reviews.apache.org/r/22284/#comment79907> Ditto naming. Is the lack of 'future1' here to imply that there is an implicit initial authorization? src/tests/master_authorization_tests.cpp <https://reviews.apache.org/r/22284/#comment79908> Ditto naming. - Ben Mahler On June 9, 2014, 10:09 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22284/ > ----------------------------------------------------------- > > (Updated June 9, 2014, 10:09 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-1307 > https://issues.apache.org/jira/browse/MESOS-1307 > > > Repository: mesos-git > > > Description > ------- > > Added authorization for roles during framework (re-)registration. > > > Diffs > ----- > > src/master/flags.hpp 486335970ef05b345c5584ac012dde63437ef149 > src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3 > src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f > src/tests/master_authorization_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/22284/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
