----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51257/#review150747 -----------------------------------------------------------
Almost there :) src/Makefile.am (line 1954) <https://reviews.apache.org/r/51257/#comment218803> nano-nit: The ending slash should line up on ReviewBoard. Your editor needs to show tabs as 8-spaces to have a similar display. src/Makefile.am (line 2276) <https://reviews.apache.org/r/51257/#comment218816> Reviewbot is complaining about your test script. It is running a `make dist-check`, which tars up the sources, untars them in a separate folder and builds that folder. ``` E0925 23:15:17.706635 29168 lib_externallogger.cpp:290] Failed to parse parameters: Cannot find: /mesos/mesos-1.1.0/src/tests/container_logger_external.sh ``` Your test script should be added here. src/slave/container_loggers/lib_externallogger.hpp (lines 48 - 50) <https://reviews.apache.org/r/51257/#comment218809> Realized that `--external_logger_cmd` may mislead the user into thinking shell commands work too. (The user would need to write a script to do that, rather than passing in the shell command directly here.) `--external_logger_binary` may be better. You can even add an alias (the `None()` argument) for `--external_logger_script`. src/slave/container_loggers/lib_externallogger.hpp (line 52) <https://reviews.apache.org/r/51257/#comment218807> Avoid the C-style cast in favour of: ``` static_cast<const std::string*>(nullptr), ``` src/slave/container_loggers/lib_externallogger.hpp (lines 55 - 65) <https://reviews.apache.org/r/51257/#comment218810> Nit: Indent with two spaces. src/slave/container_loggers/lib_externallogger.hpp (line 58) <https://reviews.apache.org/r/51257/#comment218808> s/the specified logger/the specified command/ s/executable by Mesos/an executable./ src/slave/container_loggers/lib_externallogger.hpp (lines 84 - 86) <https://reviews.apache.org/r/51257/#comment218815> I'd mention that this field is also prefixed. src/slave/container_loggers/lib_externallogger.cpp (lines 84 - 86) <https://reviews.apache.org/r/51257/#comment218812> Let's just use `flags.mesos_field_prefix;` where needed, instead of copying it into a local variable. src/slave/container_loggers/lib_externallogger.cpp (lines 90 - 92) <https://reviews.apache.org/r/51257/#comment218813> ``` environment[prefix + "SANDBOX_DIRECTORY"] = sandboxDirectory; ``` src/slave/container_loggers/lib_externallogger.cpp (line 99) <https://reviews.apache.org/r/51257/#comment218814> We should prefix this one too. - Joseph Wu On Sept. 25, 2016, 11:11 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, 11:11 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 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 > >
