Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

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

(Updated Dec. 21, 2015, 4:49 p.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, 
and Thomas Rampelberg.


Changes
---

Change the restriction on `IO::PIPE` into a compile-time restriction.


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 (updated)
-

  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

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

> On Dec. 20, 2015, 6:21 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, lines 85-93
> > 
> >
> > I'm a little confused. Who executes this? The comment makes it read 
> > like there is some particular code that executes this, which makes me 
> > wonder why it needs to be a static function on `ContainerLogger`?
> > 
> > And why don't we want `SubprocessInfo::out/err` to use pipes? I thought 
> > that was the whole point?
> > 
> > Finally, killing the agent seems a little bit harsh. Couldn't the 
> > container just get killed?
> 
> Joseph Wu wrote:
> This is meant to enforce the `NOTE` in `SubprocessInfo` about how the 
> module may not specify `Subprocess::IO::PIPE()` as part of the return value.  
> 
> I talked with Joris about either making this a compile-time or run-time 
> check, and we decided that a compile-time check would be somewhat distracting 
> compared to a `validate` function like this.  (We would need to add a new 
> class that provides just `Subprocess::FD` and `Subprocess::PATH`.)
> 
> This function will exit on failure because it indicates that the chosen 
> `ContainerLogger` breaks that assumption.

Changed to a compile-time restriction, as discussed.


- Joseph


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


On Dec. 21, 2015, 4:49 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> ---
> 
> (Updated Dec. 21, 2015, 4:49 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
> 
>

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

> On Dec. 20, 2015, 6:21 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, lines 85-93
> > 
> >
> > I'm a little confused. Who executes this? The comment makes it read 
> > like there is some particular code that executes this, which makes me 
> > wonder why it needs to be a static function on `ContainerLogger`?
> > 
> > And why don't we want `SubprocessInfo::out/err` to use pipes? I thought 
> > that was the whole point?
> > 
> > Finally, killing the agent seems a little bit harsh. Couldn't the 
> > container just get killed?

This is meant to enforce the `NOTE` in `SubprocessInfo` about how the module 
may not specify `Subprocess::IO::PIPE()` as part of the return value.  

I talked with Joris about either making this a compile-time or run-time check, 
and we decided that a compile-time check would be somewhat distracting compared 
to a `validate` function like this.  (We would need to add a new class that 
provides just `Subprocess::FD` and `Subprocess::PATH`.)

This function will exit on failure because it indicates that the chosen 
`ContainerLogger` breaks that assumption.


- Joseph


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


On Dec. 20, 2015, 4:39 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> ---
> 
> (Updated Dec. 20, 2015, 4:39 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
> 
>

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, lines 73-75
> > 
> >
> > 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?
> 
> Joseph Wu wrote:
> Would it be reasonable to `ABORT` the agent if the module specifies a 
> `PIPE`?  Or perhaps make this a compile-time restriction (i.e. by introducing 
> a subclass of `Subprocess::IO`).
> 
> (Note: The check for `PIPE` isn't implemented yet.)

Added `ContainerLogger::validate`.


> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, line 128
> > 
> >
> > 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.
> 
> Joseph Wu wrote:
> For Docker command executors, there will be a container logger running 
> alongside the executor.  But it will only ever spawn one task (because the 
> executor will refuse >1 task).
> 
> In general, this means we could:
> 
> - Launch an executor that logs to the sandbox,
> - Restart the agent with a special container logger (say, to syslog),
> - Have the recovered executor continue logging to the sandbox, while new 
> executors log to syslog.

Note: We decided to remove the ContainerLogger from the executor (so now it 
only lives in the Containerizer, and only one module instance will exist per 
containerizer.)


- Joseph


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


On Dec. 20, 2015, 4:39 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> ---
> 
> (Updated Dec. 20, 2015, 4:39 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
> 
>

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

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

(Updated Dec. 20, 2015, 4:39 p.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, 
and Thomas Rampelberg.


Changes
---

Remove `subprocess` arguments except for pipes.  Added `validate` function.


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 (updated)
-

  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

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Benjamin Hindman]

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


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)


Make this part a TODO?



include/mesos/slave/container_logger.hpp (lines 73 - 75)


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)


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



include/mesos/slave/container_logger.hpp (line 128)


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)


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



include/mesos/slave/container_logger.hpp (line 139)


s/an executor or task/container/



include/mesos/slave/container_logger.hpp (line 155)


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

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > 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?

I wanted to be explicit about the scope of the module.  i.e. All 
executors/tasks run in containers, but not all containers are executors/tasks.

Changed to use "container" in the comments.


> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, lines 43-44
> > 
> >
> > Make this part a TODO?

I'll add a TODO to expose some sort of message or URL in the Mesos web UI.  

The module still needs to provide a log-reading interface (because write-only 
logs aren't very useful).  But Mesos won't know how to read said logs.


> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, lines 73-75
> > 
> >
> > 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?

Would it be reasonable to `ABORT` the agent if the module specifies a `PIPE`?  
Or perhaps make this a compile-time restriction (i.e. by introducing a subclass 
of `Subprocess::IO`).

(Note: The check for `PIPE` isn't implemented yet.)


> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, line 128
> > 
> >
> > 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.

For Docker command executors, there will be a container logger running 
alongside the executor.  But it will only ever spawn one task (because the 
executor will refuse >1 task).

In general, this means we could:

- Launch an executor that logs to the sandbox,
- Restart the agent with a special container logger (say, to syslog),
- Have the recovered executor continue logging to the sandbox, while new 
executors log to syslog.


- Joseph


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


On Dec. 17, 2015, 3:46 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> ---
> 
> (Updated Dec. 17, 2015, 3:46 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
> 
>

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

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

(Updated Dec. 17, 2015, 3:46 p.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, 
and Thomas Rampelberg.


Changes
---

Comment changes, per BenH's comments.


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 (updated)
-

  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

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

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

(Updated Dec. 15, 2015, 11:24 a.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, 
and Thomas Rampelberg.


Changes
---

Comment tweaks.  Changed `options` to an `Option`.  Moved call to 
`initialize` into `create`.


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 (updated)
-

  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

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

> On Dec. 14, 2015, 8:02 p.m., Timothy Chen wrote:
> > include/mesos/slave/container_logger.hpp, line 120
> > 
> >
> > What `captureOutput` are you referring to here?

Oops :)
This was renamed (twice) to `prepare`.


- Joseph


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


On Dec. 14, 2015, 3:46 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> ---
> 
> (Updated Dec. 14, 2015, 3:46 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
> 
>

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

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

(Updated Dec. 15, 2015, 12:38 p.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, 
and Thomas Rampelberg.


Changes
---

Remove `options` in favor of module parameters.


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 (updated)
-

  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

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

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

(Updated Dec. 14, 2015, 3:46 p.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, 
and Thomas Rampelberg.


Changes
---

Tweaked some comments.  Change `ExecutorID` to `ExecutorInfo`, so as to pass 
more information to the module.


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 (updated)
-

  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

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

> On Dec. 12, 2015, 10:49 p.m., James DeFelice wrote:
> > include/mesos/slave/container_logger.hpp, line 157
> > 
> >
> > It may be useful here to include the executor labels here so that, for 
> > example, frameworks could advertise additional information to logging 
> > modules. for example, things like additional log files (LOGFILEx=...), or a 
> > logs dir (LOGSDIRx=xyz) to monitor.
> > 
> > It costs very little to include the executor labels here in the API and 
> > opens the door to more powerful modules down the road w/o needing to change 
> > the API to pass in additional context.
> 
> Joseph Wu wrote:
> (Followed up in the design doc:)
> 
> https://docs.google.com/document/d/1Y8Sh_1eya0xI-djPmBZvqB-IVp1aaYSl-HCI6-2u5w4/

I'll change `ExecutorID` to `ExecutorInfo`.


> On Dec. 12, 2015, 10:49 p.m., James DeFelice wrote:
> > include/mesos/slave/container_logger.hpp, line 133
> > 
> >
> > I'm probably missing something here but why all the "executor or task" 
> > and "executor/task" references? Mesos doesn't spawn processes for tasks, it 
> > spawns processes for executors and then executors execute tasks (using 
> > threads, child procs, containers, whatever). Which means that this module 
> > is invoked for executors only -- not tasks, correct? The current docs read 
> > in a way that's misleading.
> 
> Joseph Wu wrote:
> This logging module would would be executor-only for custom executors 
> (the original name of the class was "ExecutorLogger").  However, two of the 
> executors (and their tasks) provided by Mesos will be affected by the logging 
> module:
> 
> * The command executor => spawns as an executor (process), but acts like 
> a task.
> * `mesos-docker-executor` => spawns as a process (or in a Docker 
> container).  Mesos launches command tasks (Docker containers) using this, and 
> the resulting stdout/stderr goes into the sandbox currently.
> 
> The only logging Mesos does not necessarily control is tasks launched by 
> custom executors.

Tweaked some comments to make the distinction.  i.e. Some tasks are involved, 
but only the ones that Mesos (not custom executor) launches.


- Joseph


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


On Dec. 14, 2015, 3:46 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> ---
> 
> (Updated Dec. 14, 2015, 3:46 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
> 
>

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Timothy Chen]

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



include/mesos/slave/container_logger.hpp (line 120)


What `captureOutput` are you referring to here?


- Timothy Chen


On Dec. 14, 2015, 11:46 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> ---
> 
> (Updated Dec. 14, 2015, 11:46 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
> 
>

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Timothy Chen]

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



include/mesos/slave/container_logger.hpp (line 111)


Extra space in the comments


- Timothy Chen


On Dec. 14, 2015, 11:46 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> ---
> 
> (Updated Dec. 14, 2015, 11:46 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
> 
>

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

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

(Updated Dec. 11, 2015, 4:39 p.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, 
and Thomas Rampelberg.


Changes
---

Rename from ExecutorLogger to ContainerLogger.  Removed `getLogURL` for now, 
because the existing code does not exercise it.


Summary (updated)
-

Logger Module: Introduce the ContainerLogger interface for logging the 
stdout/stderr of executors and tasks.


Bugs: MESOS-4087
https://issues.apache.org/jira/browse/MESOS-4087


Repository: mesos


Description (updated)
---

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 (updated)
-

  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