----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72680 -----------------------------------------------------------
Looks much cleaner on first pass, but I'm still trying to figure out if you really need the whole Authenticator->AuthenticatorProcess->AuthenticatorSession->AuthenticatorSessionProcess hierarchy, or if just 2-3 of those would suffice. What's your reasoning? src/master/master.hpp <https://reviews.apache.org/r/27760/#comment118741> So glad this doesn't have to be `string> > > >&` src/authentication/cram_md5/authenticator.hpp <https://reviews.apache.org/r/27760/#comment118742> Comment should end in punctuation. src/authentication/cram_md5/authenticator.hpp <https://reviews.apache.org/r/27760/#comment118743> Why aren't these method implementations in an `authenticator.cpp`? src/authentication/cram_md5/authenticator.hpp <https://reviews.apache.org/r/27760/#comment118744> Should this still `process::terminate(process, false)` if the short term fix is now in `~CRAMMD5AuthenticatorSession`? The 'false' injects the 'terminate' at the end of the process' queue, so other in-flight events get handled first (semi-graceful shutdown). src/authentication/cram_md5/authenticator.hpp <https://reviews.apache.org/r/27760/#comment118746> What's the reasoning for this being a `static*`? Why wouldn't a plain old `Option<Error>` work? src/master/master.hpp <https://reviews.apache.org/r/27760/#comment118747> "... and its string will hold the authenticated principal." src/master/master.cpp <https://reviews.apache.org/r/27760/#comment118750> Not sure why this couldn't be merged into `_authenticate()`. What really needs to be done asynchronously? ... Oh, is this in case something like `Master::finalize()` discards the outer future, then the inner (authenticate) future will also be discarded? Worth a comment? src/master/master.cpp <https://reviews.apache.org/r/27760/#comment118748> `&& authenticate.get().isSome()`? src/master/master.cpp <https://reviews.apache.org/r/27760/#comment118749> `else if`? Do you really want to call discard again if already discarded? - Adam B On Feb. 13, 2015, 7:20 a.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27760/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2015, 7:20 a.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/authentication/cram_md5/authenticator.hpp 7578ea1 > 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 > >
