> On March 7, 2014, 12:07 a.m., Vinod Kone wrote: > > src/master/master.cpp, lines 259-262 > > <https://reviews.apache.org/r/18381/diff/2/?file=511057#file511057line259> > > > > New line. > > > > Also, this file doesn't use multi-line style comments. Can yo just use > > "//"? > > Adam B wrote: > Any thoughts on using Doxygen-style comments > (http://www.doxygen.nl/docblocks.html#cppblock), at least on public methods? > Better (and more accessible) internal documentation would help the rest of us > get up to speed on the code internals.
I think Doxygen-style comments is a great idea. But lets create a ticket specifically for that so that we can discuss 1) whether we want to do it and 2) how we want to do it. Sounds good? - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18381/#review36419 ----------------------------------------------------------- On March 7, 2014, 9:11 a.m., Adam B wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18381/ > ----------------------------------------------------------- > > (Updated March 7, 2014, 9:11 a.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-804 > https://issues.apache.org/jira/browse/MESOS-804 > > > Repository: mesos-git > > > Description > ------- > > Added authentication support for slaves. > Fixes MESOS-804. > > Open Issues: > - Should AuthenticateMessage be replaced with AuthenticateFrameworkMessage, > or specify an Authenticatee type as coded here? > - removeSlave vs. deactivate(Slave): Some uses of removeSlave might benefit > from just deactivating if checkpointing is enabled. > - We currently deactivate a registered slave/framework when a new > authenticate message comes in, even if the new authentication message is a > failure/fake. Will file a new JIRA for this security hole. > - When multiple entries for the same principal exist in the credentials file, > only the last entry is used. Acceptable behavior, but shouldn't this be > documented? > > > Diffs > ----- > > src/master/flags.hpp 159b2de > src/master/master.hpp 49a3e15 > src/master/master.cpp f7ba9aa > src/messages/messages.proto c26a3d0 > src/sasl/authenticatee.hpp 42a4eba > src/sched/sched.cpp 00f6307 > src/slave/flags.hpp e4d98a5 > src/slave/slave.hpp 01b80df > src/slave/slave.cpp b350df4 > src/tests/authentication_tests.cpp 127c5e6 > src/tests/cluster.hpp d1bf680 > src/tests/mesos.cpp 96adeac > src/tests/sasl_tests.cpp 945426d > src/tests/slave_recovery_tests.cpp 40a9599 > > Diff: https://reviews.apache.org/r/18381/diff/ > > > Testing > ------- > > make check; manually tested flatfile slave authentication success/failure. > Added new slave authentication unit tests in authentication_tests.cpp. > > > Thanks, > > Adam B > >
