----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20850/#review41757 -----------------------------------------------------------
Looks great Alexandra, I made a few suggestions below, let me know what you think! src/logging/logging.hpp <https://reviews.apache.org/r/20850/#comment75338> It doesn't look like the logging header needs to expose this? What if we remove this function and inside 'initialize()' we directly use LOG_AT_LEVEL to indicate that logging is initialized? Keeping the comment would be great though to let others understand that we're forcing the creation of the log file. src/master/master.cpp <https://reviews.apache.org/r/20850/#comment75339> 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"; ... } - Ben Mahler On April 29, 2014, 4:06 p.m., Alexandra Sava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20850/ > ----------------------------------------------------------- > > (Updated April 29, 2014, 4:06 p.m.) > > > Review request for mesos. > > > 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 > > Diff: https://reviews.apache.org/r/20850/diff/ > > > Testing > ------- > > > Thanks, > > Alexandra Sava > >
