----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4888/#review7262 -----------------------------------------------------------
src/common/logging.cpp <https://reviews.apache.org/r/4888/#comment16037> Slightly too bad you have to repeat yourself here on the default just to avoid the Option. It might be nice if Configurator::addOption returned a handle that contained the key, value type and default value if specified and you could retrieve values from a Configuration with the handle as the fundamental form. src/common/logging.cpp <https://reviews.apache.org/r/4888/#comment16039> Reading the options above I take away: stderr is on by default (--quiet=false) logging to files is off by default (--log_dir=something turns it on) And whether or not turning on log_dir turns off stderr is unspecified - however the implementation appears to do this. It would be good to update the log_dir flag help to make this clear. src/common/logging.cpp <https://reviews.apache.org/r/4888/#comment16041> Actually - this comment makes me question my conclusion ... what is intended here vs. what is actually happening vs what do the flag helps say - Do all 3 jive? src/common/logging.cpp <https://reviews.apache.org/r/4888/#comment16040> kill trailing ws src/configurator/configuration.hpp <https://reviews.apache.org/r/4888/#comment16042> I really don't like this feature! The asymmetry with none below is jarring. Using a null -> none implicit conversion would also be jarring (if thats even valid in c++). The argument in favor then must be clarity/brevity for line 105 - which probably comes with day to day use. I am not a fan of certain abbreviations. Editorial complete. src/master/main.cpp <https://reviews.apache.org/r/4888/#comment16043> Its suprising MasterDetector doesn't use Option in favor of "" src/sched/sched.cpp <https://reviews.apache.org/r/4888/#comment16044> s/wierd/weird/ src/tests/main.cpp <https://reviews.apache.org/r/4888/#comment16045> kill trailing ws third_party/libprocess/include/process/once.hpp <https://reviews.apache.org/r/4888/#comment16046> Its not clear what the bool means from the signature alone and its critical that false lead to a done() call no matter what. Its only because you named your usage in logging.cpp in the past tense that that site reads naturally and so 'clearly' does the right thing from only local reading context. Seems like use deserves docs. - John On 2012-04-26 04:52:53, Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4888/ > ----------------------------------------------------------- > > (Updated 2012-04-26 04:52:53) > > > Review request for mesos and John Sirois. > > > Summary > ------- > > Scratched some itches and cleaned up some aspects of logging and > configuration: > > (1) Removed initializing glog from within libprocess (that should > never have been done, if a user wants that they should just set > GLOG_* environment variables before initializing libprocess). > > (2) Make 'localquiet' actually be quiet again. > > (3) Added a 'once' abstraction to libprocess (needed to only > initialize logging once). > > (4) Made mesos-tests actually use logging and configuration. > > (5) Removed unused Configuration methods (and cleaned up existing APIs > as well as how they were getting called/used). > > > Diffs > ----- > > src/common/logging.hpp f0cee46 > src/common/logging.cpp bd9c36f > src/common/webui_utils.cpp fc0cc6c > src/configurator/configuration.hpp 7addfa4 > src/exec/exec.cpp e8db407 > src/local/local.cpp affe432 > src/local/main.cpp ffb2585 > src/log/log.cpp 84e03be > src/log/main.cpp 3cf680c > src/master/main.cpp f2bad09 > src/master/webui.cpp 9f26417 > src/mesos/main.cpp a121a42 > src/sched/sched.cpp dcadb10 > src/slave/lxc_isolation_module.cpp 66a2a89 > src/slave/main.cpp 85cba25 > src/slave/process_based_isolation_module.cpp 2b37d42 > src/slave/slave.hpp 0dc7140 > src/slave/slave.cpp b233b68 > src/slave/webui.cpp 1782f6c > src/tests/main.cpp c546b8d > third_party/libprocess/include/process/nothing.hpp PRE-CREATION > third_party/libprocess/include/process/once.hpp PRE-CREATION > third_party/libprocess/include/process/process.hpp 8e957cb > third_party/libprocess/src/process.cpp 5c1123c > > Diff: https://reviews.apache.org/r/4888/diff > > > Testing > ------- > > make check > > > Thanks, > > Benjamin > >
