----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19372/#review37850 -----------------------------------------------------------
Thanks Yan! src/master/master.hpp <https://reviews.apache.org/r/19372/#comment69619> Can you just forward declare these in the header? Otherwise I would be inclined to leave the implementation in the header given how small these are and how much easier it is to understand them. We already do this in Master for simple things like 'Slave' and 'Framework'. src/master/registrar.hpp <https://reviews.apache.org/r/19372/#comment69615> Why is this no longer templated? I can envision us overloading our Registrar apply() call based on the type of Operation, as we add more keys outside the Registry: Future<bool> apply(Owned<Operation<Registry> > operation); Future<bool> apply(Owned<Operation<Reservations> > operation); Future<bool> apply(Owned<Operation<Repairs> > operation); src/master/registrar.hpp <https://reviews.apache.org/r/19372/#comment69617> This may prove useful with future operations, but the difference between an invalid operation and an operation that cannot be applied is a bit fuzzy. How about for now you add some sanity CHECKs for slave IDs in the constructors for the Master's operations, and we can think about validation in a later follow up? src/master/registrar.hpp <https://reviews.apache.org/r/19372/#comment69618> What is 'info'? ;) src/master/registrar.cpp <https://reviews.apache.org/r/19372/#comment69613> Okay, let's just call this 'applied' then? src/master/registrar.cpp <https://reviews.apache.org/r/19372/#comment69614> Remove the 'this->' if you're changing the name of the 'operations' argument. - Ben Mahler On March 20, 2014, 12:12 a.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19372/ > ----------------------------------------------------------- > > (Updated March 20, 2014, 12:12 a.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-1112 > https://issues.apache.org/jira/browse/MESOS-1112 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7 > src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437 > src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c > src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1 > src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e > > Diff: https://reviews.apache.org/r/19372/diff/ > > > Testing > ------- > > make check GTEST_FILTER=*Registrar* > > > Thanks, > > Jiang Yan Xu > >
