mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 41783: Logger Module: Implement the rotating container logger module.
Date Wed, 30 Dec 2015 00:38:13 GMT

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


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