> On June 10, 2014, 1:01 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 994-1002 > > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line994> > > > > This could be re-used from your other change right?
not sure what you mean. this is reused. just moved it up. > On June 10, 2014, 1:01 a.m., Ben Mahler wrote: > > src/master/master.hpp, lines 662-665 > > <https://reviews.apache.org/r/22284/diff/2/?file=606171#file606171line662> > > > > 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. done. > On June 10, 2014, 1:01 a.m., Ben Mahler wrote: > > src/master/master.cpp, line 1034 > > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1034> > > > > Remove period? oops. > On June 10, 2014, 1:01 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1101-1102 > > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1101> > > > > Do we want to log the framework id here and below? it is registration. there is no framework id yet. > On June 10, 2014, 1:01 a.m., Ben Mahler wrote: > > src/master/master.cpp, line 1111 > > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1111> > > > > Using "new" here and "another" below? clarified the comments. > On June 10, 2014, 1:01 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1222-1223 > > <https://reviews.apache.org/r/22284/diff/2/?file=606172#file606172line1222> > > > > 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? better. fixed. > On June 10, 2014, 1:01 a.m., Ben Mahler wrote: > > src/tests/master_authorization_tests.cpp, lines 840-843 > > <https://reviews.apache.org/r/22284/diff/2/?file=606173#file606173line840> > > > > Ditto naming. > > > > Is the lack of 'future1' here to imply that there is an implicit > > initial authorization? yea. i didn't want the comment to say "second attempt" and the variable to be "future1". > On June 10, 2014, 1:01 a.m., Ben Mahler wrote: > > src/tests/master_authorization_tests.cpp, lines 724-727 > > <https://reviews.apache.org/r/22284/diff/2/?file=606173#file606173line724> > > > > A bit confusing, how about calling the futures: 'authorize1' and > > 'authorize2'? Would read better in the AWAIT_ statements below. hmm. not sure. what about promises? it seems weird if authorization promise is called 'promise' and its future is called 'authorize'. > On June 10, 2014, 1:01 a.m., Ben Mahler wrote: > > src/tests/master_authorization_tests.cpp, lines 818-822 > > <https://reviews.apache.org/r/22284/diff/2/?file=606173#file606173line818> > > > > 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. done - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22284/#review45180 ----------------------------------------------------------- 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 > >
