You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2014/03/19 18:21:19 UTC
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/
-----------------------------------------------------------
(Updated March 19, 2014, 10:21 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
- Removed the old APIs and updated tests.
Bugs: MESOS-1112
https://issues.apache.org/jira/browse/MESOS-1112
Repository: mesos-git
Description
-------
For initial feedback.
Diffs (updated)
-----
src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
Diff: https://reviews.apache.org/r/19372/diff/
Testing
-------
make check GTEST_FILTER=*Registrar*
Thanks,
Jiang Yan Xu
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37752
-----------------------------------------------------------
src/master/registrar.hpp
<https://reviews.apache.org/r/19372/#comment69398>
this should probably be a class, given its complexity
src/master/registrar.hpp
<https://reviews.apache.org/r/19372/#comment69401>
consider moving these implementations to source files, given they are fairly complex.
- Dominic Hamon
On March 19, 2014, 10:21 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 10:21 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> For initial feedback.
>
>
> Diffs
> -----
>
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Ben Mahler <be...@gmail.com>.
> On March 19, 2014, 11:05 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 3234
> > <https://reviews.apache.org/r/19372/diff/4/?file=528499#file528499line3234>
> >
> > i find this more readable:
> >
> > if (strict) {
> > return Error("...");
> > }
> > return false;
Please revert this per the comments on: https://reviews.apache.org/r/19085/
I would like us to keep the distinction between 'strict' and 'non-strict' very explicit in the code.
> On March 19, 2014, 11:05 p.m., Dominic Hamon wrote:
> > src/master/registrar.cpp, line 438
> > <https://reviews.apache.org/r/19372/diff/4/?file=528501#file528501line438>
> >
> > it's a shame you have to copy the entire operations deque here to be able to clear it before the store completes. The only way around it i can think of is to track the last store position and only apply from there until the store completes, but that's hairy. Maybe you can think of something else.
> >
> > Could you at least name this (or the member variable) differently to avoid shadowing the member variable?
>
> Jiang Yan Xu wrote:
> Yeah I am going to do some benchmark next to determine whether/how to optimize the registrar.
Keep in mind we're copying a deque of *pointers* once per write batch, it's likely not a problem so please measure first!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37819
-----------------------------------------------------------
On March 20, 2014, 12:12 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 12:12 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On March 19, 2014, 4:05 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 3234
> > <https://reviews.apache.org/r/19372/diff/4/?file=528499#file528499line3234>
> >
> > i find this more readable:
> >
> > if (strict) {
> > return Error("...");
> > }
> > return false;
>
> Ben Mahler wrote:
> Please revert this per the comments on: https://reviews.apache.org/r/19085/
>
> I would like us to keep the distinction between 'strict' and 'non-strict' very explicit in the code.
Ok reverted.
> On March 19, 2014, 4:05 p.m., Dominic Hamon wrote:
> > src/master/registrar.cpp, line 438
> > <https://reviews.apache.org/r/19372/diff/4/?file=528501#file528501line438>
> >
> > it's a shame you have to copy the entire operations deque here to be able to clear it before the store completes. The only way around it i can think of is to track the last store position and only apply from there until the store completes, but that's hairy. Maybe you can think of something else.
> >
> > Could you at least name this (or the member variable) differently to avoid shadowing the member variable?
>
> Jiang Yan Xu wrote:
> Yeah I am going to do some benchmark next to determine whether/how to optimize the registrar.
>
> Ben Mahler wrote:
> Keep in mind we're copying a deque of *pointers* once per write batch, it's likely not a problem so please measure first!
>
True :)
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37819
-----------------------------------------------------------
On March 19, 2014, 5:12 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 5:12 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On March 19, 2014, 4:05 p.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 724
> > <https://reviews.apache.org/r/19372/diff/4/?file=528498#file528498line724>
> >
> > this could be one line:
> >
> > return info.has_id() ? Nothing() : Error("...");
Nothing and Error are not convertible to one another in a ternary operator.
> On March 19, 2014, 4:05 p.m., Dominic Hamon wrote:
> > src/master/registrar.cpp, line 438
> > <https://reviews.apache.org/r/19372/diff/4/?file=528501#file528501line438>
> >
> > it's a shame you have to copy the entire operations deque here to be able to clear it before the store completes. The only way around it i can think of is to track the last store position and only apply from there until the store completes, but that's hairy. Maybe you can think of something else.
> >
> > Could you at least name this (or the member variable) differently to avoid shadowing the member variable?
Yeah I am going to do some benchmark next to determine whether/how to optimize the registrar.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37819
-----------------------------------------------------------
On March 19, 2014, 3:50 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 3:50 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> For initial feedback.
>
>
> Diffs
> -----
>
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37819
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/19372/#comment69565>
should 'validate()' be marked const in the base?
src/master/master.hpp
<https://reviews.apache.org/r/19372/#comment69564>
this could be one line:
return info.has_id() ? Nothing() : Error("...");
src/master/master.cpp
<https://reviews.apache.org/r/19372/#comment69566>
i find this more readable:
if (strict) {
return Error("...");
}
return false;
src/master/registrar.cpp
<https://reviews.apache.org/r/19372/#comment69567>
it's a shame you have to copy the entire operations deque here to be able to clear it before the store completes. The only way around it i can think of is to track the last store position and only apply from there until the store completes, but that's hairy. Maybe you can think of something else.
Could you at least name this (or the member variable) differently to avoid shadowing the member variable?
- Dominic Hamon
On March 19, 2014, 3:50 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 3:50 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> For initial feedback.
>
>
> Diffs
> -----
>
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/
-----------------------------------------------------------
(Updated March 20, 2014, 3:31 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Comments. NNFR.
Bugs: MESOS-1112
https://issues.apache.org/jira/browse/MESOS-1112
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
Diff: https://reviews.apache.org/r/19372/diff/
Testing
-------
make check GTEST_FILTER=*Registrar*
Thanks,
Jiang Yan Xu
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37986
-----------------------------------------------------------
Ship it!
Looks good Yan!
src/master/master.hpp
<https://reviews.apache.org/r/19372/#comment69863>
My fault, can you add a "// Mutation." comment here?
src/master/registrar.hpp
<https://reviews.apache.org/r/19372/#comment69864>
Can you kill these two lines?
src/master/registrar.hpp
<https://reviews.apache.org/r/19372/#comment69867>
"Applies an operation on the Registry."
src/master/registrar.cpp
<https://reviews.apache.org/r/19372/#comment69868>
My fault, can you add a "// Mutation." comment here?
- Ben Mahler
On March 20, 2014, 7:41 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 7:41 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/
-----------------------------------------------------------
(Updated March 20, 2014, 12:41 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Comments.
Bugs: MESOS-1112
https://issues.apache.org/jira/browse/MESOS-1112
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
Diff: https://reviews.apache.org/r/19372/diff/
Testing
-------
make check GTEST_FILTER=*Registrar*
Thanks,
Jiang Yan Xu
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37836
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19372]
All tests passed.
- Mesos ReviewBot
On March 20, 2014, 12:12 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 12:12 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On March 20, 2014, 1:39 a.m., Ben Mahler wrote:
> > src/master/registrar.hpp, line 42
> > <https://reviews.apache.org/r/19372/diff/5/?file=528688#file528688line42>
> >
> > Why is this no longer templated?
> >
> > I can envision us overloading our Registrar apply() call based on the type of Operation, as we add more keys outside the Registry:
> >
> > Future<bool> apply(Owned<Operation<Registry> > operation);
> > Future<bool> apply(Owned<Operation<Reservations> > operation);
> > Future<bool> apply(Owned<Operation<Repairs> > operation);
IMHO it seems easy enough to add this back in later, and simpler to read and understand right now without it. When we do add operations on multiple keys we might even consider calling it something other than the Registrar too as it becomes more of an abstraction for performing operations in batch on state variables. ;)
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37850
-----------------------------------------------------------
On March 20, 2014, 12:12 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 12:12 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On March 19, 2014, 6:39 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 732
> > <https://reviews.apache.org/r/19372/diff/5/?file=528686#file528686line732>
> >
> > Can you just forward declare these in the header?
> >
> > Otherwise I would be inclined to leave the implementation in the header given how small these are and how much easier it is to understand them. We already do this in Master for simple things like 'Slave' and 'Framework'.
> >
I cannot forward declare because it's for regisrar_tests.cpp where the operation classes are instantiated. Put it back to the header.
> On March 19, 2014, 6:39 p.m., Ben Mahler wrote:
> > src/master/registrar.hpp, lines 62-64
> > <https://reviews.apache.org/r/19372/diff/5/?file=528688#file528688line62>
> >
> > This may prove useful with future operations, but the difference between an invalid operation and an operation that cannot be applied is a bit fuzzy.
> >
> > How about for now you add some sanity CHECKs for slave IDs in the constructors for the Master's operations, and we can think about validation in a later follow up?
Fine, just wanted to put the code out there and we can reference it later.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37850
-----------------------------------------------------------
On March 19, 2014, 5:12 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 5:12 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On March 19, 2014, 6:39 p.m., Ben Mahler wrote:
> > src/master/registrar.hpp, line 42
> > <https://reviews.apache.org/r/19372/diff/5/?file=528688#file528688line42>
> >
> > Why is this no longer templated?
> >
> > I can envision us overloading our Registrar apply() call based on the type of Operation, as we add more keys outside the Registry:
> >
> > Future<bool> apply(Owned<Operation<Registry> > operation);
> > Future<bool> apply(Owned<Operation<Reservations> > operation);
> > Future<bool> apply(Owned<Operation<Repairs> > operation);
>
> Benjamin Hindman wrote:
> IMHO it seems easy enough to add this back in later, and simpler to read and understand right now without it. When we do add operations on multiple keys we might even consider calling it something other than the Registrar too as it becomes more of an abstraction for performing operations in batch on state variables. ;)
Ok I thought this registry is a high level concept like "Windows Registry" and new data types are put inside. I know that the underlying storage supports multiple variables but at least the Registrar only works with the "Registry".
Going forward will it be multiple instances of the Registrar with whatever the new name is, whether or not itself templatized, or have a single instance of it handle all the variables/keys? It's not clear to me but can we refactor then?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37850
-----------------------------------------------------------
On March 19, 2014, 5:12 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 5:12 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37850
-----------------------------------------------------------
Thanks Yan!
src/master/master.hpp
<https://reviews.apache.org/r/19372/#comment69619>
Can you just forward declare these in the header?
Otherwise I would be inclined to leave the implementation in the header given how small these are and how much easier it is to understand them. We already do this in Master for simple things like 'Slave' and 'Framework'.
src/master/registrar.hpp
<https://reviews.apache.org/r/19372/#comment69615>
Why is this no longer templated?
I can envision us overloading our Registrar apply() call based on the type of Operation, as we add more keys outside the Registry:
Future<bool> apply(Owned<Operation<Registry> > operation);
Future<bool> apply(Owned<Operation<Reservations> > operation);
Future<bool> apply(Owned<Operation<Repairs> > operation);
src/master/registrar.hpp
<https://reviews.apache.org/r/19372/#comment69617>
This may prove useful with future operations, but the difference between an invalid operation and an operation that cannot be applied is a bit fuzzy.
How about for now you add some sanity CHECKs for slave IDs in the constructors for the Master's operations, and we can think about validation in a later follow up?
src/master/registrar.hpp
<https://reviews.apache.org/r/19372/#comment69618>
What is 'info'? ;)
src/master/registrar.cpp
<https://reviews.apache.org/r/19372/#comment69613>
Okay, let's just call this 'applied' then?
src/master/registrar.cpp
<https://reviews.apache.org/r/19372/#comment69614>
Remove the 'this->' if you're changing the name of the 'operations' argument.
- Ben Mahler
On March 20, 2014, 12:12 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 12:12 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/
-----------------------------------------------------------
(Updated March 19, 2014, 5:12 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Dominic's comment.
Bugs: MESOS-1112
https://issues.apache.org/jira/browse/MESOS-1112
Repository: mesos-git
Description (updated)
-------
See summary.
Diffs (updated)
-----
src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
Diff: https://reviews.apache.org/r/19372/diff/
Testing
-------
make check GTEST_FILTER=*Registrar*
Thanks,
Jiang Yan Xu
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/
-----------------------------------------------------------
(Updated March 19, 2014, 3:50 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Addressed BenH and Dominic's comments.
Bugs: MESOS-1112
https://issues.apache.org/jira/browse/MESOS-1112
Repository: mesos-git
Description
-------
For initial feedback.
Diffs (updated)
-----
src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
Diff: https://reviews.apache.org/r/19372/diff/
Testing
-------
make check GTEST_FILTER=*Registrar*
Thanks,
Jiang Yan Xu
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37746
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19372]
All tests passed.
- Mesos ReviewBot
On March 19, 2014, 5:21 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 5:21 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> For initial feedback.
>
>
> Diffs
> -----
>
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On March 19, 2014, 10:27 a.m., Jiang Yan Xu wrote:
> > src/master/registrar.hpp, line 181
> > <https://reviews.apache.org/r/19372/diff/3/?file=528110#file528110line181>
> >
> > When the API was like: remove(const SlaveInfo&) we had the ability to overload the methods for other data types. e.g.
> > remove(const FrameworkInfo&)
> >
> > Now we have lost it. What if in the future we want a 'Remove' for something else?
> >
> > Should we rename it to RemoveSlave?
> >
> > Ditto for Admit & Readmit.
>
> Benjamin Hindman wrote:
> Changing the names make sense, and also moving the implementations from registrar.hpp into master.cpp might make sense as well (as these are master specific registrar operations.
Maybe class declarations in master.hpp and perform() implementations in master.cpp? registrar_tests.cpp needs the class declarations.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37739
-----------------------------------------------------------
On March 19, 2014, 10:21 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 10:21 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> For initial feedback.
>
>
> Diffs
> -----
>
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Dominic Hamon <dh...@twopensource.com>.
> On March 19, 2014, 10:27 a.m., Jiang Yan Xu wrote:
> > src/master/registrar.hpp, lines 42-43
> > <https://reviews.apache.org/r/19372/diff/3/?file=528110#file528110line42>
> >
> > To make it less annoying to call the APIs with all the template parameters, i.e. `apply(Owned<Operation<Registry> > > operation)`, can we not make this a template?
> >
> > AFAICT, this Operation is only intended for the "Registrar" hence I can't see it work with anything other than the "Registry".
Further to this - why does Operation inherit from Promise? Could it use composition instead?
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37739
-----------------------------------------------------------
On March 19, 2014, 5:12 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 5:12 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/master.cpp 6da776699beb6f449e8160dcb6a125d94c1ab437
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On March 19, 2014, 10:27 a.m., Jiang Yan Xu wrote:
> > src/master/registrar.hpp, lines 42-43
> > <https://reviews.apache.org/r/19372/diff/3/?file=528110#file528110line42>
> >
> > To make it less annoying to call the APIs with all the template parameters, i.e. `apply(Owned<Operation<Registry> > > operation)`, can we not make this a template?
> >
> > AFAICT, this Operation is only intended for the "Registrar" hence I can't see it work with anything other than the "Registry".
>
> Dominic Hamon wrote:
> Further to this - why does Operation inherit from Promise? Could it use composition instead?
I'll let BenM or BenH answer this but I think while composition is better as it follows the general principle, inheritance didn't do much harm here so I kept it unchanged and we can revisit this.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37739
-----------------------------------------------------------
On March 20, 2014, 12:41 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 12:41 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 0c7c5204c31087f12c4e98028f90c1b941eab7c7
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On March 19, 2014, 5:27 p.m., Jiang Yan Xu wrote:
> > src/master/registrar.hpp, line 181
> > <https://reviews.apache.org/r/19372/diff/3/?file=528110#file528110line181>
> >
> > When the API was like: remove(const SlaveInfo&) we had the ability to overload the methods for other data types. e.g.
> > remove(const FrameworkInfo&)
> >
> > Now we have lost it. What if in the future we want a 'Remove' for something else?
> >
> > Should we rename it to RemoveSlave?
> >
> > Ditto for Admit & Readmit.
Changing the names make sense, and also moving the implementations from registrar.hpp into master.cpp might make sense as well (as these are master specific registrar operations.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37739
-----------------------------------------------------------
On March 19, 2014, 5:21 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 5:21 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> For initial feedback.
>
>
> Diffs
> -----
>
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19372: Refactored the Registrar to push the operations to
the caller to simplify the interface.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19372/#review37739
-----------------------------------------------------------
src/master/registrar.hpp
<https://reviews.apache.org/r/19372/#comment69382>
To make it less annoying to call the APIs with all the template parameters, i.e. `apply(Owned<Operation<Registry> > > operation)`, can we not make this a template?
AFAICT, this Operation is only intended for the "Registrar" hence I can't see it work with anything other than the "Registry".
src/master/registrar.hpp
<https://reviews.apache.org/r/19372/#comment69383>
When the API was like: remove(const SlaveInfo&) we had the ability to overload the methods for other data types. e.g.
remove(const FrameworkInfo&)
Now we have lost it. What if in the future we want a 'Remove' for something else?
Should we rename it to RemoveSlave?
Ditto for Admit & Readmit.
- Jiang Yan Xu
On March 19, 2014, 10:21 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19372/
> -----------------------------------------------------------
>
> (Updated March 19, 2014, 10:21 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1112
> https://issues.apache.org/jira/browse/MESOS-1112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> For initial feedback.
>
>
> Diffs
> -----
>
> src/master/registrar.hpp 98bfa1e83b7e6d28e011444a665155e31922446c
> src/master/registrar.cpp cbb67bd17402692d3bb81fe58baea952897c56d1
> src/tests/registrar_tests.cpp 41836aeaf94e1b21e040b9e8ecf71e0b5a351f8e
>
> Diff: https://reviews.apache.org/r/19372/diff/
>
>
> Testing
> -------
>
> make check GTEST_FILTER=*Registrar*
>
>
> Thanks,
>
> Jiang Yan Xu
>
>