You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2014/07/16 05:16:34 UTC
Review Request 23542: Do not require the SlaveID in reconciliation requests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23542/
-----------------------------------------------------------
Review request for mesos, Niklas Nielsen and Vinod Kone.
Bugs: MESOS-1525
https://issues.apache.org/jira/browse/MESOS-1525
Repository: mesos-git
Description
-------
I would recommend applying this patch to review the reconcileTasks() logic instead of using the diff viewer.
Reconciliation requests currently specify a list of TaskStatuses. SlaveID is optional inside TaskStatus but reconciliation requests are dropped when the SlaveID is not specified. We can answer reconciliation requests for a task so long as there are no transient slaves, this is what we should do when the slave id is not specified.
Also, I realized that we should answer when a non-strict registry is in use, see the comment.
Diffs
-----
src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20
src/master/master.hpp 10bd95abc816c58fc2b72d3ce2d85128333cde39
src/master/master.cpp 3ba8c334b38e16f072f59f13c718b2deb48a39f4
src/tests/reconciliation_tests.cpp 6edbf7563375a69e3148e74a8cd99ddd13fc445b
Diff: https://reviews.apache.org/r/23542/diff/
Testing
-------
Removed a test that is now invalid and added a test for pending tasks.
Thanks,
Ben Mahler
Re: Review Request 23542: Do not require the SlaveID in reconciliation
requests.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23542/#review48242
-----------------------------------------------------------
Bad patch!
Reviews applied: [23542]
Failed command: git apply --index 23542.patch
Error:
error: patch failed: src/common/protobuf_utils.hpp:48
error: src/common/protobuf_utils.hpp: patch does not apply
error: patch failed: src/master/master.hpp:492
error: src/master/master.hpp: patch does not apply
error: patch failed: src/master/master.cpp:3278
error: src/master/master.cpp: patch does not apply
- Mesos ReviewBot
On July 16, 2014, 3:16 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23542/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 3:16 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-1525
> https://issues.apache.org/jira/browse/MESOS-1525
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I would recommend applying this patch to review the reconcileTasks() logic instead of using the diff viewer.
>
> Reconciliation requests currently specify a list of TaskStatuses. SlaveID is optional inside TaskStatus but reconciliation requests are dropped when the SlaveID is not specified. We can answer reconciliation requests for a task so long as there are no transient slaves, this is what we should do when the slave id is not specified.
>
> Also, I realized that we should answer when a non-strict registry is in use, see the comment.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20
> src/master/master.hpp 10bd95abc816c58fc2b72d3ce2d85128333cde39
> src/master/master.cpp 3ba8c334b38e16f072f59f13c718b2deb48a39f4
> src/tests/reconciliation_tests.cpp 6edbf7563375a69e3148e74a8cd99ddd13fc445b
>
> Diff: https://reviews.apache.org/r/23542/diff/
>
>
> Testing
> -------
>
> Removed a test that is now invalid and added a test for pending tasks.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23542: Do not require the SlaveID in reconciliation
requests.
Posted by Vinod Kone <vi...@gmail.com>.
> On July 18, 2014, 6:38 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 500
> > <https://reviews.apache.org/r/23542/diff/1/?file=633320#file633320line500>
> >
> > pull "||" up to the end of the previous lines?
>
> Ben Mahler wrote:
> I was thinking about the readability of conditional wrapping recently from a thread here, where the GNU formatting uses this style:
> https://news.ycombinator.com/item?id=7975386
> https://www.gnu.org/prep/standards/html_node/Formatting.html
>
> Example to consider:
>
> if (recovered.contains(slaveId.get()) ||
> reregistering.contains(slaveId.get()) ||
> registered.contains(slaveId.get()) ||
> removing.contains(slaveId.get()) {
>
> if (recovered.contains(slaveId.get())
> || reregistering.contains(slaveId.get())
> || registered.contains(slaveId.get())
> || removing.contains(slaveId.get()) {
>
> The alignment of the || operator across lines exposes the underlying structure more clearly. Less "jaggedness" as we used to talk about.
>
> The google style guide refers to the former as being more common but allows both:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolean_Expressions#Boolean_Expressions
interesting. are you planning to do a sweep of our code base and update per this new convention? otherwise, i prefer sticking to the former style to be consistent.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23542/#review48149
-----------------------------------------------------------
On July 21, 2014, 7:57 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23542/
> -----------------------------------------------------------
>
> (Updated July 21, 2014, 7:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-1525
> https://issues.apache.org/jira/browse/MESOS-1525
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I would recommend applying this patch to review the reconcileTasks() logic instead of using the diff viewer.
>
> Reconciliation requests currently specify a list of TaskStatuses. SlaveID is optional inside TaskStatus but reconciliation requests are dropped when the SlaveID is not specified. We can answer reconciliation requests for a task so long as there are no transient slaves, this is what we should do when the slave id is not specified.
>
> Also, I realized that we should answer when a non-strict registry is in use, see the comment.
>
>
> Diffs
> -----
>
> src/master/master.hpp 7e7a75bd7e0fafc084ad2663c894e76e5fb35edd
> src/master/master.cpp dc60c47b9c08b1e83fd72b6b86393fdc11314ea1
> src/tests/reconciliation_tests.cpp 6edbf7563375a69e3148e74a8cd99ddd13fc445b
>
> Diff: https://reviews.apache.org/r/23542/diff/
>
>
> Testing
> -------
>
> Removed a test that is now invalid and added a test for pending tasks.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23542: Do not require the SlaveID in reconciliation
requests.
Posted by Ben Mahler <be...@gmail.com>.
> On July 18, 2014, 6:38 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 500
> > <https://reviews.apache.org/r/23542/diff/1/?file=633320#file633320line500>
> >
> > pull "||" up to the end of the previous lines?
>
> Ben Mahler wrote:
> I was thinking about the readability of conditional wrapping recently from a thread here, where the GNU formatting uses this style:
> https://news.ycombinator.com/item?id=7975386
> https://www.gnu.org/prep/standards/html_node/Formatting.html
>
> Example to consider:
>
> if (recovered.contains(slaveId.get()) ||
> reregistering.contains(slaveId.get()) ||
> registered.contains(slaveId.get()) ||
> removing.contains(slaveId.get()) {
>
> if (recovered.contains(slaveId.get())
> || reregistering.contains(slaveId.get())
> || registered.contains(slaveId.get())
> || removing.contains(slaveId.get()) {
>
> The alignment of the || operator across lines exposes the underlying structure more clearly. Less "jaggedness" as we used to talk about.
>
> The google style guide refers to the former as being more common but allows both:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolean_Expressions#Boolean_Expressions
>
> Vinod Kone wrote:
> interesting. are you planning to do a sweep of our code base and update per this new convention? otherwise, i prefer sticking to the former style to be consistent.
Not planning to change this now so I will stick with the current style.
Won't be doing a sweep myself, but it's a style change I would vote for if someone was willing to do a code sweep.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23542/#review48149
-----------------------------------------------------------
On July 21, 2014, 7:57 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23542/
> -----------------------------------------------------------
>
> (Updated July 21, 2014, 7:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-1525
> https://issues.apache.org/jira/browse/MESOS-1525
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I would recommend applying this patch to review the reconcileTasks() logic instead of using the diff viewer.
>
> Reconciliation requests currently specify a list of TaskStatuses. SlaveID is optional inside TaskStatus but reconciliation requests are dropped when the SlaveID is not specified. We can answer reconciliation requests for a task so long as there are no transient slaves, this is what we should do when the slave id is not specified.
>
> Also, I realized that we should answer when a non-strict registry is in use, see the comment.
>
>
> Diffs
> -----
>
> src/master/master.hpp 7e7a75bd7e0fafc084ad2663c894e76e5fb35edd
> src/master/master.cpp dc60c47b9c08b1e83fd72b6b86393fdc11314ea1
> src/tests/reconciliation_tests.cpp 6edbf7563375a69e3148e74a8cd99ddd13fc445b
>
> Diff: https://reviews.apache.org/r/23542/diff/
>
>
> Testing
> -------
>
> Removed a test that is now invalid and added a test for pending tasks.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23542: Do not require the SlaveID in reconciliation
requests.
Posted by Vinod Kone <vi...@gmail.com>.
> On July 18, 2014, 6:38 p.m., Vinod Kone wrote:
> > src/tests/reconciliation_tests.cpp, lines 672-674
> > <https://reviews.apache.org/r/23542/diff/1/?file=633322#file633322line672>
> >
> > pull this below the offers expectation.
>
> Ben Mahler wrote:
> It is below the offers expectation, something I'm missing? Is this only applicable to this test?
I meant below EXPECT_NE(0u, offers.get().size());
There is no away the scheduler could get an update after driver.start() but before driver.launchTasks() right?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23542/#review48149
-----------------------------------------------------------
On July 21, 2014, 7:57 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23542/
> -----------------------------------------------------------
>
> (Updated July 21, 2014, 7:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-1525
> https://issues.apache.org/jira/browse/MESOS-1525
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I would recommend applying this patch to review the reconcileTasks() logic instead of using the diff viewer.
>
> Reconciliation requests currently specify a list of TaskStatuses. SlaveID is optional inside TaskStatus but reconciliation requests are dropped when the SlaveID is not specified. We can answer reconciliation requests for a task so long as there are no transient slaves, this is what we should do when the slave id is not specified.
>
> Also, I realized that we should answer when a non-strict registry is in use, see the comment.
>
>
> Diffs
> -----
>
> src/master/master.hpp 7e7a75bd7e0fafc084ad2663c894e76e5fb35edd
> src/master/master.cpp dc60c47b9c08b1e83fd72b6b86393fdc11314ea1
> src/tests/reconciliation_tests.cpp 6edbf7563375a69e3148e74a8cd99ddd13fc445b
>
> Diff: https://reviews.apache.org/r/23542/diff/
>
>
> Testing
> -------
>
> Removed a test that is now invalid and added a test for pending tasks.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23542: Do not require the SlaveID in reconciliation
requests.
Posted by Ben Mahler <be...@gmail.com>.
> On July 18, 2014, 6:38 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 500
> > <https://reviews.apache.org/r/23542/diff/1/?file=633320#file633320line500>
> >
> > pull "||" up to the end of the previous lines?
I was thinking about the readability of conditional wrapping recently from a thread here, where the GNU formatting uses this style:
https://news.ycombinator.com/item?id=7975386
https://www.gnu.org/prep/standards/html_node/Formatting.html
Example to consider:
if (recovered.contains(slaveId.get()) ||
reregistering.contains(slaveId.get()) ||
registered.contains(slaveId.get()) ||
removing.contains(slaveId.get()) {
if (recovered.contains(slaveId.get())
|| reregistering.contains(slaveId.get())
|| registered.contains(slaveId.get())
|| removing.contains(slaveId.get()) {
The alignment of the || operator across lines exposes the underlying structure more clearly. Less "jaggedness" as we used to talk about.
The google style guide refers to the former as being more common but allows both:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolean_Expressions#Boolean_Expressions
> On July 18, 2014, 6:38 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 3332
> > <https://reviews.apache.org/r/23542/diff/1/?file=633321#file633321line3332>
> >
> > I remember we talked about this. But sending a TASK_STAGING here might be better here for frameworks than ignoring? we could consider the act of validation/authorization and sending it to the slave all part of "staging" the task.
Ok, since it's orthogonal to this change and needs to be done for the implicit reconciliation as well, I will follow up separately:
https://issues.apache.org/jira/browse/MESOS-1620
> On July 18, 2014, 6:38 p.m., Vinod Kone wrote:
> > src/tests/reconciliation_tests.cpp, lines 672-674
> > <https://reviews.apache.org/r/23542/diff/1/?file=633322#file633322line672>
> >
> > pull this below the offers expectation.
It is below the offers expectation, something I'm missing? Is this only applicable to this test?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23542/#review48149
-----------------------------------------------------------
On July 16, 2014, 3:16 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23542/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 3:16 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-1525
> https://issues.apache.org/jira/browse/MESOS-1525
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I would recommend applying this patch to review the reconcileTasks() logic instead of using the diff viewer.
>
> Reconciliation requests currently specify a list of TaskStatuses. SlaveID is optional inside TaskStatus but reconciliation requests are dropped when the SlaveID is not specified. We can answer reconciliation requests for a task so long as there are no transient slaves, this is what we should do when the slave id is not specified.
>
> Also, I realized that we should answer when a non-strict registry is in use, see the comment.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20
> src/master/master.hpp 10bd95abc816c58fc2b72d3ce2d85128333cde39
> src/master/master.cpp 3ba8c334b38e16f072f59f13c718b2deb48a39f4
> src/tests/reconciliation_tests.cpp 6edbf7563375a69e3148e74a8cd99ddd13fc445b
>
> Diff: https://reviews.apache.org/r/23542/diff/
>
>
> Testing
> -------
>
> Removed a test that is now invalid and added a test for pending tasks.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23542: Do not require the SlaveID in reconciliation
requests.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23542/#review48149
-----------------------------------------------------------
Ship it!
src/master/master.hpp
<https://reviews.apache.org/r/23542/#comment84445>
pull "||" up to the end of the previous lines?
src/master/master.cpp
<https://reviews.apache.org/r/23542/#comment84446>
s/will/can/
src/master/master.cpp
<https://reviews.apache.org/r/23542/#comment84447>
pull this below the first if loop.
src/master/master.cpp
<https://reviews.apache.org/r/23542/#comment84448>
I remember we talked about this. But sending a TASK_STAGING here might be better here for frameworks than ignoring? we could consider the act of validation/authorization and sending it to the slave all part of "staging" the task.
src/tests/reconciliation_tests.cpp
<https://reviews.apache.org/r/23542/#comment84456>
pull this below the offers expectation.
src/tests/reconciliation_tests.cpp
<https://reviews.apache.org/r/23542/#comment84460>
s/,/;/
src/tests/reconciliation_tests.cpp
<https://reviews.apache.org/r/23542/#comment84461>
if you havent already, please make sure it run this test a few hundred times to avoid flakiness.
- Vinod Kone
On July 16, 2014, 3:16 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23542/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 3:16 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Vinod Kone.
>
>
> Bugs: MESOS-1525
> https://issues.apache.org/jira/browse/MESOS-1525
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> I would recommend applying this patch to review the reconcileTasks() logic instead of using the diff viewer.
>
> Reconciliation requests currently specify a list of TaskStatuses. SlaveID is optional inside TaskStatus but reconciliation requests are dropped when the SlaveID is not specified. We can answer reconciliation requests for a task so long as there are no transient slaves, this is what we should do when the slave id is not specified.
>
> Also, I realized that we should answer when a non-strict registry is in use, see the comment.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 12ff00a2e1262d785dbd6ec4d54e50d88d611e20
> src/master/master.hpp 10bd95abc816c58fc2b72d3ce2d85128333cde39
> src/master/master.cpp 3ba8c334b38e16f072f59f13c718b2deb48a39f4
> src/tests/reconciliation_tests.cpp 6edbf7563375a69e3148e74a8cd99ddd13fc445b
>
> Diff: https://reviews.apache.org/r/23542/diff/
>
>
> Testing
> -------
>
> Removed a test that is now invalid and added a test for pending tasks.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 23542: Do not require the SlaveID in reconciliation
requests.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23542/
-----------------------------------------------------------
(Updated July 21, 2014, 10:40 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Bugs: MESOS-1525
https://issues.apache.org/jira/browse/MESOS-1525
Repository: mesos-git
Description
-------
I would recommend applying this patch to review the reconcileTasks() logic instead of using the diff viewer.
Reconciliation requests currently specify a list of TaskStatuses. SlaveID is optional inside TaskStatus but reconciliation requests are dropped when the SlaveID is not specified. We can answer reconciliation requests for a task so long as there are no transient slaves, this is what we should do when the slave id is not specified.
Also, I realized that we should answer when a non-strict registry is in use, see the comment.
Diffs (updated)
-----
src/master/master.hpp 7e7a75bd7e0fafc084ad2663c894e76e5fb35edd
src/master/master.cpp dc60c47b9c08b1e83fd72b6b86393fdc11314ea1
src/tests/reconciliation_tests.cpp 6edbf7563375a69e3148e74a8cd99ddd13fc445b
Diff: https://reviews.apache.org/r/23542/diff/
Testing
-------
Removed a test that is now invalid and added a test for pending tasks.
Thanks,
Ben Mahler
Re: Review Request 23542: Do not require the SlaveID in reconciliation
requests.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23542/
-----------------------------------------------------------
(Updated July 21, 2014, 7:57 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Rebased and updated per comments.
Bugs: MESOS-1525
https://issues.apache.org/jira/browse/MESOS-1525
Repository: mesos-git
Description
-------
I would recommend applying this patch to review the reconcileTasks() logic instead of using the diff viewer.
Reconciliation requests currently specify a list of TaskStatuses. SlaveID is optional inside TaskStatus but reconciliation requests are dropped when the SlaveID is not specified. We can answer reconciliation requests for a task so long as there are no transient slaves, this is what we should do when the slave id is not specified.
Also, I realized that we should answer when a non-strict registry is in use, see the comment.
Diffs (updated)
-----
src/master/master.hpp 7e7a75bd7e0fafc084ad2663c894e76e5fb35edd
src/master/master.cpp dc60c47b9c08b1e83fd72b6b86393fdc11314ea1
src/tests/reconciliation_tests.cpp 6edbf7563375a69e3148e74a8cd99ddd13fc445b
Diff: https://reviews.apache.org/r/23542/diff/
Testing
-------
Removed a test that is now invalid and added a test for pending tasks.
Thanks,
Ben Mahler