This is an automatically generated e-mail. To reply, visit:

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


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

Reply via email to