Sorry for the delay, I've been bogged down quite a bit lately but I will take a look today!
Sent from my iPhone > On May 7, 2014, at 5:17 AM, Alexandra Sava <[email protected]> wrote: > > Hi guys, > > Any feedback for my changes would be welcomed. > > > > Thanks, > Alexandra > > >> On 1 May 2014 14:16, Alexandra Sava <[email protected]> wrote: >> >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/20850/ >> >> On April 29th, 2014, 7:18 p.m. UTC, Ben Mahler wrote: >> >> src/master/master.cpp (Diff revision 1) >> void Master::initialize() >> 524 >> // No need to access FATAL log file; if the program >> 524 >> google::LogSeverity logging_level = >> logging::getLogSeverity(flags.logging_level); >> 525 >> // is still running, there definitely haven't been any >> 525 >> if (flags.log_dir.isSome()) { >> 526 >> // FATAL logs yet; a FATAL log will cause the program to crash. >> 526 >> Try<string> log = logging::getLogFile(logging_level); >> What about having getLogFile take a logging::Flags object? Since it already >> returns a Try we could even return Error() instead of EXIT(1) when the level >> is invalid. >> >> If we did this change, then logging::initialize could EXIT(1) when the level >> is invalid. It seems a bit unfortunate to have getLogSeverity call EXIT(1) >> when the input is invalid since it is exposed in the header and can be >> called from anywhere! >> >> E.g.: >> >> initialize(...) >> { >> ... >> >> if (flags.logging_level != "INFO" && >> flags.logging_level != "WARNING" && >> flags.logging_level != "ERROR") { >> EXIT(1) << ...; >> } >> >> // We can hide getLogSeverity from the header and we can do an internal >> CHECK inside, instead of EXIT(1). >> LogSeverity severity = getLogSeverity(flags.logging_level); >> >> LOG_AT_LEVEL(severity) << "Logging level " + flags.logging_level + " >> initialized"; >> >> ... >> } >> I don't really see the purpose in changing the argument of getLogFile into >> logging::FLags type. getLogFile returns the log file associate to the given >> severity, so it makes sence to provide a LogSeverity type as an argument. >> Also, getLogFile already does what you said: it returns an Error when the >> level is invalid (it never returns an Exit). >> >> Yes I also think it is not ok to have an EXIT in getLogSeverity as long as >> it is exposed in the header file. I changed that part as you suggested. >> >> - Alexandra >> >> >> On May 1st, 2014, 11:09 a.m. UTC, Alexandra Sava wrote: >> >> Review request for mesos. >> By Alexandra Sava. >> Updated May 1, 2014, 11:09 a.m. >> >> Repository: mesos-git >> Description >> >> Rename 'minloglevel' flag in 'logging_level' in order to be consistent with >> the existing flags. >> >> Rename 'getMinLogLevel' function in 'getLogSeverity' to be more suggestive >> for what it does. >> >> Disallow configuring FATAL logging because it's a special case and it >> involves too many changes across the entire source code. >> >> >> Create validation for the 'logging_level' flag in order to notify the user >> if it had entered an unsupported value. >> >> >> Update the documentation for 'logging_level' flag, with the values it can >> take. >> 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 >> >
