----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review73310 -----------------------------------------------------------
src/authentication/cram_md5/authenticator.hpp <https://reviews.apache.org/r/27760/#comment119590> Do we need this in the header? Can it be moved to cpp? src/authentication/cram_md5/authenticator.cpp <https://reviews.apache.org/r/27760/#comment119591> How about: ``` sessions.put(pid, session); return session->authenticate() .onAny(defer(..)); ``` src/authentication/cram_md5/authenticator.cpp <https://reviews.apache.org/r/27760/#comment119592> Can you add a comment on when this is possible? src/authentication/cram_md5/authenticator.cpp <https://reviews.apache.org/r/27760/#comment119593> What do you mean by "within our Once covered case"? Also, just put "subsequent" on the next line. src/authentication/cram_md5/authenticator.cpp <https://reviews.apache.org/r/27760/#comment119595> Do this at the very beginning. src/authentication/cram_md5/authenticator.cpp <https://reviews.apache.org/r/27760/#comment119594> Shouldn't this be protected by once() to avoid 2 different threads loading secrets at the same time? src/authentication/cram_md5/auxprop.hpp <https://reviews.apache.org/r/27760/#comment119596> s/persistence/properties/ src/authentication/cram_md5/auxprop.hpp <https://reviews.apache.org/r/27760/#comment119597> if you protect secrets::load() by Once, as suggested above, do we need this? src/master/master.hpp <https://reviews.apache.org/r/27760/#comment119598> can you name this something more descriptive than "inner"? Also comment this method. src/master/master.hpp <https://reviews.apache.org/r/27760/#comment119605> make this Option<Authenticator*> src/master/master.cpp <https://reviews.apache.org/r/27760/#comment119604> why this if condition for logging? src/master/master.cpp <https://reviews.apache.org/r/27760/#comment119607> I think seeing this log line in master log would not reveal much information on what's happening. How about LOG(ERROR) << "Received authentication request from " << pid << " but authenticator is not loaded"; src/master/master.cpp <https://reviews.apache.org/r/27760/#comment119620> Did you mix up "inner" and "outer" here? src/master/master.cpp <https://reviews.apache.org/r/27760/#comment119621> Why do we need this 'onAny'? Why can't 'authenticated' be set in __authenticate()? src/master/master.cpp <https://reviews.apache.org/r/27760/#comment119614> Can you rephrase these log messages to something meaninguful when one looks at master log? - Vinod Kone On Feb. 19, 2015, 2:02 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27760/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2015, 2:02 p.m.) > > > Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. > > > Bugs: MESOS-2050 > https://issues.apache.org/jira/browse/MESOS-2050 > > > Repository: mesos > > > Description > ------- > > The initial design and implementation of the authenticator module interface > caused issues and was not optimal for heavy lifting setup of external > dependencies. By introducing a two fold design, this has been decoupled from > the authentication message processing. The new design also gets us back on > track to the goal of makeing SASL a soft dependency of mesos. > > > Diffs > ----- > > include/mesos/authentication/authenticator.hpp f66217a > src/Makefile.am d372404 > src/authentication/cram_md5/authenticator.hpp 7578ea1 > src/authentication/cram_md5/authenticator.cpp PRE-CREATION > src/authentication/cram_md5/auxprop.hpp d036b11 > src/authentication/cram_md5/auxprop.cpp 5ff9755 > src/master/master.hpp 6a39df0 > src/master/master.cpp f10a3cf > src/tests/cram_md5_authentication_tests.cpp dd102dc > > Diff: https://reviews.apache.org/r/27760/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Till Toenshoff > >
