> On March 25, 2014, 12:23 a.m., Ben Mahler wrote: > > src/logging/logging.cpp, lines 100-114 > > <https://reviews.apache.org/r/19357/diff/3/?file=533903#file533903line100> > > > > Can you add a comment that this is because GLOG only lazily creates the > > log file once the first log message occurs? > > > > Also, seems like this code could be cleaned up to be just: > > > > LOG(severity) << "Logging " << google::getLogSeverity(severity) << " > > level started"; > > Alexandra Sava wrote: > LOG(severity) does not work. You get a compilation error ( error: > 'COMPACT_GOOGLE_LOG_severity' was not declared in this scope). > > Ben Mahler wrote: > Can you use LOG_AT_LEVEL?
Yes, LOG_AT_LEVEL worked. > On March 25, 2014, 12:23 a.m., Ben Mahler wrote: > > src/master/master.cpp, lines 514-519 > > <https://reviews.apache.org/r/19357/diff/3/?file=533904#file533904line514> > > > > 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? > > 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". > > 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. 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. > On March 25, 2014, 12:23 a.m., Ben Mahler wrote: > > src/logging/flags.hpp, lines 43-45 > > <https://reviews.apache.org/r/19357/diff/3/?file=533901#file533901line43> > > > > Please quote the other flags, e.g. 'quiet' and 'log_dir'. Also, can we > > tell them what the valid values for this "logging_level" are? How are they > > to know that they can specify "INFO", "WARNING", "ERROR", "FATAL"? > > Alexandra Sava wrote: > I think users will be able to see what flags/flags values Mesos supports > through documentation. Ho were they able to find out which flags/flags values > Mesos supports up until now? > > Ben Mahler wrote: > This is the part of the help message that gets printed when a user enters: > > $ mesos-master --help > $ mesos-slave --help > etc > > So this is documentation, and we should guide users as to what flag > values are acceptable :) I updated the documentation with the values it can have. > On March 25, 2014, 12:23 a.m., Ben Mahler wrote: > > src/logging/logging.cpp, lines 118-136 > > <https://reviews.apache.org/r/19357/diff/3/?file=533903#file533903line118> > > > > This looks more like a "getLogSeverity" function to convert from > > logging level string to 'LogSeverity' type? > > > > I'm curious why you decided to proceed with INFO if the flag has an > > unexpected value? Seems like an operator would want an EXIT(1) in this case > > to be aware of the mistake in flags.. > > Alexandra Sava wrote: > I decided to proceed with INFO because up until now the default log level > was INFO. I didn't put an EXIT there because this is not the kind of flag > without whom the program could not work. > > Ben Mahler wrote: > Best practice is to inform the operator if they've made a mistake in > their configuration rather than to silently change it. > > If they pass FOOBAR, it would be nice to tell them 'FOOBAR is not a valid > logging level' as this avoids possible confusion on the side of the operator. - Alexandra ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19357/#review38382 ----------------------------------------------------------- On April 28, 2014, 7:14 a.m., Alexandra Sava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19357/ > ----------------------------------------------------------- > > (Updated April 28, 2014, 7:14 a.m.) > > > Review request for mesos and Vinod Kone. > > > 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. > > > 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 > > Diff: https://reviews.apache.org/r/19357/diff/ > > > 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 > > > Thanks, > > Alexandra Sava > >
