> On 2012-04-26 14:06:49, John Sirois wrote: > > src/common/logging.cpp, line 70 > > <https://reviews.apache.org/r/4888/diff/1/?file=104459#file104459line70> > > > > 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.
Yes, it was a design flaw from the beginning. I only helped it by adding the Option version. You suggestion is a nice one, in general I'd like to return ConfigOption instances (or collection of instances) instead of calling *::registerOptions and passing in a Configurator. There are lots of things that could get cleaned up here, but I only bit off a small piece. > On 2012-04-26 14:06:49, John Sirois wrote: > > src/common/logging.cpp, line 93 > > <https://reviews.apache.org/r/4888/diff/1/?file=104459#file104459line93> > > > > kill trailing ws Done. > On 2012-04-26 14:06:49, John Sirois wrote: > > src/sched/sched.cpp, line 640 > > <https://reviews.apache.org/r/4888/diff/1/?file=104470#file104470line640> > > > > s/wierd/weird/ Done. > On 2012-04-26 14:06:49, John Sirois wrote: > > src/master/main.cpp, line 123 > > <https://reviews.apache.org/r/4888/diff/1/?file=104467#file104467line123> > > > > Its suprising MasterDetector doesn't use Option in favor of "" This is the deprecated MasterDetector, to be replaced in the not too distance future. > On 2012-04-26 14:06:49, John Sirois wrote: > > src/tests/main.cpp, line 88 > > <https://reviews.apache.org/r/4888/diff/1/?file=104477#file104477line88> > > > > kill trailing ws Done. > On 2012-04-26 14:06:49, John Sirois wrote: > > third_party/libprocess/include/process/once.hpp, line 16 > > <https://reviews.apache.org/r/4888/diff/1/?file=104479#file104479line16> > > > > 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. Added docs. > On 2012-04-26 14:06:49, John Sirois wrote: > > src/configurator/configuration.hpp, line 113 > > <https://reviews.apache.org/r/4888/diff/1/?file=104461#file104461line113> > > > > 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. This is a question about "implicit" versus "explicit". Here's my take, if the implicit thing is harmless, I see no reason why it needs to be explicit. On the other hand, implicit things that can be super dangerous (such as using Foo& as an argument instead of const Foo&) mean that I avoid them at all costs. In this case, this seems harmless, unless I'm missing something. > On 2012-04-26 14:06:49, John Sirois wrote: > > src/common/logging.cpp, line 83 > > <https://reviews.apache.org/r/4888/diff/1/?file=104459#file104459line83> > > > > 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. > > > > I reorganized the code to make the intentions more clear and updated the help message. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4888/#review7262 ----------------------------------------------------------- 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 > >
