> On March 25, 2014, 12:23 a.m., Ben Mahler wrote: > > src/logging/flags.hpp, line 42 > > <https://reviews.apache.org/r/19357/diff/3/?file=533901#file533901line42> > > > > This flag name is inconsistent with our other flag names which use > > snake case. It would be nice if we could call this "logging_level" :)
The only one that has sneak case is 'log_dir'. For example, there is 'logbufsecs' as well that does not have it. Anyway, for me is not a problem changing this but the code has already been commited (commit 7c366bad61e8c241797ef5c92562e0539c0dd02a). I'm not sure how we should proceed in this case: reopen the jira ticket(which has been marked as resolved) or create another one? > 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"; LOG(severity) does not work. You get a compilation error ( error: 'COMPACT_GOOGLE_LOG_severity' was not declared in this scope). > 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.. 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. > 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? 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 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"? 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? - Alexandra ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19357/#review38382 ----------------------------------------------------------- On March 24, 2014, 12:51 p.m., Alexandra Sava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19357/ > ----------------------------------------------------------- > > (Updated March 24, 2014, 12:51 p.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 e2e4afccc4d8b8cc269960f90d59aab9c1e53807 > src/logging/logging.hpp 3c24211cb711a43d6f23950607f5a30e1351dd7d > src/logging/logging.cpp a46a3f133830297cbc466e761e1682fb49df09df > src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437 > src/slave/slave.cpp d8d3e0fa54972201d72b2650ec0ba922a4912d54 > src/webui/master/static/js/controllers.js > afb24fb9c2184772f7314162f5637dbabaa2ab94 > > 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 > >
