mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Will Rouesnel <w.roues...@gmail.com>
Subject Re: Review Request 51257: Add external process container logger.
Date Mon, 26 Sep 2016 14:14:29 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message