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