----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19357/#review38382 -----------------------------------------------------------
src/logging/flags.hpp <https://reviews.apache.org/r/19357/#comment70472> 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" :) src/logging/flags.hpp <https://reviews.apache.org/r/19357/#comment70475> 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"? src/logging/logging.cpp <https://reviews.apache.org/r/19357/#comment70522> I would prefer to see validation of the flag to ensure these cases are not possible, that way operators know they've made a mistake! :) src/logging/logging.cpp <https://reviews.apache.org/r/19357/#comment70521> 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"; src/logging/logging.cpp <https://reviews.apache.org/r/19357/#comment70519> 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.. src/master/master.cpp <https://reviews.apache.org/r/19357/#comment70523> 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? - Ben Mahler 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 > >
