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