mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joseph Wu" <jos...@mesosphere.io>
Subject Re: Review Request 41783: Logger Module: Implement the rotating container logger module.
Date Tue, 05 Jan 2016 02:22:00 GMT


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


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