mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 51257: Add external process container logger.
Date Wed, 28 Sep 2016 19:31:32 GMT

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


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