mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.
Date Thu, 17 Dec 2015 20:56:37 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review110826
-----------------------------------------------------------

Ship it!


Only high-level comment is that the _container_ logger talks a lot about executor's and tasks,
I need to look into some of the upcoming reviews, but why isn't it sufficient to only talk
about containers?


include/mesos/slave/container_logger.hpp (line 40)
<https://reviews.apache.org/r/41002/#comment171027>

    s/executors/containers/
    
    s/of tasks launched by default executors/of tasks launched without an executor (that implicitly
use the command executor)/



include/mesos/slave/container_logger.hpp (lines 43 - 44)
<https://reviews.apache.org/r/41002/#comment171028>

    Make this part a TODO?



include/mesos/slave/container_logger.hpp (lines 73 - 75)
<https://reviews.apache.org/r/41002/#comment171029>

    Can we document what the failure case of using an IO::PIPE means? As in, what if someone
specifies an IO::PIPE? What breaks down?



include/mesos/slave/container_logger.hpp (line 125)
<https://reviews.apache.org/r/41002/#comment171030>

    s/Error/Failure/
    s/error/failure/



include/mesos/slave/container_logger.hpp (line 128)
<https://reviews.apache.org/r/41002/#comment171031>

    What's the long term plan for this? Will we have multiple container loggers running simultaneously
at some point?
    
    I'd like you to capture a TODO here to follow up with the semantics on executors that
can't be recovered. And let's roll this into the next phase so that we have consistent semantics
here.



include/mesos/slave/container_logger.hpp (line 135)
<https://reviews.apache.org/r/41002/#comment171032>

    s/launches an executor or a default executor launches a task/creates a container/



include/mesos/slave/container_logger.hpp (line 139)
<https://reviews.apache.org/r/41002/#comment171033>

    s/an executor or task/container/



include/mesos/slave/container_logger.hpp (line 155)
<https://reviews.apache.org/r/41002/#comment170823>

    Kill newline.


- Benjamin Hindman


On Dec. 15, 2015, 8:38 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
> 
> (Updated Dec. 15, 2015, 8:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas
Rampelberg.
> 
> 
> Bugs: MESOS-4087
>     https://issues.apache.org/jira/browse/MESOS-4087
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr,
and pipe it to some sink.
> 
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr")
located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated
logging solutions.
> 
> 
> Diffs
> -----
> 
>   include/mesos/slave/container_logger.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41002/diff/
> 
> 
> Testing
> -------
> 
> This is added to the Makefile later in the review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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