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