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/07/10 00:22:38 UTC
Review Request: FLAGS (10): A fairly significant refactor of Flags to make
composing flags better.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5863/
-----------------------------------------------------------
Review request for mesos, John Sirois and Vinod Kone.
Description
-------
After converting some things over to use the new flags abstraction, I realized that I didn't provide everything I really wanted, so I refactored it a bit. This is really the "flags" abstraction review.
Diffs
-----
src/Makefile.am 11f6b4c
src/configurator/configurator.hpp 8569645
src/flags/flag.hpp PRE-CREATION
src/flags/flags.hpp PRE-CREATION
src/flags/loader.hpp PRE-CREATION
src/logging/flags.hpp PRE-CREATION
src/logging/logging.hpp PRE-CREATION
src/logging/logging.cpp PRE-CREATION
src/master/flags.hpp PRE-CREATION
src/master/http.cpp 82784fb
src/master/main.cpp bcbe35a
src/master/master.hpp 886f79c
src/master/master.cpp 44a5dc6
src/master/webui.hpp 660d5e1
src/master/webui.cpp 56134b7
src/tests/flags_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/5863/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: FLAGS (10): A fairly significant refactor of Flags to
make composing flags better.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On July 11, 2012, 5:56 p.m., John Sirois wrote:
> > src/flags/flag.hpp, line 18
> > <https://reviews.apache.org/r/5863/diff/1/?file=120836#file120836line18>
> >
> > In isolation this is odd coupling:
> > Flag depends on FlagBase, it has no manifest value type here, but a value type is implied by the loader (it clearly knows the type it loads).
> > It would seem more natural to have
> > <typename T>
> > struct Flag<T>
> > {
> > ...
> > std::tr1::function<T(const std::string&)> parser;
> > }
> >
I need Flag to be typeless, thus no templates.
> On July 11, 2012, 5:56 p.m., John Sirois wrote:
> > src/flags/flags.hpp, line 126
> > <https://reviews.apache.org/r/5863/diff/1/?file=120837#file120837line126>
> >
> > urg - I suppose this blocks my Flag<T> suggestion :/
Yup.
> On July 11, 2012, 5:56 p.m., John Sirois wrote:
> > src/flags/flags.hpp, line 310
> > <https://reviews.apache.org/r/5863/diff/1/?file=120837#file120837line310>
> >
> > It would be nice if there was an overload where I could pass a loader reference for those times when I have a 1-off flag type to parse
Yeah, passing a function that does the work would be great. Right now you have to overload the Loader/OptionLoader/MemberLoader/MemberOptionLoader. It's possible that would even be the better "top-level" FlagsBase::add (i.e., the other 'add' functions call that one), but since that shouldn't change this interface I'm going to punt that for later.
> On July 11, 2012, 5:56 p.m., John Sirois wrote:
> > src/flags/flags.hpp, line 392
> > <https://reviews.apache.org/r/5863/diff/1/?file=120837#file120837line392>
> >
> > kill
Done.
> On July 11, 2012, 5:56 p.m., John Sirois wrote:
> > src/master/webui.hpp, line 49
> > <https://reviews.apache.org/r/5863/diff/1/?file=120847#file120847line49>
> >
> > kill trailing ws
Killed from a previous review comment.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5863/#review9073
-----------------------------------------------------------
On July 9, 2012, 10:22 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5863/
> -----------------------------------------------------------
>
> (Updated July 9, 2012, 10:22 p.m.)
>
>
> Review request for mesos, John Sirois and Vinod Kone.
>
>
> Description
> -------
>
> After converting some things over to use the new flags abstraction, I realized that I didn't provide everything I really wanted, so I refactored it a bit. This is really the "flags" abstraction review.
>
>
> Diffs
> -----
>
> src/Makefile.am 11f6b4c
> src/configurator/configurator.hpp 8569645
> src/flags/flag.hpp PRE-CREATION
> src/flags/flags.hpp PRE-CREATION
> src/flags/loader.hpp PRE-CREATION
> src/logging/flags.hpp PRE-CREATION
> src/logging/logging.hpp PRE-CREATION
> src/logging/logging.cpp PRE-CREATION
> src/master/flags.hpp PRE-CREATION
> src/master/http.cpp 82784fb
> src/master/main.cpp bcbe35a
> src/master/master.hpp 886f79c
> src/master/master.cpp 44a5dc6
> src/master/webui.hpp 660d5e1
> src/master/webui.cpp 56134b7
> src/tests/flags_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5863/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: FLAGS (10): A fairly significant refactor of Flags to
make composing flags better.
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5863/#review9073
-----------------------------------------------------------
src/flags/flag.hpp
<https://reviews.apache.org/r/5863/#comment19211>
In isolation this is odd coupling:
Flag depends on FlagBase, it has no manifest value type here, but a value type is implied by the loader (it clearly knows the type it loads).
It would seem more natural to have
<typename T>
struct Flag<T>
{
...
std::tr1::function<T(const std::string&)> parser;
}
src/flags/flags.hpp
<https://reviews.apache.org/r/5863/#comment19219>
urg - I suppose this blocks my Flag<T> suggestion :/
src/flags/flags.hpp
<https://reviews.apache.org/r/5863/#comment19218>
It would be nice if there was an overload where I could pass a loader reference for those times when I have a 1-off flag type to parse
src/flags/flags.hpp
<https://reviews.apache.org/r/5863/#comment19216>
kill
src/master/webui.hpp
<https://reviews.apache.org/r/5863/#comment19217>
kill trailing ws
- John Sirois
On July 9, 2012, 10:22 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5863/
> -----------------------------------------------------------
>
> (Updated July 9, 2012, 10:22 p.m.)
>
>
> Review request for mesos, John Sirois and Vinod Kone.
>
>
> Description
> -------
>
> After converting some things over to use the new flags abstraction, I realized that I didn't provide everything I really wanted, so I refactored it a bit. This is really the "flags" abstraction review.
>
>
> Diffs
> -----
>
> src/Makefile.am 11f6b4c
> src/configurator/configurator.hpp 8569645
> src/flags/flag.hpp PRE-CREATION
> src/flags/flags.hpp PRE-CREATION
> src/flags/loader.hpp PRE-CREATION
> src/logging/flags.hpp PRE-CREATION
> src/logging/logging.hpp PRE-CREATION
> src/logging/logging.cpp PRE-CREATION
> src/master/flags.hpp PRE-CREATION
> src/master/http.cpp 82784fb
> src/master/main.cpp bcbe35a
> src/master/master.hpp 886f79c
> src/master/master.cpp 44a5dc6
> src/master/webui.hpp 660d5e1
> src/master/webui.cpp 56134b7
> src/tests/flags_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5863/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: FLAGS (10): A fairly significant refactor of Flags to
make composing flags better.
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5863/#review9091
-----------------------------------------------------------
Ship it!
Ship It!
- John Sirois
On July 9, 2012, 10:22 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5863/
> -----------------------------------------------------------
>
> (Updated July 9, 2012, 10:22 p.m.)
>
>
> Review request for mesos, John Sirois and Vinod Kone.
>
>
> Description
> -------
>
> After converting some things over to use the new flags abstraction, I realized that I didn't provide everything I really wanted, so I refactored it a bit. This is really the "flags" abstraction review.
>
>
> Diffs
> -----
>
> src/Makefile.am 11f6b4c
> src/configurator/configurator.hpp 8569645
> src/flags/flag.hpp PRE-CREATION
> src/flags/flags.hpp PRE-CREATION
> src/flags/loader.hpp PRE-CREATION
> src/logging/flags.hpp PRE-CREATION
> src/logging/logging.hpp PRE-CREATION
> src/logging/logging.cpp PRE-CREATION
> src/master/flags.hpp PRE-CREATION
> src/master/http.cpp 82784fb
> src/master/main.cpp bcbe35a
> src/master/master.hpp 886f79c
> src/master/master.cpp 44a5dc6
> src/master/webui.hpp 660d5e1
> src/master/webui.cpp 56134b7
> src/tests/flags_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5863/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>