Re: Review Request 41783: Logger Module: Implement the rotating container logger module.

[Joseph Wu]

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

[Joseph Wu]

> On Dec. 29, 2015, 4:38 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/rotate.cpp, line 99
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
-

Re: Review Request 41783: Logger Module: Implement the rotating container logger module.

[Mesos ReviewBot]

---
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, 4, 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.

[Joseph Wu]

> On Jan. 4, 2016, 10:30 p.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [41002, 41003, 41004, 41061, 4, 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.

[Joseph Wu]

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

[Mesos ReviewBot]

---
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, 4, 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.

[Joseph Wu]

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

[Mesos ReviewBot]

---
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, 4, 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.

[Benjamin Hindman]

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


{ on newline please.



src/slave/container_loggers/rotate.hpp (lines 46 - 50)


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)


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)


These pulled out?



src/slave/container_loggers/rotate.cpp (lines 57 - 61)


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)


Want to do this validation in the flags themselves?



src/slave/container_loggers/rotating.hpp (line 49)


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.

[Joseph Wu]

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

[Joseph Wu]

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

[Benjamin Hindman]

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


{ on newline please.



src/slave/container_loggers/rotating.cpp (lines 148 - 151)


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: Logger Module: Implement the rotating container logger module.

[Joseph Wu]

> On Jan. 13, 2016, 9:03 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/rotating.cpp, lines 148-151
> > 
> >
> > 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.

[Joseph Wu]

---
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: Logger Module: Implement the rotating container logger module.

[Joseph Wu]

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

[Joseph Wu]

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

[Shuai Lin]

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


nitpick: "--version 2>/dev/null"



src/slave/container_loggers/logrotate.cpp (line 55)


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.

[Benjamin Hindman]

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


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

[Timothy Chen]

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


Are you expecting termination sent as a empty pipe from stdin here?



src/slave/container_loggers/rotate.cpp (line 125)


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

Re: Review Request 41783: Logger Module: Implement the rotating container logger module.

[Joseph Wu]

> On Dec. 29, 2015, 2:34 p.m., Timothy Chen wrote:
> > src/slave/container_loggers/rotate.cpp, line 115
> > 
> >
> > 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
> > 
> >
> > 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.

[Mesos ReviewBot]

---
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, 4, 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.

[Benjamin Hindman]

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


virtual



src/slave/container_loggers/rotate.cpp (line 99)


Why abstract this?



src/slave/container_loggers/rotate.cpp (lines 114 - 116)


This needs to be commented!



src/slave/container_loggers/rotate.cpp (line 121)


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)


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)


What if this fails?



src/slave/container_loggers/rotate.cpp (lines 174 - 179)


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)


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)


No indent here.



src/slave/container_loggers/rotating.cpp (line 252)


= None();



src/slave/container_loggers/rotating.cpp (line 268)


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)


This line (and subsequent lines below) should not be indented.



src/slave/container_loggers/rotating.cpp (line 280)


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