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<https://reviews.apache.org/r/20850/diff/1/?file=570723#file570723line524> > (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 <https://reviews.apache.org/r/20850/diff/> >
