----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26857/#review57756 -----------------------------------------------------------
Looks great! Just a few minor nits and a suggestion of Try<Nothing> instead of Option<Error>. src/Makefile.am <https://reviews.apache.org/r/26857/#comment98623> Once we move the authenticatee into the authentication module, we should be able to make this library only build conditionally, so users without sasl can still build. Please add a TODO. src/authentication/authenticator.hpp <https://reviews.apache.org/r/26857/#comment98624> Where does "MODULE" come in? Maybe __AUTHENTICATION_AUTHENTICATOR_HPP__ instead? Or just __AUTHENTICATOR_HPP__ if it's "AUTHENTICATION" is implicit enough. src/authentication/authenticator.hpp <https://reviews.apache.org/r/26857/#comment98626> (Why) Is this intended to sit at the top-level namespace, even outside of namespace mesos? src/authentication/authenticator.hpp <https://reviews.apache.org/r/26857/#comment98625> Why not use a Try<Nothing> instead? It seems we use Option<Error> for a validation/verification function that we expect to return errors in normal operation, whereas we use Try<Nothing> for functions that should normally work, but might throw an error. src/authentication/authenticator.hpp <https://reviews.apache.org/r/26857/#comment98628> s/> >/>>/ Apparently modern compilers have finally figured out how to read this properly, so we'll be lazily migrating to ">>" src/authentication/cram_md5/authenticator/authenticator.hpp <https://reviews.apache.org/r/26857/#comment98633> Does authenticator.hpp really need to go into an authenticator subdirectory? Seems like cram_md5/authenticator.hpp|cpp should be fine, even if we also have authenticatee.hpp|cpp in the same directory. src/authentication/cram_md5/authenticator/authenticator.hpp <https://reviews.apache.org/r/26857/#comment98627> Wrap clientPid to the next line, so it can be aligned with credentials. Or indent credentials to align with clientPid, if you have room. src/authentication/cram_md5/authenticator/authenticator.hpp <https://reviews.apache.org/r/26857/#comment98629> s/> >/>>/g src/authentication/cram_md5/authenticator/authenticator.hpp <https://reviews.apache.org/r/26857/#comment98630> s/> >/>>/ src/authentication/cram_md5/authenticator/authenticator.hpp <https://reviews.apache.org/r/26857/#comment98632> Why not just do this in the class declaration if it's just a simple one-liner? - Adam B On Oct. 21, 2014, 5:59 a.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26857/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2014, 5:59 a.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. > > > Bugs: MESOS-1889 > https://issues.apache.org/jira/browse/MESOS-1889 > > > Repository: mesos-git > > > Description > ------- > > Introducing the CRAM-MD5 SASL authenticator module based on the former > sasl/authenticator. > > > Diffs > ----- > > src/Makefile.am c44a9ad > src/authentication/authenticator.hpp PRE-CREATION > src/authentication/cram_md5/authenticator/authenticator.hpp PRE-CREATION > src/authentication/cram_md5/authenticator/authenticator.cpp PRE-CREATION > src/module/manager.cpp 2727fe0 > src/sasl/authenticator.hpp 6f4d3db > src/sasl/secrets.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/26857/diff/ > > > Testing > ------- > > make check > > NOTE all three CRAM-MD5 authenticator module related RRs need to get applied > before running make check. > > > Thanks, > > Till Toenshoff > >
