----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57535/#review170056 -----------------------------------------------------------
Nice tests! Overall approach looks good to me. A few comments below. Unrelated to your changes I noticed a few issues: There are some inconsistencies between the framework and agent paths. For example, we don't log when we receive an agent's (re-)registration message but we log the framework's subscription, not sure why we did that. Also, since we don't track a framework's pending subscription, if the authorization futures are re-ordered we could process subscre 2 before subscre 1, but this is unrelated to your change here. The "queueing up" logic (example [here](https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L5344-L5345)) also allows re-ordering. src/master/master.hpp Line 1640 (original), 1665-1668 (patched) <https://reviews.apache.org/r/57535/#comment242827> It looks like the comment about not answering questions about these transitioning agents was removed, can we restore it? src/master/master.cpp Lines 3642-3656 (patched) <https://reviews.apache.org/r/57535/#comment242828> Any reason there's no logging here? It would be nice to log consistently with the framework authorization path: https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L2201-L2203 src/master/master.cpp Lines 3654 (patched) <https://reviews.apache.org/r/57535/#comment242829> Maybe a little comment about why we don't have a request object here? - Benjamin Mahler On March 22, 2017, 12:48 a.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57535/ > ----------------------------------------------------------- > > (Updated March 22, 2017, 12:48 a.m.) > > > Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin > Mahler, Greg Mann, and Vinod Kone. > > > Bugs: MESOS-7097 > https://issues.apache.org/jira/browse/MESOS-7097 > > > Repository: mesos > > > Description > ------- > > Applied RegisterAgent ACL to the master. > > > Diffs > ----- > > src/master/master.hpp d92c8adef79d997f255cf26ebd10ab0e87da8413 > src/master/master.cpp 43e9d26167c1f405329ea05224c22e7b8c65315f > src/tests/master_authorization_tests.cpp > 1a0285a3f345ef21a8256d4123d8bb684f538da4 > > > Diff: https://reviews.apache.org/r/57535/diff/5/ > > > Testing > ------- > > make check. > > The tests added here cover some basic scenarios, I have more tests but will > add them when MESOS-7244 is fixed. > > > Thanks, > > Jiang Yan Xu > >
