You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2015/06/25 02:31:46 UTC
Review Request 35858: Added Message call support to the master and
the C++ scheduler library.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35858/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Ben Mahler, Isabel Jimenez, and Marco Massenzio.
Bugs: MESOS-2551
https://issues.apache.org/jira/browse/MESOS-2551
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df
src/scheduler/scheduler.cpp 1efc6fb351e49deaa8f626823592bc9155f5137b
src/tests/scheduler_tests.cpp cbe6c91a1b4f864ceb11cf062da0ada6c9666f9f
Diff: https://reviews.apache.org/r/35858/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 35858: Added Message call support to the master
and the C++ scheduler library.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35858/#review89696
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On June 25, 2015, 12:31 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35858/
> -----------------------------------------------------------
>
> (Updated June 25, 2015, 12:31 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Isabel Jimenez, and Marco Massenzio.
>
>
> Bugs: MESOS-2551
> https://issues.apache.org/jira/browse/MESOS-2551
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df
> src/scheduler/scheduler.cpp 1efc6fb351e49deaa8f626823592bc9155f5137b
> src/tests/scheduler_tests.cpp cbe6c91a1b4f864ceb11cf062da0ada6c9666f9f
>
> Diff: https://reviews.apache.org/r/35858/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 35858: Added Message call support to the master
and the C++ scheduler library.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 25, 2015, 4:35 p.m., Anand Mazumdar wrote:
> > src/master/master.cpp, line 3139
> > <https://reviews.apache.org/r/35858/diff/1/?file=991723#file991723line3139>
> >
> > I had a general query regarding pre-pending the function arguments with _ in our style guide. Here we do the opposite i.e. use a trailing underscore. Are both allowed or is it preferred to use the one from the style guide ?
We've gone with prefix _ for parameters to constructors or functions so that we can avoid shadowing variable names. Ultimately we're trying to reserve suffix _ for member fields, ala Google Style, but we've used suffix _ for naming variables as though they're "prime", i.e., 'message_' here reads "message prime". Ultimately we should just use suffix _ for member fields.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35858/#review89371
-----------------------------------------------------------
On June 25, 2015, 12:31 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35858/
> -----------------------------------------------------------
>
> (Updated June 25, 2015, 12:31 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Isabel Jimenez, and Marco Massenzio.
>
>
> Bugs: MESOS-2551
> https://issues.apache.org/jira/browse/MESOS-2551
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df
> src/scheduler/scheduler.cpp 1efc6fb351e49deaa8f626823592bc9155f5137b
> src/tests/scheduler_tests.cpp cbe6c91a1b4f864ceb11cf062da0ada6c9666f9f
>
> Diff: https://reviews.apache.org/r/35858/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 35858: Added Message call support to the master
and the C++ scheduler library.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35858/#review89371
-----------------------------------------------------------
Ship it!
src/master/master.cpp (line 3139)
<https://reviews.apache.org/r/35858/#comment141950>
I had a general query regarding pre-pending the function arguments with _ in our style guide. Here we do the opposite i.e. use a trailing underscore. Are both allowed or is it preferred to use the one from the style guide ?
- Anand Mazumdar
On June 25, 2015, 12:31 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35858/
> -----------------------------------------------------------
>
> (Updated June 25, 2015, 12:31 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Isabel Jimenez, and Marco Massenzio.
>
>
> Bugs: MESOS-2551
> https://issues.apache.org/jira/browse/MESOS-2551
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df
> src/scheduler/scheduler.cpp 1efc6fb351e49deaa8f626823592bc9155f5137b
> src/tests/scheduler_tests.cpp cbe6c91a1b4f864ceb11cf062da0ada6c9666f9f
>
> Diff: https://reviews.apache.org/r/35858/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 35858: Added Message call support to the master
and the C++ scheduler library.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 25, 2015, 4:12 p.m., Marco Massenzio wrote:
> > src/tests/scheduler_tests.cpp, line 830
> > <https://reviews.apache.org/r/35858/diff/1/?file=991725#file991725line830>
> >
> > This is a great test (in fact, this looks to me like something we could extract as example of call sequences in some dev guide?) but it is also a big, scary test: is there any way it can be broken down in smaller chunks?
> >
> > I also have the impression that several of the 'sub-segments' are also present in other tests: could we reuse them across other tests, maybe?
> >
> > Finally, while it may all be very obvious to you :) I would suggest to add some explanation output on some of the failures, so that in future people less-experienced will be able to more easily analyze failures, should changes induce breakages.
Not sure what sort of comments do you want in failure messages. GTEST already prints out the expected and actual values with variable names. FWIW, it is consistent with the rest of the code base.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35858/#review89369
-----------------------------------------------------------
On July 1, 2015, 5:26 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35858/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 5:26 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Isabel Jimenez, and Marco Massenzio.
>
>
> Bugs: MESOS-2551
> https://issues.apache.org/jira/browse/MESOS-2551
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
> src/scheduler/scheduler.cpp f360e4d062488986b14e3d48d140996e8ed9e7d6
> src/tests/scheduler_tests.cpp cbe6c91a1b4f864ceb11cf062da0ada6c9666f9f
>
> Diff: https://reviews.apache.org/r/35858/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 35858: Added Message call support to the master
and the C++ scheduler library.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35858/#review89369
-----------------------------------------------------------
Ship it!
I would like, if possible, to see some documentation added to the new methods (or old ones too, for that matter) in the form of Doxygen comments: they help greatly in understanding what classes/methods do.
src/tests/scheduler_tests.cpp (line 830)
<https://reviews.apache.org/r/35858/#comment141944>
This is a great test (in fact, this looks to me like something we could extract as example of call sequences in some dev guide?) but it is also a big, scary test: is there any way it can be broken down in smaller chunks?
I also have the impression that several of the 'sub-segments' are also present in other tests: could we reuse them across other tests, maybe?
Finally, while it may all be very obvious to you :) I would suggest to add some explanation output on some of the failures, so that in future people less-experienced will be able to more easily analyze failures, should changes induce breakages.
- Marco Massenzio
On June 25, 2015, 12:31 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35858/
> -----------------------------------------------------------
>
> (Updated June 25, 2015, 12:31 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Isabel Jimenez, and Marco Massenzio.
>
>
> Bugs: MESOS-2551
> https://issues.apache.org/jira/browse/MESOS-2551
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df
> src/scheduler/scheduler.cpp 1efc6fb351e49deaa8f626823592bc9155f5137b
> src/tests/scheduler_tests.cpp cbe6c91a1b4f864ceb11cf062da0ada6c9666f9f
>
> Diff: https://reviews.apache.org/r/35858/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 35858: Added Message call support to the master
and the C++ scheduler library.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 25, 2015, 5:20 p.m., Isabel Jimenez wrote:
> > src/scheduler/scheduler.cpp, line 306
> > <https://reviews.apache.org/r/35858/diff/1/?file=991724#file991724line306>
> >
> > Now that we're sending calls in every case (besides SUBSCRIBE) of this switch, I suppose it'll be better to move this to the end and maybe test just for SUBSCRIBE outside the switch?
i tried doing that but felt this was more readable, especially for cases like TEARDOWN which doesn't have any logic in the case statement.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35858/#review89384
-----------------------------------------------------------
On July 1, 2015, 5:26 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35858/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 5:26 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Isabel Jimenez, and Marco Massenzio.
>
>
> Bugs: MESOS-2551
> https://issues.apache.org/jira/browse/MESOS-2551
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
> src/scheduler/scheduler.cpp f360e4d062488986b14e3d48d140996e8ed9e7d6
> src/tests/scheduler_tests.cpp cbe6c91a1b4f864ceb11cf062da0ada6c9666f9f
>
> Diff: https://reviews.apache.org/r/35858/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 35858: Added Message call support to the master
and the C++ scheduler library.
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35858/#review89384
-----------------------------------------------------------
src/scheduler/scheduler.cpp (line 306)
<https://reviews.apache.org/r/35858/#comment141964>
Now that we're sending calls in every case (besides SUBSCRIBE) of this switch, I suppose it'll be better to move this to the end and maybe test just for SUBSCRIBE outside the switch?
- Isabel Jimenez
On June 25, 2015, 12:31 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35858/
> -----------------------------------------------------------
>
> (Updated June 25, 2015, 12:31 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Isabel Jimenez, and Marco Massenzio.
>
>
> Bugs: MESOS-2551
> https://issues.apache.org/jira/browse/MESOS-2551
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df
> src/scheduler/scheduler.cpp 1efc6fb351e49deaa8f626823592bc9155f5137b
> src/tests/scheduler_tests.cpp cbe6c91a1b4f864ceb11cf062da0ada6c9666f9f
>
> Diff: https://reviews.apache.org/r/35858/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 35858: Added Message call support to the master
and the C++ scheduler library.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35858/#review89306
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35855, 35856, 35857, 35858]
All tests passed.
- Mesos ReviewBot
On June 25, 2015, 12:31 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35858/
> -----------------------------------------------------------
>
> (Updated June 25, 2015, 12:31 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, Isabel Jimenez, and Marco Massenzio.
>
>
> Bugs: MESOS-2551
> https://issues.apache.org/jira/browse/MESOS-2551
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
> src/master/master.cpp 0782b543b451921d2240958c7ef612a9e30972df
> src/scheduler/scheduler.cpp 1efc6fb351e49deaa8f626823592bc9155f5137b
> src/tests/scheduler_tests.cpp cbe6c91a1b4f864ceb11cf062da0ada6c9666f9f
>
> Diff: https://reviews.apache.org/r/35858/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 35858: Added Message call support to the master
and the C++ scheduler library.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35858/
-----------------------------------------------------------
(Updated July 1, 2015, 10:19 p.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Isabel Jimenez, and Marco Massenzio.
Changes
-------
rebased. NNFR.
Bugs: MESOS-2551
https://issues.apache.org/jira/browse/MESOS-2551
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
src/scheduler/scheduler.cpp f360e4d062488986b14e3d48d140996e8ed9e7d6
src/tests/scheduler_tests.cpp cbe6c91a1b4f864ceb11cf062da0ada6c9666f9f
Diff: https://reviews.apache.org/r/35858/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 35858: Added Message call support to the master
and the C++ scheduler library.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35858/
-----------------------------------------------------------
(Updated July 1, 2015, 5:26 a.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, Isabel Jimenez, and Marco Massenzio.
Changes
-------
rebased on top of Call protobuf changes. haven't addressed comments yet.
Bugs: MESOS-2551
https://issues.apache.org/jira/browse/MESOS-2551
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064
src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92
src/scheduler/scheduler.cpp f360e4d062488986b14e3d48d140996e8ed9e7d6
src/tests/scheduler_tests.cpp cbe6c91a1b4f864ceb11cf062da0ada6c9666f9f
Diff: https://reviews.apache.org/r/35858/diff/
Testing
-------
make check
Thanks,
Vinod Kone