----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26859/#review59088 -----------------------------------------------------------
Looks really good, but I think we need one more round to clear out the references to 'modules' when using generic 'authenticators'. src/examples/test_authenticator_module.cpp <https://reviews.apache.org/r/26859/#comment100396> Strange, I didn't see these merge annotations in your previous patch that introduced this file. Not sure why it shows up in this diff, but as long as the lhs never gets checked in, we're fine. src/master/flags.hpp <https://reviews.apache.org/r/26859/#comment100400> Why does this even talk about modules? It should talk about authenticators, and the --modules flag can talk about modules. And if there's a module with a new authenticator, you can tell them in your module's howto doc to use --modules to load it and --authenticators to enable it. Suggested new text: "Comma separated list of authenticator(s) to use. Only uses the first authenticator in the list for now." src/master/master.hpp <https://reviews.apache.org/r/26859/#comment100401> These are no longer necessarily 'modules', so maybe rename the variable to 'authenticatorNames' src/master/master.cpp <https://reviews.apache.org/r/26859/#comment100403> s/module // src/master/master.cpp <https://reviews.apache.org/r/26859/#comment100404> Shouldn't you also check that !authenticatorNames.empty() in case somebody overrides the default to set --authenticators=""? Error and exit src/master/master.cpp <https://reviews.apache.org/r/26859/#comment100402> It would be nice to see the name of the authenticator being used: "Multiple authenticators are not supported. Only using: " + authenticatorNames[0] src/master/master.cpp <https://reviews.apache.org/r/26859/#comment100406> Let's get rid of the magic string and make "crammd5" a string constant. src/master/master.cpp <https://reviews.apache.org/r/26859/#comment100405> Can we reword this to not discuss 'modules'? Maybe just "Authenticator '" << authenticatorNames[0] << "' not found." If you want to suggest how to fix it, you could suggest they check the spelling (compare to "crammd5") or verify that they have the authenticator loaded in a module from --modules" src/master/master.cpp <https://reviews.apache.org/r/26859/#comment100407> Is this an error to 'load' or an error to 'create'? s/load/create/? src/master/master.cpp <https://reviews.apache.org/r/26859/#comment100409> 'auth' is to short/ambiguous. Use authenticator and authenticator_. You can pick which one is the Owned ptr vs. the normal pointer. src/master/master.cpp <https://reviews.apache.org/r/26859/#comment100412> Blank like between these; pretty much always want a blank line before comments. src/tests/cram_md5_authentication_tests.cpp <https://reviews.apache.org/r/26859/#comment100450> Two blank lines between tests - 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/26859/ > ----------------------------------------------------------- > > (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 > ------- > > Enables selecting a module based authenticator via the new --authenticators > flag for mesos master. > > Additionally, all "> >" have been fixed towards ">>" in master.hpp and > master.cpp. > > > Diffs > ----- > > src/examples/test_authenticator_module.cpp PRE-CREATION > src/master/flags.hpp c931fd9 > src/master/master.hpp b1a2cd0 > src/master/master.cpp 762d2ff > src/tests/cram_md5_authentication_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/26859/diff/ > > > Testing > ------- > > make check > > NOTE all four CRAM-MD5 authenticator module related RRs need to get applied > before running make check. > > > Thanks, > > Till Toenshoff > >
