You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Dominic Hamon <dh...@twopensource.com> on 2014/10/23 19:51:55 UTC
Re: Review Request 20423: Moved validation visitors out of master.cpp.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/
-----------------------------------------------------------
(Updated Oct. 23, 2014, 10:51 a.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
rebased
Bugs: MESOS-1064
https://issues.apache.org/jira/browse/MESOS-1064
Repository: mesos-git
Description
-------
This is the first step toward being able to write independent unit tests for the validation visitors.
It also uses Owned to make visitor cleanup simpler (non-existent).
Diffs (updated)
-----
src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09
src/master/master.cpp 95589b8f25a3e496eabc0cf84319c883c1ed7aec
src/master/offervisitor.hpp PRE-CREATION
src/master/offervisitor.cpp PRE-CREATION
src/master/taskinfovisitor.hpp PRE-CREATION
src/master/taskinfovisitor.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20423/diff/
Testing
-------
make check.
ran Java test framework.
Thanks,
Dominic Hamon
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 2, 2014, 6:07 p.m., Jie Yu wrote:
> > src/Makefile.am, lines 274-277
> > <https://reviews.apache.org/r/20423/diff/4/?file=731480#file731480line274>
> >
> > visitor is the pattern we use to check offers and tasks. How about calling them task_checker.hpp and offer_checker.hpp?
>
> Jie Yu wrote:
> Could you please explain the reason why dropping this? At least, it should be offer_visitor.hpp (similar to drf_sorter.hpp, etc.)
>
> Dominic Hamon wrote:
> i'm naming the files after the classes within for discoverability purposes. are you suggesting i rename the classes too?
IMO, by reading the file name offer_visitor.hpp, I don't know immediately the purpose of the file until I read the code. We have things linke TaskIDChecker, UniqueTaskIdChecker, etc in the file, so even for discoverability purpose, calling it task_checker.hpp is appropriate.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review63552
-----------------------------------------------------------
On Dec. 2, 2014, 6:46 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2014, 6:46 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> src/master/master.cpp 8fcda4b9b5857f14cff8f6af2de31cca0208b88d
> src/master/offervisitor.hpp PRE-CREATION
> src/master/offervisitor.cpp PRE-CREATION
> src/master/taskinfovisitor.hpp PRE-CREATION
> src/master/taskinfovisitor.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 2, 2014, 6:07 p.m., Jie Yu wrote:
> > src/Makefile.am, lines 274-277
> > <https://reviews.apache.org/r/20423/diff/4/?file=731480#file731480line274>
> >
> > visitor is the pattern we use to check offers and tasks. How about calling them task_checker.hpp and offer_checker.hpp?
Could you please explain the reason why dropping this? At least, it should be offer_visitor.hpp (similar to drf_sorter.hpp, etc.)
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review63552
-----------------------------------------------------------
On Dec. 2, 2014, 6:46 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2014, 6:46 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> src/master/master.cpp 8fcda4b9b5857f14cff8f6af2de31cca0208b88d
> src/master/offervisitor.hpp PRE-CREATION
> src/master/offervisitor.cpp PRE-CREATION
> src/master/taskinfovisitor.hpp PRE-CREATION
> src/master/taskinfovisitor.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Dec. 2, 2014, 10:07 a.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/timeout.hpp, line 4
> > <https://reviews.apache.org/r/20423/diff/4/?file=731479#file731479line4>
> >
> > Could you please split the patches? Is this just a cleanup, or it's necessary for this patch?
rebase issue.
> On Dec. 2, 2014, 10:07 a.m., Jie Yu wrote:
> > src/Makefile.am, lines 274-277
> > <https://reviews.apache.org/r/20423/diff/4/?file=731480#file731480line274>
> >
> > visitor is the pattern we use to check offers and tasks. How about calling them task_checker.hpp and offer_checker.hpp?
>
> Jie Yu wrote:
> Could you please explain the reason why dropping this? At least, it should be offer_visitor.hpp (similar to drf_sorter.hpp, etc.)
i'm naming the files after the classes within for discoverability purposes. are you suggesting i rename the classes too?
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review63552
-----------------------------------------------------------
On Dec. 2, 2014, 10:46 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2014, 10:46 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> src/master/master.cpp 8fcda4b9b5857f14cff8f6af2de31cca0208b88d
> src/master/offervisitor.hpp PRE-CREATION
> src/master/offervisitor.cpp PRE-CREATION
> src/master/taskinfovisitor.hpp PRE-CREATION
> src/master/taskinfovisitor.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review63552
-----------------------------------------------------------
3rdparty/libprocess/include/process/timeout.hpp
<https://reviews.apache.org/r/20423/#comment105817>
Could you please split the patches? Is this just a cleanup, or it's necessary for this patch?
src/Makefile.am
<https://reviews.apache.org/r/20423/#comment105824>
visitor is the pattern we use to check offers and tasks. How about calling them task_checker.hpp and offer_checker.hpp?
src/master/offervisitor.hpp
<https://reviews.apache.org/r/20423/#comment105828>
We rarely use typedef as it's not good for readability. Could you revert?
src/master/taskinfovisitor.hpp
<https://reviews.apache.org/r/20423/#comment105829>
Ditto.
src/slave/containerizer/mesos/containerizer.cpp
<https://reviews.apache.org/r/20423/#comment105830>
Hum, looks like you need to rebase.
- Jie Yu
On Oct. 23, 2014, 10:48 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 10:48 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
> It also uses Owned to make visitor cleanup simpler (non-existent).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/timeout.hpp 0bf63e11a7a63b714aafb5386bf2d169260396ce
> src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09
> src/master/master.cpp 95589b8f25a3e496eabc0cf84319c883c1ed7aec
> src/master/offervisitor.hpp PRE-CREATION
> src/master/offervisitor.cpp PRE-CREATION
> src/master/taskinfovisitor.hpp PRE-CREATION
> src/master/taskinfovisitor.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 9f745d897119a814bd9f8e1b6a0ce5eaef60ed36
> src/zookeeper/group.hpp 924613065521a72887a2721b3db89f448fa50900
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review63565
-----------------------------------------------------------
Patch looks great!
Reviews applied: [20423]
All tests passed.
- Mesos ReviewBot
On Dec. 2, 2014, 7:02 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2014, 7:02 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> src/master/master.cpp 8fcda4b9b5857f14cff8f6af2de31cca0208b88d
> src/master/offer_visitor.hpp PRE-CREATION
> src/master/offer_visitor.cpp PRE-CREATION
> src/master/taskinfo_visitor.hpp PRE-CREATION
> src/master/taskinfo_visitor.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review63579
-----------------------------------------------------------
Patch looks great!
Reviews applied: [20423]
All tests passed.
- Mesos ReviewBot
On Dec. 2, 2014, 8:36 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2014, 8:36 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> src/master/master.cpp 8fcda4b9b5857f14cff8f6af2de31cca0208b88d
> src/master/offer_checker.hpp PRE-CREATION
> src/master/offer_checker.cpp PRE-CREATION
> src/master/task_checker.hpp PRE-CREATION
> src/master/task_checker.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Dec. 2, 2014, 1:02 p.m., Ben Mahler wrote:
> > I think I agree with Vinod that this is just movement without adding any value, for now, given that these are not independent of the Master.
> >
> > In fact, to understand these, one needs to understand the internals of the Master, which now requires even more non-local reasoning (need to be jumping across files to understand these). IMO this makes it harder to understand, and I would punt on this change in favor of first trying to clean these up to make them unit-testable. Until then, the loss of local reasoning in this change seems really unfortunate.
> >
> > This is not yours, but the existing names could be improved. What is a "checker"? And yes, these are "Visitors", but they are all ultimately for validation.
> > So, how about we call these Validators? TaskIDValidator is a TaskValidator, UniqueOfferIDValidator is an OfferValidator.
There's huge value in removing code from a source file and localising them in another. From a discoverability and readability point of view, at least. Smaller, localised, files allow for better reasoning, better discoverability, and less impactful changes. Ie, changing the logic in one of these checkers shouldn't be a change to master.cpp.
We are adding more validators and every one makes master.cpp larger, heavier, and harder to navigate.
Yes, these are used by the master internally, but that's why they're in the master folder in src and in the relevant namespace. The logical extension of your reasoning is that all code relating to the master should be in one file, which is obviously (i think) a mistake.
I'm happy to rename the classes in a followup patch, but we could bikeshed that for a while so I don't want to conflate them.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review63577
-----------------------------------------------------------
On Dec. 2, 2014, 12:36 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2014, 12:36 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> src/master/master.cpp 8fcda4b9b5857f14cff8f6af2de31cca0208b88d
> src/master/offer_checker.hpp PRE-CREATION
> src/master/offer_checker.cpp PRE-CREATION
> src/master/task_checker.hpp PRE-CREATION
> src/master/task_checker.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review63577
-----------------------------------------------------------
I think I agree with Vinod that this is just movement without adding any value, for now, given that these are not independent of the Master.
In fact, to understand these, one needs to understand the internals of the Master, which now requires even more non-local reasoning (need to be jumping across files to understand these). IMO this makes it harder to understand, and I would punt on this change in favor of first trying to clean these up to make them unit-testable. Until then, the loss of local reasoning in this change seems really unfortunate.
This is not yours, but the existing names could be improved. What is a "checker"? And yes, these are "Visitors", but they are all ultimately for validation.
So, how about we call these Validators? TaskIDValidator is a TaskValidator, UniqueOfferIDValidator is an OfferValidator.
- Ben Mahler
On Dec. 2, 2014, 8:36 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2014, 8:36 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
>
> Diffs
> -----
>
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> src/master/master.cpp 8fcda4b9b5857f14cff8f6af2de31cca0208b88d
> src/master/offer_checker.hpp PRE-CREATION
> src/master/offer_checker.cpp PRE-CREATION
> src/master/task_checker.hpp PRE-CREATION
> src/master/task_checker.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/
-----------------------------------------------------------
(Updated Dec. 2, 2014, 12:36 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
renamed again :)
Bugs: MESOS-1064
https://issues.apache.org/jira/browse/MESOS-1064
Repository: mesos-git
Description
-------
This is the first step toward being able to write independent unit tests for the validation visitors.
Diffs (updated)
-----
src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
src/master/master.cpp 8fcda4b9b5857f14cff8f6af2de31cca0208b88d
src/master/offer_checker.hpp PRE-CREATION
src/master/offer_checker.cpp PRE-CREATION
src/master/task_checker.hpp PRE-CREATION
src/master/task_checker.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20423/diff/
Testing
-------
make check.
ran Java test framework.
Thanks,
Dominic Hamon
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/
-----------------------------------------------------------
(Updated Dec. 2, 2014, 11:02 a.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
renamed
Bugs: MESOS-1064
https://issues.apache.org/jira/browse/MESOS-1064
Repository: mesos-git
Description
-------
This is the first step toward being able to write independent unit tests for the validation visitors.
Diffs (updated)
-----
src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
src/master/master.cpp 8fcda4b9b5857f14cff8f6af2de31cca0208b88d
src/master/offer_visitor.hpp PRE-CREATION
src/master/offer_visitor.cpp PRE-CREATION
src/master/taskinfo_visitor.hpp PRE-CREATION
src/master/taskinfo_visitor.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20423/diff/
Testing
-------
make check.
ran Java test framework.
Thanks,
Dominic Hamon
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/
-----------------------------------------------------------
(Updated Dec. 2, 2014, 10:46 a.m.)
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-1064
https://issues.apache.org/jira/browse/MESOS-1064
Repository: mesos-git
Description (updated)
-------
This is the first step toward being able to write independent unit tests for the validation visitors.
Diffs (updated)
-----
src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
src/master/master.cpp 8fcda4b9b5857f14cff8f6af2de31cca0208b88d
src/master/offervisitor.hpp PRE-CREATION
src/master/offervisitor.cpp PRE-CREATION
src/master/taskinfovisitor.hpp PRE-CREATION
src/master/taskinfovisitor.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20423/diff/
Testing
-------
make check.
ran Java test framework.
Thanks,
Dominic Hamon
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review58242
-----------------------------------------------------------
Patch looks great!
Reviews applied: [20423]
All tests passed.
- Mesos ReviewBot
On Oct. 23, 2014, 10:48 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 10:48 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
> It also uses Owned to make visitor cleanup simpler (non-existent).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/timeout.hpp 0bf63e11a7a63b714aafb5386bf2d169260396ce
> src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09
> src/master/master.cpp 95589b8f25a3e496eabc0cf84319c883c1ed7aec
> src/master/offervisitor.hpp PRE-CREATION
> src/master/offervisitor.cpp PRE-CREATION
> src/master/taskinfovisitor.hpp PRE-CREATION
> src/master/taskinfovisitor.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 9f745d897119a814bd9f8e1b6a0ce5eaef60ed36
> src/zookeeper/group.hpp 924613065521a72887a2721b3db89f448fa50900
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/
-----------------------------------------------------------
(Updated Oct. 23, 2014, 3:48 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
removed blank line at EOF
Bugs: MESOS-1064
https://issues.apache.org/jira/browse/MESOS-1064
Repository: mesos-git
Description
-------
This is the first step toward being able to write independent unit tests for the validation visitors.
It also uses Owned to make visitor cleanup simpler (non-existent).
Diffs (updated)
-----
3rdparty/libprocess/include/process/timeout.hpp 0bf63e11a7a63b714aafb5386bf2d169260396ce
src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09
src/master/master.cpp 95589b8f25a3e496eabc0cf84319c883c1ed7aec
src/master/offervisitor.hpp PRE-CREATION
src/master/offervisitor.cpp PRE-CREATION
src/master/taskinfovisitor.hpp PRE-CREATION
src/master/taskinfovisitor.cpp PRE-CREATION
src/slave/containerizer/mesos/containerizer.cpp 9f745d897119a814bd9f8e1b6a0ce5eaef60ed36
src/zookeeper/group.hpp 924613065521a72887a2721b3db89f448fa50900
Diff: https://reviews.apache.org/r/20423/diff/
Testing
-------
make check.
ran Java test framework.
Thanks,
Dominic Hamon
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review58067
-----------------------------------------------------------
Bad patch!
Reviews applied: [20423]
Failed command: ./support/apply-review.sh -n -r 20423
Error:
--2014-10-23 19:35:19-- https://reviews.apache.org/r/20423/diff/raw/
Resolving reviews.apache.org (reviews.apache.org)... 140.211.11.74
Connecting to reviews.apache.org (reviews.apache.org)|140.211.11.74|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 31756 (31K) [text/x-patch]
Saving to: '20423.patch'
0K .......... .......... .......... . 100% 970K=0.03s
2014-10-23 19:35:20 (970 KB/s) - '20423.patch' saved [31756/31756]
20423.patch:819: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Successfully applied: Moved validation visitors out of master.cpp.
This is the first step toward being able to write independent unit tests for the validation visitors.
It also uses Owned to make visitor cleanup simpler (non-existent).
Review: https://reviews.apache.org/r/20423
src/master/taskinfovisitor.hpp:177: new blank line at EOF.
Failed to commit patch
- Mesos ReviewBot
On Oct. 23, 2014, 5:51 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 5:51 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
> It also uses Owned to make visitor cleanup simpler (non-existent).
>
>
> Diffs
> -----
>
> src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09
> src/master/master.cpp 95589b8f25a3e496eabc0cf84319c883c1ed7aec
> src/master/offervisitor.hpp PRE-CREATION
> src/master/offervisitor.cpp PRE-CREATION
> src/master/taskinfovisitor.hpp PRE-CREATION
> src/master/taskinfovisitor.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Oct. 23, 2014, 12:46 p.m., Vinod Kone wrote:
> > Why are some definitions in .hpp and some in .cpp? Why not all in .cpp?
> >
> > Also, it's not clear to me how this split would help in unit testing? AFAICT, all these visitors take Master or Framework or Slave which needs bringing up a cluster.
>
> Dominic Hamon wrote:
> They could be all in cpp, but some are so simple that being inlineable seemed beneficial.
>
> This is a first step. The Master/Slave/Frameworks passed in could be mock/stub versions that would support lightweight testing. The change also has a benefit in reducing the amount of code in the master source file, which helps with compile time and readability.
>
> Cody Maloney wrote:
> Things in cpp files can be inlined with LTO (Which is something I'd like to get functioning in Mesos tooling in in the long term). There isn't much code which includes the header, so the increase in compile time / object size from having more in the header isn't something I'm worried about in this case though, so net "meh" from me either way.
>
> Reducing code in master.cpp is definitely good, although most of the slowness compiling it comes from things it includes, not master.cpp itself. (Flags tends to be one of the worse offenders last time I did some investigation).
Agreed on all points. The main concern for me is starting to tease apart master.cpp to make things more readable, more composable, and to start to adhere to the Single Responsibility Principle. As a side effect, we can start writing more focused unit testing rather than integration testing (ie, the validation steps can be tested in isolation rather than requiring a master/slave/framework to actually be running). This should help our test time and increase code coverage (in cases where we don't happen to test the validation, which is admittedly rare).
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review58071
-----------------------------------------------------------
On Oct. 23, 2014, 3:48 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 3:48 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
> It also uses Owned to make visitor cleanup simpler (non-existent).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/timeout.hpp 0bf63e11a7a63b714aafb5386bf2d169260396ce
> src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09
> src/master/master.cpp 95589b8f25a3e496eabc0cf84319c883c1ed7aec
> src/master/offervisitor.hpp PRE-CREATION
> src/master/offervisitor.cpp PRE-CREATION
> src/master/taskinfovisitor.hpp PRE-CREATION
> src/master/taskinfovisitor.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 9f745d897119a814bd9f8e1b6a0ce5eaef60ed36
> src/zookeeper/group.hpp 924613065521a72887a2721b3db89f448fa50900
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Cody Maloney <co...@mesosphere.io>.
> On Oct. 23, 2014, 7:46 p.m., Vinod Kone wrote:
> > Why are some definitions in .hpp and some in .cpp? Why not all in .cpp?
> >
> > Also, it's not clear to me how this split would help in unit testing? AFAICT, all these visitors take Master or Framework or Slave which needs bringing up a cluster.
>
> Dominic Hamon wrote:
> They could be all in cpp, but some are so simple that being inlineable seemed beneficial.
>
> This is a first step. The Master/Slave/Frameworks passed in could be mock/stub versions that would support lightweight testing. The change also has a benefit in reducing the amount of code in the master source file, which helps with compile time and readability.
Things in cpp files can be inlined with LTO (Which is something I'd like to get functioning in Mesos tooling in in the long term). There isn't much code which includes the header, so the increase in compile time / object size from having more in the header isn't something I'm worried about in this case though, so net "meh" from me either way.
Reducing code in master.cpp is definitely good, although most of the slowness compiling it comes from things it includes, not master.cpp itself. (Flags tends to be one of the worse offenders last time I did some investigation).
- Cody
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review58071
-----------------------------------------------------------
On Oct. 23, 2014, 10:48 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 10:48 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
> It also uses Owned to make visitor cleanup simpler (non-existent).
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/timeout.hpp 0bf63e11a7a63b714aafb5386bf2d169260396ce
> src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09
> src/master/master.cpp 95589b8f25a3e496eabc0cf84319c883c1ed7aec
> src/master/offervisitor.hpp PRE-CREATION
> src/master/offervisitor.cpp PRE-CREATION
> src/master/taskinfovisitor.hpp PRE-CREATION
> src/master/taskinfovisitor.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 9f745d897119a814bd9f8e1b6a0ce5eaef60ed36
> src/zookeeper/group.hpp 924613065521a72887a2721b3db89f448fa50900
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On Oct. 23, 2014, 12:46 p.m., Vinod Kone wrote:
> > Why are some definitions in .hpp and some in .cpp? Why not all in .cpp?
> >
> > Also, it's not clear to me how this split would help in unit testing? AFAICT, all these visitors take Master or Framework or Slave which needs bringing up a cluster.
They could be all in cpp, but some are so simple that being inlineable seemed beneficial.
This is a first step. The Master/Slave/Frameworks passed in could be mock/stub versions that would support lightweight testing. The change also has a benefit in reducing the amount of code in the master source file, which helps with compile time and readability.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review58071
-----------------------------------------------------------
On Oct. 23, 2014, 10:51 a.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 10:51 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
> It also uses Owned to make visitor cleanup simpler (non-existent).
>
>
> Diffs
> -----
>
> src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09
> src/master/master.cpp 95589b8f25a3e496eabc0cf84319c883c1ed7aec
> src/master/offervisitor.hpp PRE-CREATION
> src/master/offervisitor.cpp PRE-CREATION
> src/master/taskinfovisitor.hpp PRE-CREATION
> src/master/taskinfovisitor.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>
Re: Review Request 20423: Moved validation visitors out of master.cpp.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20423/#review58071
-----------------------------------------------------------
Why are some definitions in .hpp and some in .cpp? Why not all in .cpp?
Also, it's not clear to me how this split would help in unit testing? AFAICT, all these visitors take Master or Framework or Slave which needs bringing up a cluster.
- Vinod Kone
On Oct. 23, 2014, 5:51 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20423/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 5:51 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-1064
> https://issues.apache.org/jira/browse/MESOS-1064
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is the first step toward being able to write independent unit tests for the validation visitors.
>
> It also uses Owned to make visitor cleanup simpler (non-existent).
>
>
> Diffs
> -----
>
> src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09
> src/master/master.cpp 95589b8f25a3e496eabc0cf84319c883c1ed7aec
> src/master/offervisitor.hpp PRE-CREATION
> src/master/offervisitor.cpp PRE-CREATION
> src/master/taskinfovisitor.hpp PRE-CREATION
> src/master/taskinfovisitor.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20423/diff/
>
>
> Testing
> -------
>
> make check.
> ran Java test framework.
>
>
> Thanks,
>
> Dominic Hamon
>
>