> On Sept. 7, 2016, 7:34 p.m., Joseph Wu wrote: > > src/slave/container_loggers/lib_externallogger.hpp, lines 52-61 > > <https://reviews.apache.org/r/51257/diff/2/?file=1485815#file1485815line52> > > > > This flag should be required. You can use `flags.add` like: > > ``` > > add(&external_logger_cmd, > > "external_logger_cmd", > > None(), > > "Path to the external command which will read STDIN for logs", > > [](const std::string& executablePath) -> Option<Error> { > > if (!os::exists(executablePath)) { > > return Error("Cannot find: " + executablePath); > > } > > return None(); > > }); > > ``` > > > > The `None()` as the third argument is an alias. We need this to > > disambiguate the various overloads of `flags.add`. > > > > --- > > > > Also, add a newline after this block. > > Will Rouesnel wrote: > This doesn't actually compile for me at the moment, unless something's > changed on master I'm not rebased against?
Ah, looks like the sample was missing an extra nullptr to for the default value disambiguation. - Will ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51257/#review148063 ----------------------------------------------------------- On Sept. 25, 2016, 6:11 p.m., Will Rouesnel wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51257/ > ----------------------------------------------------------- > > (Updated Sept. 25, 2016, 6:11 p.m.) > > > Review request for mesos and Joseph Wu. > > > Bugs: MESOS-6003 > https://issues.apache.org/jira/browse/MESOS-6003 > > > Repository: mesos > > > Description > ------- > > Adds the external process container logger. This functions like the > logrotate container logger, but instead calls a custom host binary > (or script) and passes the executorInfo as JSON via environment > variables. This makes it very easy for users to configure custom > logging solutions, without needing to write and maintain logging > modules. > > Tests passing and complete. > > > Diffs > ----- > > src/Makefile.am 410b5f2e5bc50a5aa7856645c8cf4a3e43505a84 > src/slave/container_loggers/lib_externallogger.hpp PRE-CREATION > src/slave/container_loggers/lib_externallogger.cpp PRE-CREATION > src/tests/container_logger_external.sh PRE-CREATION > src/tests/container_logger_tests.cpp > f76117230e0517ddc3cb8e0bf482085fad6950d2 > src/tests/module.hpp e661d95fa44fc1aedfe83c564c826d5b7d32c85b > src/tests/module.cpp 5b83fd6358ddea4c9d849b8992e1a6040ef74505 > > Diff: https://reviews.apache.org/r/51257/diff/ > > > Testing > ------- > > Adds ContainerLoggerTest.EXTERNAL_RecieveEnvironment which tests all major > parameters of the change. > > A synthetic external container logger is provided by the script > tests/container_logger_external.sh which is setup to fail if any important > output is unavailable to the logging process. > > The other basic checks are duplicated from the Logrotate container logger, > from where this change inherits a lot of its code. > > > Thanks, > > Will Rouesnel > >
