You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2015/11/06 20:50:30 UTC
Review Request 40036: Added TaskStatus.container_status to
reconcialiation updates.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40036/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Repository: mesos
Description
-------
Along the lines of health checks, the container_status should also be sent
inside reconciliation updates.
Diffs
-----
src/common/protobuf_utils.hpp 44a2b1d3bab227294f5c5854acd87b7bd75ef589
src/common/protobuf_utils.cpp 1e795dcc90b2225ad7b0a124dbdcdabdc9d2e6aa
src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965
src/tests/master_tests.cpp 856440559de7d58be5bcf2ab6be911b5c67001cd
Diff: https://reviews.apache.org/r/40036/diff/
Testing
-------
`make check` with updated `MasterTest.TaskStatusContainerStatus` test.
Thanks,
Kapil Arya
Re: Review Request 40036: Added TaskStatus.container_status to
reconcialiation updates.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40036/#review105574
-----------------------------------------------------------
Bad patch!
Reviews applied: [40036]
Failed command: ./support/apply-review.sh -n -r 40036
Error:
2015-11-07 05:03:45 URL:https://reviews.apache.org/r/40036/diff/raw/ [6668/6668] -> "40036.patch" [1]
error: patch failed: src/tests/master_tests.cpp:3339
error: src/tests/master_tests.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On Nov. 7, 2015, 1:47 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40036/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2015, 1:47 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-3845
> https://issues.apache.org/jira/browse/MESOS-3845
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Along the lines of health checks, the container_status should also be sent
> inside reconciliation updates.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 44a2b1d3bab227294f5c5854acd87b7bd75ef589
> src/common/protobuf_utils.cpp b3c2d1183647ec1961f5d404355d08a5ce1d42a0
> src/master/master.cpp 25b94c8b41f65599327e0a0459ba86c6078895a8
> src/tests/master_tests.cpp 856440559de7d58be5bcf2ab6be911b5c67001cd
>
> Diff: https://reviews.apache.org/r/40036/diff/
>
>
> Testing
> -------
>
> `make check` with updated `MasterTest.TaskStatusContainerStatus` test.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 40036: Added TaskStatus.container_status to
reconcialiation updates.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40036/
-----------------------------------------------------------
(Updated Nov. 7, 2015, 10:04 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased
Bugs: MESOS-3845
https://issues.apache.org/jira/browse/MESOS-3845
Repository: mesos
Description
-------
Along the lines of health checks, the container_status should also be sent
inside reconciliation updates.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 44a2b1d3bab227294f5c5854acd87b7bd75ef589
src/common/protobuf_utils.cpp b3c2d1183647ec1961f5d404355d08a5ce1d42a0
src/master/master.cpp 25b94c8b41f65599327e0a0459ba86c6078895a8
src/tests/master_tests.cpp 856440559de7d58be5bcf2ab6be911b5c67001cd
Diff: https://reviews.apache.org/r/40036/diff/
Testing
-------
`make check` with updated `MasterTest.TaskStatusContainerStatus` test.
Thanks,
Kapil Arya
Re: Review Request 40036: Added TaskStatus.container_status to
reconcialiation updates.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40036/
-----------------------------------------------------------
(Updated Nov. 6, 2015, 8:47 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Addressed Ben's comments.
Bugs: MESOS-3845
https://issues.apache.org/jira/browse/MESOS-3845
Repository: mesos
Description
-------
Along the lines of health checks, the container_status should also be sent
inside reconciliation updates.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 44a2b1d3bab227294f5c5854acd87b7bd75ef589
src/common/protobuf_utils.cpp b3c2d1183647ec1961f5d404355d08a5ce1d42a0
src/master/master.cpp 25b94c8b41f65599327e0a0459ba86c6078895a8
src/tests/master_tests.cpp 856440559de7d58be5bcf2ab6be911b5c67001cd
Diff: https://reviews.apache.org/r/40036/diff/
Testing
-------
`make check` with updated `MasterTest.TaskStatusContainerStatus` test.
Thanks,
Kapil Arya
Re: Review Request 40036: Added TaskStatus.container_status to
reconcialiation updates.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Nov. 6, 2015, 5:23 p.m., Ben Mahler wrote:
> > src/common/protobuf_utils.cpp, lines 178-187
> > <https://reviews.apache.org/r/40036/diff/1/?file=1118429#file1118429line178>
> >
> > You don't need the 'if' with the way you've structured your loop, I assume it's just here from copy paste?
Yup, thanks for catching this.
> On Nov. 6, 2015, 5:23 p.m., Ben Mahler wrote:
> > src/common/protobuf_utils.cpp, line 182
> > <https://reviews.apache.org/r/40036/diff/1/?file=1118429#file1118429line182>
> >
> > Consider:
> >
> > ```
> > for (auto status = task.statuses().rbegin(); status != task.statuses().rend(); ++status)
> > ```
> >
> > Ideally we could do:
> >
> > ```
> > foreach (const TaskStatus& status, reverse(task.statuses()))
> > ```
> >
> > If you want to avoid the indexing math.
I chose the former ... looks like `foreach (..., reverse())` isn't used anywhere in the codebase (even `reverse()` alone isn't being used) and so wasn't sure if I should do it. (It would have also required `#include <algorithm>`). I wouldn't mind changing it though. Any preferences?
> On Nov. 6, 2015, 5:23 p.m., Ben Mahler wrote:
> > src/tests/master_tests.cpp, line 3371
> > <https://reviews.apache.org/r/40036/diff/1/?file=1118431#file1118431line3371>
> >
> > The convention here would be `status2` or something like `reconciliationStatus`.
I changed it to `explicitReconciliationStatus` to make it more explicit. Same for implicit.
> On Nov. 6, 2015, 5:23 p.m., Ben Mahler wrote:
> > src/tests/master_tests.cpp, lines 3375-3379
> > <https://reviews.apache.org/r/40036/diff/1/?file=1118431#file1118431line3375>
> >
> > You can avoid the vector here by passing in an initializer list into your reconcileTasks call.
Fixed.
> On Nov. 6, 2015, 5:23 p.m., Ben Mahler wrote:
> > src/common/protobuf_utils.cpp, line 190
> > <https://reviews.apache.org/r/40036/diff/1/?file=1118429#file1118429line190>
> >
> > two newlines here :)
Fixed.
> On Nov. 6, 2015, 5:23 p.m., Ben Mahler wrote:
> > src/tests/master_tests.cpp, line 3396
> > <https://reviews.apache.org/r/40036/diff/1/?file=1118431#file1118431line3396>
> >
> > Ditto here.
Fixed.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40036/#review105513
-----------------------------------------------------------
On Nov. 6, 2015, 8:47 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40036/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2015, 8:47 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-3845
> https://issues.apache.org/jira/browse/MESOS-3845
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Along the lines of health checks, the container_status should also be sent
> inside reconciliation updates.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 44a2b1d3bab227294f5c5854acd87b7bd75ef589
> src/common/protobuf_utils.cpp b3c2d1183647ec1961f5d404355d08a5ce1d42a0
> src/master/master.cpp 25b94c8b41f65599327e0a0459ba86c6078895a8
> src/tests/master_tests.cpp 856440559de7d58be5bcf2ab6be911b5c67001cd
>
> Diff: https://reviews.apache.org/r/40036/diff/
>
>
> Testing
> -------
>
> `make check` with updated `MasterTest.TaskStatusContainerStatus` test.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 40036: Added TaskStatus.container_status to
reconcialiation updates.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40036/#review105513
-----------------------------------------------------------
Ship it!
Thanks for the test!
src/common/protobuf_utils.cpp (lines 178 - 187)
<https://reviews.apache.org/r/40036/#comment164130>
You don't need the 'if' with the way you've structured your loop, I assume it's just here from copy paste?
src/common/protobuf_utils.cpp (line 182)
<https://reviews.apache.org/r/40036/#comment164131>
Consider:
```
for (auto status = task.statuses().rbegin(); status != task.statuses().rend(); ++status)
```
Ideally we could do:
```
foreach (const TaskStatus& status, reverse(task.statuses()))
```
If you want to avoid the indexing math.
src/common/protobuf_utils.cpp (line 190)
<https://reviews.apache.org/r/40036/#comment164129>
two newlines here :)
src/tests/master_tests.cpp (line 3369)
<https://reviews.apache.org/r/40036/#comment164134>
The convention here would be `status2` or something like `reconciliationStatus`.
src/tests/master_tests.cpp (lines 3373 - 3377)
<https://reviews.apache.org/r/40036/#comment164136>
You can avoid the vector here by passing in an initializer list into your reconcileTasks call.
src/tests/master_tests.cpp (line 3386)
<https://reviews.apache.org/r/40036/#comment164137>
We now have the `->` operator for Future, could you do a sweep in the code you've introduced to replace `.get().` with `->`? Thanks!!
src/tests/master_tests.cpp (line 3394)
<https://reviews.apache.org/r/40036/#comment164135>
Ditto here.
- Ben Mahler
On Nov. 6, 2015, 7:53 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40036/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2015, 7:53 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-3845
> https://issues.apache.org/jira/browse/MESOS-3845
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Along the lines of health checks, the container_status should also be sent
> inside reconciliation updates.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 44a2b1d3bab227294f5c5854acd87b7bd75ef589
> src/common/protobuf_utils.cpp 1e795dcc90b2225ad7b0a124dbdcdabdc9d2e6aa
> src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965
> src/tests/master_tests.cpp 856440559de7d58be5bcf2ab6be911b5c67001cd
>
> Diff: https://reviews.apache.org/r/40036/diff/
>
>
> Testing
> -------
>
> `make check` with updated `MasterTest.TaskStatusContainerStatus` test.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 40036: Added TaskStatus.container_status to
reconcialiation updates.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40036/
-----------------------------------------------------------
(Updated Nov. 6, 2015, 2:53 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-3845
https://issues.apache.org/jira/browse/MESOS-3845
Repository: mesos
Description
-------
Along the lines of health checks, the container_status should also be sent
inside reconciliation updates.
Diffs
-----
src/common/protobuf_utils.hpp 44a2b1d3bab227294f5c5854acd87b7bd75ef589
src/common/protobuf_utils.cpp 1e795dcc90b2225ad7b0a124dbdcdabdc9d2e6aa
src/master/master.cpp cbae27e7a4059a72bc69e152ec8adaf4ef725965
src/tests/master_tests.cpp 856440559de7d58be5bcf2ab6be911b5c67001cd
Diff: https://reviews.apache.org/r/40036/diff/
Testing
-------
`make check` with updated `MasterTest.TaskStatusContainerStatus` test.
Thanks,
Kapil Arya