From reviews-return-18955-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Tue Jan 5 02:22:02 2016 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 956FA185EE for ; Tue, 5 Jan 2016 02:22:02 +0000 (UTC) Received: (qmail 60328 invoked by uid 500); 5 Jan 2016 02:22:02 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 60306 invoked by uid 500); 5 Jan 2016 02:22:02 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 60286 invoked by uid 99); 5 Jan 2016 02:22:02 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 05 Jan 2016 02:22:02 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id D3178297BD9; Tue, 5 Jan 2016 02:22:00 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============2472780926442097338==" MIME-Version: 1.0 Subject: Re: Review Request 41783: Logger Module: Implement the rotating container logger module. From: "Joseph Wu" To: "Artem Harutyunyan" , "Benjamin Hindman" Cc: "Joseph Wu" , "mesos" Date: Tue, 05 Jan 2016 02:22:00 -0000 Message-ID: <20160105022200.26043.97342@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: "Joseph Wu" X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/41783/ X-Sender: "Joseph Wu" References: <20151230003813.4181.3255@reviews.apache.org> In-Reply-To: <20151230003813.4181.3255@reviews.apache.org> Reply-To: "Joseph Wu" X-ReviewRequest-Repository: mesos --===============2472780926442097338== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > 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 > > --===============2472780926442097338==--