----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19357/#review37632 -----------------------------------------------------------
src/logging/flags.hpp <https://reviews.apache.org/r/19357/#comment69246> +1 to Dominic's comment. Also make "INFO" the default. src/logging/logging.hpp <https://reviews.apache.org/r/19357/#comment69248> New line. We separate outer elements by 2 lines. src/logging/logging.cpp <https://reviews.apache.org/r/19357/#comment69256> We use braces around if blocks, even when they contain one statement. src/logging/logging.cpp <https://reviews.apache.org/r/19357/#comment69258> How about: string message = "Logging level " + string(google::GetLogSeverityName(severity)) + " started"; src/logging/logging.cpp <https://reviews.apache.org/r/19357/#comment69257> End with period. s/run/run./ src/logging/logging.cpp <https://reviews.apache.org/r/19357/#comment69259> This means we will crash if someone calls this function with "FATAL". Doesn't seem to be nice. Is it possible to just create an empty file for a log level? src/logging/logging.cpp <https://reviews.apache.org/r/19357/#comment69262> Lets do the validation for 'minloglevel' once. src/logging/logging.cpp <https://reviews.apache.org/r/19357/#comment69260> s/above/above./ src/logging/logging.cpp <https://reviews.apache.org/r/19357/#comment69264> This is a bit hard to follow. Why not just: FLAGS_minlogevel = flags.minloglevel at line #148. src/master/master.cpp <https://reviews.apache.org/r/19357/#comment69255> End comment with a period. s/crash/crash./ src/master/master.cpp <https://reviews.apache.org/r/19357/#comment69254> End comment with a period. s/point/point./ src/slave/slave.cpp <https://reviews.apache.org/r/19357/#comment69253> End comment with a period. s/crash/crash./ src/slave/slave.cpp <https://reviews.apache.org/r/19357/#comment69266> ditto. see above. src/slave/slave.cpp <https://reviews.apache.org/r/19357/#comment69252> End comment with a period. s/point/point./ src/webui/master/static/js/controllers.js <https://reviews.apache.org/r/19357/#comment69250> Why did you need this special case? In other words, what is the behavior if you don't have this? src/webui/master/static/js/controllers.js <https://reviews.apache.org/r/19357/#comment69249> ditto. see above. - Vinod Kone On March 18, 2014, 5 p.m., Alexandra Sava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19357/ > ----------------------------------------------------------- > > (Updated March 18, 2014, 5 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 takes values from > 0 to 3 which traduces into INFO, WARNING, ERROR and FATAL. 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 1 > - 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 3 > - 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 0 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 0 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 > >
