> On Sept. 7, 2016, 7:34 p.m., Joseph Wu wrote: > > src/slave/container_loggers/lib_externallogger.cpp, line 152 > > <https://reviews.apache.org/r/51257/diff/2/?file=1485816#file1485816line152> > > > > You should `SETSID` here. Otherwise, you will lose logging (the > > external command will die) when the agent dies.
Good point - I'd say this also warrants a test-case. I'll get it implemented. > 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. This doesn't actually compile for me at the moment, unless something's changed on master I'm not rebased against? - Will ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51257/#review148063 ----------------------------------------------------------- On Sept. 25, 2016, 3:56 a.m., Will Rouesnel wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51257/ > ----------------------------------------------------------- > > (Updated Sept. 25, 2016, 3:56 a.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 69e56551f0adca6d6a9811cafea9a8d3c56d1df9 > 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 > e8f934106510fe02b8b92be19c918a1e5c0b78fd > 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 > >
