> On March 24, 2017, 2:58 p.m., Benjamin Mahler wrote: > > 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.
Thanks! - The logging is easy to fix. We can add such logging for the agent registration as well. - Pending framework registration re-ordering: It looks to me we are OK for now because we only have one hop in the async flow (subscribe -> authorizer -> _subscribe) and do we [this](https://github.com/apache/mesos/blob/1bb7ed28977d9b03c6a9162e8d8d10c7420a47f9/src/master/master.cpp#L2902-L2911) in the 2nd step. If we later add frameworks to the registrar we would need more hops and need to do something similar to what this patch does. - Can authorization futures be re-ordered? At least with the local authorizer they are executed by the same authorizer actor that satifies the futures. I was thinking more about other events like ExitedEvent being enqueued between duplicate subscribes and registrations. - The "queueing up" logic: I don't see how `registerSlave` retries can themsevles get re-ordered. Perhaps two `Master::registerSlave` dispatches enqueued by the authenticator can have an UnregisterSlaveMessage go between then due to the async step but `registering` and `reregistering` seem to be able to handle it. > On March 24, 2017, 2:58 p.m., Benjamin Mahler wrote: > > src/master/master.hpp > > Line 1640 (original), 1665-1668 (patched) > > <https://reviews.apache.org/r/57535/diff/5/?file=1671256#file1671256line1665> > > > > It looks like the comment about not answering questions about these > > transitioning agents was removed, can we restore it? Given the recent changes in /r/52083/, Perhaps it's best to put the comment above `recovered`? ``` // Slaves that have been recovered from the registrar after master // failover. Slaves are removed from this collection when they // either re-register with the master or are marked unreachable // because they do not re-register before `recoveredTimer` fires. // We must not answer questions related to these slaves (e.g., // during task reconciliation) until we determine their fate. hashmap<SlaveID, SlaveInfo> recovered; ``` - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57535/#review170056 ----------------------------------------------------------- On March 28, 2017, 1:40 a.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57535/ > ----------------------------------------------------------- > > (Updated March 28, 2017, 1:40 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/6/ > > > 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 > >
