Hi guys, Thanks for the feedback. I will create a new review .
Alexandra On 28 April 2014 22:19, Ben Mahler <benjamin.mah...@gmail.com> wrote: > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19357/ > > On March 25th, 2014, 12:23 a.m. UTC, *Ben Mahler* wrote: > > > src/master/master.cpp<https://reviews.apache.org/r/19357/diff/3/?file=533904#file533904line514> > (Diff > revision 3) > > void Master::initialize() > > 514 > > if (flags.log_dir.isSome()) { > > 514 > > // No need to access FATAL log file; if the program > > 515 > > Try<string> log = logging::getLogFile(google::INFO); > > 515 > > // is still running, there definitely haven't been any > > 516 > > // FATAL logs yet; a FATAL log will cause the program to crash. > > 517 > > google::LogSeverity minloglevel = > logging::getMinLogLevel(flags.minloglevel); > > 518 > > if (flags.log_dir.isSome() && minloglevel != google::FATAL) { > > 519 > > Try<string> log = logging::getLogFile(minloglevel); > > Would it not be simpler to disallow FATAL-only logging for now so that we > don't have to do this special casing in the Master / Slave / webui? > > Or, if we want to keep FATAL-only logging as an option, we could update the > "pailer" component of the webui show the underlying error from the backend > instead of "FAILED TO INITIALIZE...RETRYING'", thoughts? > > On March 25th, 2014, 8:04 a.m. UTC, *Alexandra Sava* wrote: > > When a FATAL log is been issued, the program exits(this is how glog works) so > you won't be able to access the log file from the web gui. I already updated > the webgui for the case when minloglevel is FATAL (in this patch), so you > will not see "FAILED TO INITIALIZE...RETRYING". > > On March 25th, 2014, 6:31 p.m. UTC, *Ben Mahler* wrote: > > I understand the semantics of FATAL logging and the log file creation, I'm > just not convinced we should add the special case logic in the Master, Slave, > webui, just to support FATAL-only logging. So, I'm wondering why we don't > just support INFO,WARNING,ERROR to keep things simple. If it turns out that > people need FATAL we can add it. > > On April 28th, 2014, 7:16 a.m. UTC, *Alexandra Sava* wrote: > > If FATAL logs are used int the code, why not having the possibility of > configuring the log level as FATAL? I don't see where it gets complicated if > we have a few lines more for FATAL logs. > > We had to change the master, slave, logging library, and the webui to handle > the FATAL case. In general, special cases are unfortunate because it means an > "exceptional" case that we need to: > > 1. Remember in our mental model of the system. > 2. Add comments to inform the reader of the code. > 3. Handle correctly in all the appropriate places (opening room for more > bugs). > > This is why I feel supporting FATAL introduces too much complexity for the > use-case. Does this make sense? > > As vinod said, following up in a separate review would be great! > > > - Ben > > On April 28th, 2014, 7:14 a.m. UTC, Alexandra Sava wrote: > Review request for mesos and Vinod Kone. > By Alexandra Sava. > > *Updated April 28, 2014, 7:14 a.m.* > *Bugs: * MESOS-1067 <https://issues.apache.org/jira/browse/MESOS-1067> > *Repository: * mesos-git > Description > > minloglevel flag is passed as an argument to the command line and it > configures the level of logging to stderr and file. It can take INFO, > WARNING, ERROR or FATAL values. All log messages > at or above the configured log level will be printed. > > If 'quiet' flag is also passed to the command line, minloglevel will > affect just the logs from file (if configured by log_dir flag) > > If a log directory is present, the log file that will be available for > viewing in the browser > will be the one for the minimum log level configured (with or without the > presence of minloglevel flag) > If FATAL is the minimum log level, there is an exception: no log file will > be available in the browser. This is because FATAL logs will cause the > program to crash. > So if at any point the user can still access mesos page in the browser, it > means no > FATAL logs have been issued. > > Testing > > 1.Test1 - run master and slave with log_dir flag defined and minloglevel flag > set to WARNING > - expected results: - logs at and above WARNING level are logged into > log_dir and stderr > - the log file for WARNING logs is accessible in > the browser > > 2.Test2 - run master and slave with log_dir flag defined and minloglevel flag > set to FATAL > - expected results: - logs at FATAL level are logged into log_dir and > stderr > - no log file is accessible in the browser > because as soon as a FATAL log will be issued, the program will crash > > 3.Test3 - run master and slave with log_dir flag defined, minloglevel flag > set to INFO and quiet flag defined > - expected results: - logs at and above INFO level are logged into > log_dir and no logs (just FATAL) are displayed at stderr > - the log file for INFO logs is accessible in the > browser > > 4.Test4 - run master and slave with minloglevel flag set to INFO and quiet > flag defined > - expected results: - no logs (just FATAL) are displayed at stderr > (quiet flag takes precedence) > - no log file is accessible in the browser > > 5.Test5 - run master and slave with log_dir and quiet flags defined > - expected results: - no logs (just FATAL) are displayed at stderr > - the log file for INFO logs is accessible in the > browse > > Diffs > > - src/logging/flags.hpp (457feeeb7ca7ececf3c4e69189800ecb370053e6) > - src/logging/logging.hpp (39c2934f733e5c058f62b5497f31f7a7057bb4c7) > - src/logging/logging.cpp (176e49a6a2aef13be44ff910144c7321c942345a) > - src/master/master.cpp (f205dca43f10697862e3fd3f435f1127a9d0aecb) > - src/slave/slave.cpp (cb80609ba421b3b9a4664e600f0e53ecab8574c4) > - src/webui/master/static/js/controllers.js > (4b8487e0c285f892ad352993c81637f38df1429f) > > View Diff <https://reviews.apache.org/r/19357/diff/> >
