> On Feb. 17, 2015, 7:37 a.m., Adam B wrote: > > 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?
We want the future to be usable for canceling the authentication progress. So the "onAny" callback of the inner future is supposed to terminate the "AuthenticatorSessionProcess" - termination here should be entirely deterministically, so it should not just signal a termination but also wait for it to be accomplished. Waiting for a process within the same process certainly would introduce a deadlock. So I need another process to cover this. We want the entire session management to be covered by the authenticator itself, hence we can not do this within the Master process. Hope I am making sense here. Please keep asking as I would love to strip this down, I just do not see any option right now -- maybe I had just worked on this stuff for too long :) > On Feb. 17, 2015, 7:37 a.m., Adam B wrote: > > src/authentication/cram_md5/authenticator.hpp, lines 112-113 > > <https://reviews.apache.org/r/27760/diff/14/?file=863170#file863170line112> > > > > Why aren't these method implementations in an `authenticator.cpp`? Good point. I was sticking to the old structure far too long. Looks much nicer when splitting. > On Feb. 17, 2015, 7:37 a.m., Adam B wrote: > > src/authentication/cram_md5/authenticator.hpp, line 533 > > <https://reviews.apache.org/r/27760/diff/14/?file=863170#file863170line533> > > > > 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). Correct, I think it should be a regular terminate at this point now (getting pushed into the front of the queue). > On Feb. 17, 2015, 7:37 a.m., Adam B wrote: > > src/authentication/cram_md5/authenticator.hpp, line 545 > > <https://reviews.apache.org/r/27760/diff/14/?file=863170#file863170line545> > > > > What's the reasoning for this being a `static*`? Why wouldn't a plain > > old `Option<Error>` work? That way, if an error occured in the "once"-covered case, then any additional calls to initialize (e.g. by another instance) would still have access to that former, "once"-covered error and be able to return it. > On Feb. 17, 2015, 7:37 a.m., Adam B wrote: > > src/master/master.cpp, lines 3932-3935 > > <https://reviews.apache.org/r/27760/diff/14/?file=863174#file863174line3932> > > > > 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? That is exactly what it is supposed to do, correct. Will add a comment. > On Feb. 17, 2015, 7:37 a.m., Adam B wrote: > > src/master/master.cpp, line 3937 > > <https://reviews.apache.org/r/27760/diff/14/?file=863174#file863174line3937> > > > > `&& authenticate.get().isSome()`? How about this: ``` if (future.isReady()) { // We should arrive at this point only if the inner future is both, // ready and has a container value set. CHECK(authenticate.isReady()); CHECK(authenticate.get().isSome()); [...] ``` > On Feb. 17, 2015, 7:37 a.m., Adam B wrote: > > src/master/master.cpp, lines 3943-3944 > > <https://reviews.apache.org/r/27760/diff/14/?file=863174#file863174line3943> > > > > `else if`? Do you really want to call discard again if already > > discarded? It should be a noop but you are certainly right, should be explicit by using such ```else if``` - Till ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review72680 ----------------------------------------------------------- On Feb. 13, 2015, 3:20 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27760/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2015, 3:20 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/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 > >
