----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26857/#review59073 -----------------------------------------------------------
Looks great! Minor tweaks for you to consider, then we can commit it. src/Makefile.am <https://reviews.apache.org/r/26857/#comment100357> I'm not sure what we're doing here. It looks like test_authenticator_module.cpp is the only source, so I'd expect something like the libtestisolator stuff above, but the language seems to imply that the CRAM-MD5 authentication (not just authenticator) modules (not just for test) all lives in this library. For now, I think the comment and names should express that this is just for the test authenticator module, and then we can update the library name and comment when we actually add the authenticatee and split all of the authentication out of mesos into this library (as per your TODOs). src/authentication/authenticator.hpp <https://reviews.apache.org/r/26857/#comment100359> __AUTHENTICATION_AUTHENTICATOR_HPP__? In case there's ever another "authenticator" not for authentication? Or this could be one of those super obvious ones (like master/master.hpp) where we don't need the directory name prefix because we never expect conflicts src/authentication/cram_md5/authenticator.hpp <https://reviews.apache.org/r/26857/#comment100382> s/authenticator/crammd5_authenticator/ in case we end up with multiple authenticator process types (+kerberos) running simultaneously src/authentication/cram_md5/authenticator.hpp <https://reviews.apache.org/r/26857/#comment100390> s/gotten/been/ src/authentication/cram_md5/authenticator.hpp <https://reviews.apache.org/r/26857/#comment100388> CHECK_NOTNULL src/examples/test_authenticator_module.cpp <https://reviews.apache.org/r/26857/#comment100392> org_apache_mesos_TestCRAMMD5Authenticator? Then when we add the real module, they won't conflict? src/module/manager.cpp <https://reviews.apache.org/r/26857/#comment100393> Maybe these should be alphabetized, to make it easier to find modules as this list grows. - Adam B On Oct. 29, 2014, 11:51 a.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26857/ > ----------------------------------------------------------- > > (Updated Oct. 29, 2014, 11:51 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. > > The former sasl/authenticator.hpp still remains as part of libmesos but now > is located at authentication/cram_md5. > > > Diffs > ----- > > src/Makefile.am f177d87 > src/authentication/authenticator.hpp PRE-CREATION > src/authentication/cram_md5/authenticator.hpp PRE-CREATION > src/examples/test_authenticator_module.cpp PRE-CREATION > src/module/authenticator.hpp PRE-CREATION > src/module/manager.cpp 7a6c884 > > Diff: https://reviews.apache.org/r/26857/diff/ > > > Testing > ------- > > make check > > NOTE all four CRAM-MD5 authenticator module related RRs need to get applied > before running make check. > > > Thanks, > > Till Toenshoff > >
