You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2012/04/26 06:52:53 UTC
Review Request: Logging and configuration cleanup.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4888/
-----------------------------------------------------------
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
Re: Review Request: Logging and configuration cleanup.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> 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
>
>
Re: Review Request: Logging and configuration cleanup.
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request: Logging and configuration cleanup.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On 2012-04-30 21:59:05, John Sirois wrote:
> >
Thanks! Submitting.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4888/#review7394
-----------------------------------------------------------
On 2012-04-26 20:25:04, Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4888/
> -----------------------------------------------------------
>
> (Updated 2012-04-26 20:25:04)
>
>
> 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
>
>
Re: Review Request: Logging and configuration cleanup.
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4888/#review7394
-----------------------------------------------------------
Ship it!
src/common/logging.cpp
<https://reviews.apache.org/r/4888/#comment16302>
kill trailing ws
- John
On 2012-04-26 20:25:04, Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4888/
> -----------------------------------------------------------
>
> (Updated 2012-04-26 20:25:04)
>
>
> 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
>
>
Re: Review Request: Logging and configuration cleanup.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4888/
-----------------------------------------------------------
(Updated 2012-04-26 20:25:04.529822)
Review request for mesos and John Sirois.
Changes
-------
Review comments.
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 (updated)
-----
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