You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2015/12/29 23:18:15 UTC
Review Request 41783: Logger Module: Implement the rotating container
logger module.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Bugs: MESOS-4136
https://issues.apache.org/jira/browse/MESOS-4136
Repository: mesos
Description
-------
Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
Diffs
-----
src/slave/container_loggers/rotate.cpp PRE-CREATION
src/slave/container_loggers/rotating.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41783/diff/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 4, 2016, 10:30 p.m., Mesos ReviewBot wrote:
> > Bad patch!
> >
> > Reviews applied: [41002, 41003, 41004, 41061, 41111, 41166, 41167, 41169, 41560, 41294, 41370, 41378, 41779]
> >
> > Failed command: ./support/apply-review.sh -n -r 41779
> >
> > Error:
> > 2016-01-05 06:30:37 URL:https://reviews.apache.org/r/41779/diff/raw/ [3222/3222] -> "41779.patch" [1]
> > Total errors found: 0
> > Checking 2 files
> > Error: Commit message summary (the first line) must not exceed 72 characters.
Shortened a couple of summary messages (3 of them were over 72 characters :).
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review112778
-----------------------------------------------------------
On Jan. 4, 2016, 6:21 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2016, 6:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.hpp PRE-CREATION
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.hpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review112778
-----------------------------------------------------------
Bad patch!
Reviews applied: [41002, 41003, 41004, 41061, 41111, 41166, 41167, 41169, 41560, 41294, 41370, 41378, 41779]
Failed command: ./support/apply-review.sh -n -r 41779
Error:
2016-01-05 06:30:37 URL:https://reviews.apache.org/r/41779/diff/raw/ [3222/3222] -> "41779.patch" [1]
Total errors found: 0
Checking 2 files
Error: Commit message summary (the first line) must not exceed 72 characters.
- Mesos ReviewBot
On Jan. 5, 2016, 2:21 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 2:21 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.hpp PRE-CREATION
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.hpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review112971
-----------------------------------------------------------
Patch looks great!
Reviews applied: [41002, 41003, 41004, 41061, 41111, 41166, 41167, 41169, 41560, 41294, 41370, 41378, 41779, 41780, 41781, 41782, 41783]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 5, 2016, 7:05 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2016, 7:05 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.hpp PRE-CREATION
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.hpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review113565
-----------------------------------------------------------
Patch looks great!
Reviews applied: [41002, 41003, 41004, 41061, 41111, 41166, 41167, 41169, 41560, 41294, 41370, 41378, 41779, 41780, 41781, 41782, 41783]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 8, 2016, 8:06 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2016, 8:06 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.hpp PRE-CREATION
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.hpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 13, 2016, 9:03 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/rotating.cpp, lines 148-151
> > <https://reviews.apache.org/r/41783/diff/5/?file=1196238#file1196238line148>
> >
> > Why not do this in the constructor and then no need for the `if (process.get() != NULL) {` everywhere?
Do you mean to remove the not-initialized and double-initialize checks?
Note: The already committed Sandbox ContainerLogger also does this. As well as the oversubscription modules.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review114410
-----------------------------------------------------------
On Jan. 13, 2016, 6:12 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.hpp PRE-CREATION
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.hpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review114410
-----------------------------------------------------------
src/slave/container_loggers/rotating.hpp (line 103)
<https://reviews.apache.org/r/41783/#comment175248>
{ on newline please.
src/slave/container_loggers/rotating.cpp (lines 148 - 151)
<https://reviews.apache.org/r/41783/#comment175249>
Why not do this in the constructor and then no need for the `if (process.get() != NULL) {` everywhere?
- Benjamin Hindman
On Jan. 14, 2016, 2:12 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2016, 2:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.hpp PRE-CREATION
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.hpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Implement the rotating container logger
module.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review115318
-----------------------------------------------------------
src/slave/container_loggers/rotate.cpp (lines 128 - 131)
<https://reviews.apache.org/r/41783/#comment176288>
Future<Nothing> loop()
{
return io::read(STDIN_FILENO, buffer, length)
.then([&](size_t readSize) -> Future<Nothing> {
// ... comment here ...
if (readSize <= 0) {
EXIT(EXIT_SUCCESS);
}
Try<Nothing> result = write(buffer, size);
if (result.isError()) {
return Error();
}
return dispatch(self(), &RotateLoggerProcess::loop);
}
Try<Nothing> write(buffer, size)
{
if file is closed: if (leading.isNone()) {
// open the leading file
// deal with errors
}
if (need_to_rotate) {
rotate();
return write(buffer, size);
}
... do the actual write ...
return Nothing();
}
- Benjamin Hindman
On Jan. 16, 2016, 12:54 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2016, 12:54 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implement the rotating container logger module.
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.hpp PRE-CREATION
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.hpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review116041
-----------------------------------------------------------
Fix it, then Ship it!
I'll fix up the issue below and commit, thanks!
src/slave/container_loggers/logrotate.cpp (line 107)
<https://reviews.apache.org/r/41783/#comment177055>
{ on newline please.
- Benjamin Hindman
On Jan. 23, 2016, 9:07 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2016, 9:07 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/lib_logrotate.hpp PRE-CREATION
> src/slave/container_loggers/lib_logrotate.cpp PRE-CREATION
> src/slave/container_loggers/logrotate.hpp PRE-CREATION
> src/slave/container_loggers/logrotate.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Shuai Lin <li...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review115993
-----------------------------------------------------------
src/slave/container_loggers/lib_logrotate.hpp (line 106)
<https://reviews.apache.org/r/41783/#comment177031>
nitpick: "--version 2>/dev/null"
src/slave/container_loggers/logrotate.cpp (line 55)
<https://reviews.apache.org/r/41783/#comment177032>
nitpick: rename `length` to something like `buflenth`, which could be more intuitive.
- Shuai Lin
On Jan. 23, 2016, 9:07 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2016, 9:07 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/lib_logrotate.hpp PRE-CREATION
> src/slave/container_loggers/lib_logrotate.cpp PRE-CREATION
> src/slave/container_loggers/logrotate.hpp PRE-CREATION
> src/slave/container_loggers/logrotate.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/
-----------------------------------------------------------
(Updated Jan. 23, 2016, 1:07 a.m.)
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Changes
-------
Cleaned up some headers.
Refactored `logrotate.cpp` a little (BenH's suggestions).
Added quotation marks inside the `logrotate` command and configuration file.
Added a `setsid` call such that the logger subprocess will not exit if the agent terminates.
Bugs: MESOS-4136
https://issues.apache.org/jira/browse/MESOS-4136
Repository: mesos
Description
-------
Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
Diffs (updated)
-----
src/slave/container_loggers/lib_logrotate.hpp PRE-CREATION
src/slave/container_loggers/lib_logrotate.cpp PRE-CREATION
src/slave/container_loggers/logrotate.hpp PRE-CREATION
src/slave/container_loggers/logrotate.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41783/diff/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/
-----------------------------------------------------------
(Updated Jan. 22, 2016, 7:04 p.m.)
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Changes
-------
Fix bug with launching the subprocess with the agent's environment.
Change subprocess's looping to have a future chain of max length 2.
Bugs: MESOS-4136
https://issues.apache.org/jira/browse/MESOS-4136
Repository: mesos
Description
-------
Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
Diffs (updated)
-----
src/slave/container_loggers/lib_logrotate.hpp PRE-CREATION
src/slave/container_loggers/lib_logrotate.cpp PRE-CREATION
src/slave/container_loggers/logrotate.hpp PRE-CREATION
src/slave/container_loggers/logrotate.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41783/diff/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/
-----------------------------------------------------------
(Updated Jan. 19, 2016, 7:41 p.m.)
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Changes
-------
Rename from "rotate" to "logrotate".
Refactored rotate.cpp (now logrotate.cpp) a bit.
Summary (updated)
-----------------
Logger Module: Implement the rotating container logger module.
Bugs: MESOS-4136
https://issues.apache.org/jira/browse/MESOS-4136
Repository: mesos
Description (updated)
-------
Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
Diffs (updated)
-----
src/slave/container_loggers/lib_logrotate.hpp PRE-CREATION
src/slave/container_loggers/lib_logrotate.cpp PRE-CREATION
src/slave/container_loggers/logrotate.hpp PRE-CREATION
src/slave/container_loggers/logrotate.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41783/diff/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 41783: Implement the rotating container logger
module.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/
-----------------------------------------------------------
(Updated Jan. 15, 2016, 4:54 p.m.)
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Changes
-------
* Rework module implementation to use `logrotate` instead of custom logic.
* * Removed the "number of files" flag in favor of a general "logrotate options" flag.
* * Gouged out the logic in `rotate.cpp` and replaced it with an `os::shell`.
Bugs: MESOS-4136
https://issues.apache.org/jira/browse/MESOS-4136
Repository: mesos
Description
-------
Implement the rotating container logger module.
Diffs (updated)
-----
src/slave/container_loggers/rotate.hpp PRE-CREATION
src/slave/container_loggers/rotate.cpp PRE-CREATION
src/slave/container_loggers/rotating.hpp PRE-CREATION
src/slave/container_loggers/rotating.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41783/diff/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 41783: Implement the rotating container logger
module.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/
-----------------------------------------------------------
(Updated Jan. 15, 2016, 11:34 a.m.)
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Changes
-------
Refactored `initialize` logic since the module is so simple (compared to other modules, like oversubscription).
Bugs: MESOS-4136
https://issues.apache.org/jira/browse/MESOS-4136
Repository: mesos
Description
-------
Implement the rotating container logger module.
Diffs (updated)
-----
src/slave/container_loggers/rotate.hpp PRE-CREATION
src/slave/container_loggers/rotate.cpp PRE-CREATION
src/slave/container_loggers/rotating.hpp PRE-CREATION
src/slave/container_loggers/rotating.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41783/diff/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 41783: Implement the rotating container logger
module.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/
-----------------------------------------------------------
(Updated Jan. 14, 2016, 11:49 a.m.)
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Changes
-------
Tweak some spacing.
Summary (updated)
-----------------
Implement the rotating container logger module.
Bugs: MESOS-4136
https://issues.apache.org/jira/browse/MESOS-4136
Repository: mesos
Description (updated)
-------
Implement the rotating container logger module.
Diffs (updated)
-----
src/slave/container_loggers/rotate.hpp PRE-CREATION
src/slave/container_loggers/rotate.cpp PRE-CREATION
src/slave/container_loggers/rotating.hpp PRE-CREATION
src/slave/container_loggers/rotating.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41783/diff/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/
-----------------------------------------------------------
(Updated Jan. 13, 2016, 6:12 p.m.)
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Changes
-------
* Reworked some flag comments. Moved validation code into flags.
* Removed `logger_log_filename` flag in favor of printing to the agent's stderr upon (critical) failure.
* Changed types of `max_..._size` flags to `Bytes`.
* Renamed `outgoing` and `head` language to `leading`.
Bugs: MESOS-4136
https://issues.apache.org/jira/browse/MESOS-4136
Repository: mesos
Description
-------
Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
Diffs (updated)
-----
src/slave/container_loggers/rotate.hpp PRE-CREATION
src/slave/container_loggers/rotate.cpp PRE-CREATION
src/slave/container_loggers/rotating.hpp PRE-CREATION
src/slave/container_loggers/rotating.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41783/diff/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 13, 2016, 11:25 a.m., Benjamin Hindman wrote:
> >
Note: I'm still gathering feedback regarding the naming scheme of the rotated logs.
> On Jan. 13, 2016, 11:25 a.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/rotate.hpp, line 54
> > <https://reviews.apache.org/r/41783/diff/3/?file=1182155#file1182155line54>
> >
> > How about s/of log/of rotated log/ here?
> >
> > Also, is 'head' better than 'current'? Just a suggestion, given that head sounds like "first", I thought by head you meant the oldest log file, not the newest.
"Current" doesn't fit very well in some comments, so I renamed to "Leading" instead. (And renamed some variables.)
> On Jan. 13, 2016, 11:25 a.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/rotate.cpp, lines 35-37
> > <https://reviews.apache.org/r/41783/diff/3/?file=1182156#file1182156line35>
> >
> > These pulled out?
Yup, done in diff#4.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review113378
-----------------------------------------------------------
On Jan. 13, 2016, 6:12 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.hpp PRE-CREATION
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.hpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review113378
-----------------------------------------------------------
src/slave/container_loggers/rotate.hpp (line 36)
<https://reviews.apache.org/r/41783/#comment173976>
{ on newline please.
src/slave/container_loggers/rotate.hpp (lines 46 - 50)
<https://reviews.apache.org/r/41783/#comment173977>
This should be `Bytes` rather than force the reader/user to write this in bytes (even if it's code that is generating this) and force you to use it as a `size_t`.
Same for other flags you have here and below too please.
src/slave/container_loggers/rotate.hpp (line 54)
<https://reviews.apache.org/r/41783/#comment173979>
How about s/of log/of rotated log/ here?
Also, is 'head' better than 'current'? Just a suggestion, given that head sounds like "first", I thought by head you meant the oldest log file, not the newest.
src/slave/container_loggers/rotate.cpp (lines 35 - 37)
<https://reviews.apache.org/r/41783/#comment174225>
These pulled out?
src/slave/container_loggers/rotate.cpp (lines 57 - 61)
<https://reviews.apache.org/r/41783/#comment174226>
Can you send me another review which updates the version of `io::read` that you're using to `dup`, `os::cloexec`, and `os::nonblock` the file descriptor so we don't have to? Then we can kill this (and other places that use that version of `io::read` too!).
src/slave/container_loggers/rotate.cpp (lines 207 - 218)
<https://reviews.apache.org/r/41783/#comment175067>
Want to do this validation in the flags themselves?
src/slave/container_loggers/rotating.hpp (line 49)
<https://reviews.apache.org/r/41783/#comment173975>
How about:
s/of stdout/of rotated stdout (i.e., stdout.1, stdout.2, ...)/
- Benjamin Hindman
On Jan. 8, 2016, 8:06 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2016, 8:06 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.hpp PRE-CREATION
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.hpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/
-----------------------------------------------------------
(Updated Jan. 8, 2016, 12:06 p.m.)
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Changes
-------
Fix module cleanup on failure in `prepare`. Fix header order, some spacing.
Bugs: MESOS-4136
https://issues.apache.org/jira/browse/MESOS-4136
Repository: mesos
Description
-------
Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
Diffs (updated)
-----
src/slave/container_loggers/rotate.hpp PRE-CREATION
src/slave/container_loggers/rotate.cpp PRE-CREATION
src/slave/container_loggers/rotating.hpp PRE-CREATION
src/slave/container_loggers/rotating.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41783/diff/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/
-----------------------------------------------------------
(Updated Jan. 5, 2016, 11:05 a.m.)
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Changes
-------
Fix some signed comparisons.
Bugs: MESOS-4136
https://issues.apache.org/jira/browse/MESOS-4136
Repository: mesos
Description
-------
Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
Diffs (updated)
-----
src/slave/container_loggers/rotate.hpp PRE-CREATION
src/slave/container_loggers/rotate.cpp PRE-CREATION
src/slave/container_loggers/rotating.hpp PRE-CREATION
src/slave/container_loggers/rotating.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41783/diff/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/
-----------------------------------------------------------
(Updated Jan. 4, 2016, 6:21 p.m.)
Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
Changes
-------
Use stout::Flags. Added lots more flags + comments. A bunch of cleanup + addressing comments.
Bugs: MESOS-4136
https://issues.apache.org/jira/browse/MESOS-4136
Repository: mesos
Description
-------
Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
Diffs (updated)
-----
src/slave/container_loggers/rotate.hpp PRE-CREATION
src/slave/container_loggers/rotate.cpp PRE-CREATION
src/slave/container_loggers/rotating.hpp PRE-CREATION
src/slave/container_loggers/rotating.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41783/diff/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review112253
-----------------------------------------------------------
Patch looks great!
Reviews applied: [41002, 41003, 41004, 41061, 41111, 41166, 41167, 41169, 41560, 41294, 41370, 41378, 41779, 41780, 41781, 41782, 41783]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 29, 2015, 10:18 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Dec. 29, 2015, 10:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 29, 2015, 4:38 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/rotate.cpp, line 99
> > <https://reviews.apache.org/r/41783/diff/1/?file=1177605#file1177605line99>
> >
> > Why abstract this?
At some point, this function was a bit longer than one statement. Cleaned up...
> On Dec. 29, 2015, 4:38 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/rotate.cpp, line 121
> > <https://reviews.apache.org/r/41783/diff/1/?file=1177605#file1177605line121>
> >
> > Why not just:
> >
> > if ((bytesWritten + readSize) > maxSize) {
> >
> > }
> >
> > Instead of asking the reader to understand that you're calculating the number of bytes that would overflow maxSize and then checking if that's greater than 0?
Oops, this was also a leftover.
At some earlier iteration, I considered writing the files exactly up to the configured log-size limit rather than by whatever `io::read` returns. (You'll notice that the test checks `2040 < log-file-size < 2048`.) This led to less-readable log files, particularly if a sentence is broken into two files and then one of those files is deleted.
> On Dec. 29, 2015, 4:38 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/rotate.cpp, lines 125-131
> > <https://reviews.apache.org/r/41783/diff/1/?file=1177605#file1177605line125>
> >
> > Okay, IIUC then you'll have a moving window of log files, rather than rotating through log files .1, .2, .3, .4, .maxFiles, is that right?
> >
> > Any reason not to do the latter?
> >
> > Either way, this needs to be documented! Preferably at the begining of this class, with a basic comment here that reminds folks. This was not what I expected so I had to read the code carefuly.
We don't cycle through a finite set of files because this makes it easier to order the files.
i.e. Suppose maxFiles is 5.
1) The logger (over time) creates the files: `log`, `log.1`, `log.2`, `log.3`, `log.4`.
2) `log` fills up and `log.1` is deleted.
Current impl) `log` is renamed to `log.5`.
Cycle impl) `log` is renamed to `log.1`. Someone reads `log.1` then `log.2` and gets confused.
I'll put the related documentation into the custom flags (`--log_filename`).
> On Dec. 29, 2015, 4:38 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/rotate.cpp, lines 174-179
> > <https://reviews.apache.org/r/41783/diff/1/?file=1177605#file1177605line174>
> >
> > Woah! Why not use stout's `Flags`!!!??? We do this with our other executables and it makes the code much simpler!
Added :)
(And some other flags too.)
> On Dec. 29, 2015, 4:38 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/rotating.cpp, line 162
> > <https://reviews.apache.org/r/41783/diff/1/?file=1177606#file1177606line162>
> >
> > Why are we not using `Path`? Do we need to do more in the codebase to make us use `Path` everywhere?
We don't use `Path` mostly because the `path::` helpers return strings.
To use `Path` as it is, we'd need to do `Path(path::join(...))`.
For now, I'll drop this (and we can track the cleanup/refactor in MESOS-2995).
> On Dec. 29, 2015, 4:38 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/rotate.cpp, line 146
> > <https://reviews.apache.org/r/41783/diff/1/?file=1177605#file1177605line146>
> >
> > What if this fails?
Added a comment. And one for `os::rm` and `os::rename`.
> On Dec. 29, 2015, 4:38 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/rotating.cpp, line 268
> > <https://reviews.apache.org/r/41783/diff/1/?file=1177606#file1177606line268>
> >
> > Could we create a "flags" for these parameters? And parse the parameters as flags? That would be very clean!!! We could then retroactively clean up the other modules, it would set a clean precedent.
> >
> > It's especially wierd in this circumstance to pass `Result` arguments into the logger and then later during initialize error out. It means that everywehre else in the logger you "know" that it's okay to just call `.get()` on the `Result` objects because you've already checked them, which is a nasty global invariant that people now have to remember! In your case you've dodged this bullet by having `RotatingContainerLogger::initialize` do the checks and error out there, but then what if you need to have `RotatingContainerLoggerProcess` do it's own initialization?
> >
> > I'd rather see the `create` function passed to the `Module` return an `Error` ... which apparently is not allowed because we didn't pull in `stout` for modules? But we did pull in `stout` for the `ContainerLogger` module ... ???
Added flags. Also added a few extra parameters.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review112260
-----------------------------------------------------------
On Jan. 4, 2016, 6:21 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2016, 6:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.hpp PRE-CREATION
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.hpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review112260
-----------------------------------------------------------
src/slave/container_loggers/rotate.cpp (line 89)
<https://reviews.apache.org/r/41783/#comment172660>
virtual
src/slave/container_loggers/rotate.cpp (line 99)
<https://reviews.apache.org/r/41783/#comment172661>
Why abstract this?
src/slave/container_loggers/rotate.cpp (lines 114 - 116)
<https://reviews.apache.org/r/41783/#comment172662>
This needs to be commented!
src/slave/container_loggers/rotate.cpp (line 121)
<https://reviews.apache.org/r/41783/#comment172664>
Why not just:
if ((bytesWritten + readSize) > maxSize) {
}
Instead of asking the reader to understand that you're calculating the number of bytes that would overflow maxSize and then checking if that's greater than 0?
src/slave/container_loggers/rotate.cpp (lines 125 - 131)
<https://reviews.apache.org/r/41783/#comment172666>
Okay, IIUC then you'll have a moving window of log files, rather than rotating through log files .1, .2, .3, .4, .maxFiles, is that right?
Any reason not to do the latter?
Either way, this needs to be documented! Preferably at the begining of this class, with a basic comment here that reminds folks. This was not what I expected so I had to read the code carefuly.
src/slave/container_loggers/rotate.cpp (line 146)
<https://reviews.apache.org/r/41783/#comment172663>
What if this fails?
src/slave/container_loggers/rotate.cpp (lines 174 - 179)
<https://reviews.apache.org/r/41783/#comment172659>
Woah! Why not use stout's `Flags`!!!??? We do this with our other executables and it makes the code much simpler!
src/slave/container_loggers/rotating.cpp (line 162)
<https://reviews.apache.org/r/41783/#comment172658>
Why are we not using `Path`? Do we need to do more in the codebase to make us use `Path` everywhere?
src/slave/container_loggers/rotating.cpp (line 217)
<https://reviews.apache.org/r/41783/#comment172656>
No indent here.
src/slave/container_loggers/rotating.cpp (line 252)
<https://reviews.apache.org/r/41783/#comment172643>
= None();
src/slave/container_loggers/rotating.cpp (line 268)
<https://reviews.apache.org/r/41783/#comment172645>
Could we create a "flags" for these parameters? And parse the parameters as flags? That would be very clean!!! We could then retroactively clean up the other modules, it would set a clean precedent.
It's especially wierd in this circumstance to pass `Result` arguments into the logger and then later during initialize error out. It means that everywehre else in the logger you "know" that it's okay to just call `.get()` on the `Result` objects because you've already checked them, which is a nasty global invariant that people now have to remember! In your case you've dodged this bullet by having `RotatingContainerLogger::initialize` do the checks and error out there, but then what if you need to have `RotatingContainerLoggerProcess` do it's own initialization?
I'd rather see the `create` function passed to the `Module` return an `Error` ... which apparently is not allowed because we didn't pull in `stout` for modules? But we did pull in `stout` for the `ContainerLogger` module ... ???
src/slave/container_loggers/rotating.cpp (line 273)
<https://reviews.apache.org/r/41783/#comment172641>
This line (and subsequent lines below) should not be indented.
src/slave/container_loggers/rotating.cpp (line 280)
<https://reviews.apache.org/r/41783/#comment172640>
Let's just embed the lambda here please!
- Benjamin Hindman
On Dec. 29, 2015, 10:18 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Dec. 29, 2015, 10:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 29, 2015, 2:34 p.m., Timothy Chen wrote:
> > src/slave/container_loggers/rotate.cpp, line 115
> > <https://reviews.apache.org/r/41783/diff/1/?file=1177605#file1177605line115>
> >
> > Are you expecting termination sent as a empty pipe from stdin here?
Yeah, closing the input pipe is the our current solution for shutting down the logger process along with the task/executor.
Other solutions would be messier. For the Mesos Containerizer, we would need to track all these extra processes along with the task/executor. For the Docker Containerizer, we would need to `docker exec` the subprocess (or spawn them in separate containers and track them there).
> On Dec. 29, 2015, 2:34 p.m., Timothy Chen wrote:
> > src/slave/container_loggers/rotate.cpp, line 125
> > <https://reviews.apache.org/r/41783/diff/1/?file=1177605#file1177605line125>
> >
> > I think maxFiles shouldn't be zero as well right? We will then not remove the oldest log file.
Right. There are actually a few places we can check these parameters; either inside the logger executable, the logger module, or both.
We presumably want at least 1 log file. And each log file should be at least some multiple of the page size (`sysconf(_SC_PAGE_SIZE)`).
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review112231
-----------------------------------------------------------
On Dec. 29, 2015, 2:18 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Dec. 29, 2015, 2:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41783: Logger Module: Implement the rotating
container logger module.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41783/#review112231
-----------------------------------------------------------
src/slave/container_loggers/rotate.cpp (line 115)
<https://reviews.apache.org/r/41783/#comment172588>
Are you expecting termination sent as a empty pipe from stdin here?
src/slave/container_loggers/rotate.cpp (line 125)
<https://reviews.apache.org/r/41783/#comment172589>
I think maxFiles shouldn't be zero as well right? We will then not remove the oldest log file.
- Timothy Chen
On Dec. 29, 2015, 10:18 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41783/
> -----------------------------------------------------------
>
> (Updated Dec. 29, 2015, 10:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-4136
> https://issues.apache.org/jira/browse/MESOS-4136
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds a non-default ContainerLogger that constrains total log size by rotating logs (i.e. renaming the head log file).
>
>
> Diffs
> -----
>
> src/slave/container_loggers/rotate.cpp PRE-CREATION
> src/slave/container_loggers/rotating.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41783/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>