You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2016/04/18 14:44:11 UTC
Review Request 46323: Propagated KillPolicy in kill task from
scheduler to executor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46323/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-4908
https://issues.apache.org/jira/browse/MESOS-4908
Repository: mesos
Description
-------
Propagated KillPolicy in kill task from scheduler to executor.
Diffs
-----
src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1
src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af
src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3
src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b
src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2
Diff: https://reviews.apache.org/r/46323/diff/
Testing
-------
The whole chain is tested in https://reviews.apache.org/r/46325/
Thanks,
Alexander Rukletsov
Re: Review Request 46323: Propagated KillPolicy in kill task from
scheduler to executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 19, 2016, 1:09 a.m., Ben Mahler wrote:
> > src/internal/evolve.hpp, line 46
> > <https://reviews.apache.org/r/46323/diff/1/?file=1348360#file1348360line46>
> >
> > Why did you choose to inject it here? Seems better closer to TaskInfo?
Because in "mesos.proto" `KillPolicy` is defined between `FrameworkInfo` and `ExecutorInfo`. Do you think we should move it closer to `TaskInfo` in *both* "mesos.proto" and "evolve.hpp"?
> On April 19, 2016, 1:09 a.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 4060-4061
> > <https://reviews.apache.org/r/46323/diff/1/?file=1348362#file1348362line4060>
> >
> > Since it seems straightforward, why not just handle the field instead of introducing this NOTE? It seems nice to add the straightforward support here and decide on the old API later (if at all).
We can do it, but I don't understand the purpose (and I'm reluctant to add any code if there is no purpose). The scheduler driver uses `Call` message instead of internal `KillTaskMessage` since Mesos 0.24. The only use case that comes to my mind is pure bindings that do not use the scheduler driver and send `KillTaskMessage` directly. Do we want to add support for this case?
> On April 19, 2016, 1:09 a.m., Ben Mahler wrote:
> > src/slave/slave.hpp, line 149
> > <https://reviews.apache.org/r/46323/diff/1/?file=1348363#file1348363line149>
> >
> > Converting an optional field into an Option is not yet supported, unless I'm missing something?
> >
> > See: https://issues.apache.org/jira/browse/MESOS-2638
> >
> > Note that I added the current workaround to that ticket description, you can take the entire message in the handler to check the optionality.
Hm, I see. I've noticed a similar pattern for https://github.com/apache/mesos/blob/45d5fc379ff59c537ffc9ddfdf33c845af1e381f/src/slave/slave.cpp#L683 and thought we support that.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46323/#review129457
-----------------------------------------------------------
On April 18, 2016, 12:44 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46323/
> -----------------------------------------------------------
>
> (Updated April 18, 2016, 12:44 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1
> src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af
> src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3
> src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
> src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
> src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b
> src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
> src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2
>
> Diff: https://reviews.apache.org/r/46323/diff/
>
>
> Testing
> -------
>
> The whole chain is tested in https://reviews.apache.org/r/46325/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 46323: Propagated KillPolicy in kill task from
scheduler to executor.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46323/#review129457
-----------------------------------------------------------
Looks good, but held off on a ship it because there is a bug in the agent's message handler, see below.
src/internal/evolve.hpp (line 46)
<https://reviews.apache.org/r/46323/#comment192878>
Why did you choose to inject it here? Seems better closer to TaskInfo?
src/master/master.cpp (lines 4060 - 4061)
<https://reviews.apache.org/r/46323/#comment192888>
Since it seems straightforward, why not just handle the field instead of introducing this NOTE? It seems nice to add the straightforward support here and decide on the old API later (if at all).
src/slave/slave.hpp (line 149)
<https://reviews.apache.org/r/46323/#comment192881>
Converting an optional field into an Option is not yet supported, unless I'm missing something?
See: https://issues.apache.org/jira/browse/MESOS-2638
Note that I added the current workaround to that ticket description, you can take the entire message in the handler to check the optionality.
- Ben Mahler
On April 18, 2016, 12:44 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46323/
> -----------------------------------------------------------
>
> (Updated April 18, 2016, 12:44 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1
> src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af
> src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3
> src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
> src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
> src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b
> src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
> src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2
>
> Diff: https://reviews.apache.org/r/46323/diff/
>
>
> Testing
> -------
>
> The whole chain is tested in https://reviews.apache.org/r/46325/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 46323: Propagated KillPolicy in kill task from
scheduler to executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46323/
-----------------------------------------------------------
(Updated April 22, 2016, 2:38 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
BenM's comment.
Bugs: MESOS-4908
https://issues.apache.org/jira/browse/MESOS-4908
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1
src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af
src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3
src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5
src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526
src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b
src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
src/tests/slave_tests.cpp 3f653354869987dce3f5fbc4513b6f3466a718cb
Diff: https://reviews.apache.org/r/46323/diff/
Testing
-------
The whole chain is tested in https://reviews.apache.org/r/46325/
Thanks,
Alexander Rukletsov
Re: Review Request 46323: Propagated KillPolicy in kill task from
scheduler to executor.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46323/#review129936
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/slave.cpp (lines 2041 - 2042)
<https://reviews.apache.org/r/46323/#comment193488>
const & for both of these since the rhs is not a temporary
- Ben Mahler
On April 21, 2016, 2:28 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46323/
> -----------------------------------------------------------
>
> (Updated April 21, 2016, 2:28 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1
> src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af
> src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3
> src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
> src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
> src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b
> src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
> src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2
>
> Diff: https://reviews.apache.org/r/46323/diff/
>
>
> Testing
> -------
>
> The whole chain is tested in https://reviews.apache.org/r/46325/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 46323: Propagated KillPolicy in kill task from
scheduler to executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46323/
-----------------------------------------------------------
(Updated April 21, 2016, 2:28 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-4908
https://issues.apache.org/jira/browse/MESOS-4908
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1
src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af
src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3
src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b
src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2
Diff: https://reviews.apache.org/r/46323/diff/
Testing
-------
The whole chain is tested in https://reviews.apache.org/r/46325/
Thanks,
Alexander Rukletsov
Re: Review Request 46323: Propagated KillPolicy in kill task from
scheduler to executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46323/
-----------------------------------------------------------
(Updated April 21, 2016, 2:06 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-4908
https://issues.apache.org/jira/browse/MESOS-4908
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1
src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af
src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3
src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b
src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2
Diff: https://reviews.apache.org/r/46323/diff/
Testing
-------
The whole chain is tested in https://reviews.apache.org/r/46325/
Thanks,
Alexander Rukletsov
Re: Review Request 46323: Propagated KillPolicy in kill task from
scheduler to executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46323/
-----------------------------------------------------------
(Updated April 18, 2016, 12:44 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-4908
https://issues.apache.org/jira/browse/MESOS-4908
Repository: mesos
Description (updated)
-------
See summary.
Diffs
-----
src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1
src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af
src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3
src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1
src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b
src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb
src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2
Diff: https://reviews.apache.org/r/46323/diff/
Testing
-------
The whole chain is tested in https://reviews.apache.org/r/46325/
Thanks,
Alexander Rukletsov