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:43:45 UTC
Review Request 46322: Added KillPolicy to scheduler and executor Kill
protobuf messages.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46322/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-4908
https://issues.apache.org/jira/browse/MESOS-4908
Repository: mesos
Description
-------
A framework may want to override the `KillPolicy` set in `TaskInfo`
when killing a task, for example to forcefully kill a task which is
already being killed.
Diffs
-----
CHANGELOG d2e902f8295644c527964123e409be460c2a5789
include/mesos/executor/executor.proto 338b3638f986244122c2d39c9aca7905c12008ce
include/mesos/scheduler/scheduler.proto 078c6550f24a3d8ac675251168434130fc3eeef3
include/mesos/v1/executor/executor.proto 4552fb5d3f9d53affd8fad0abf122fce548973b7
include/mesos/v1/scheduler/scheduler.proto 8ed9e19a9e5aa19a518b708b0e0d9cfdc038cd11
src/messages/messages.proto e0f1fca92d3ea8c29c095da31653c317873a934c
Diff: https://reviews.apache.org/r/46322/diff/
Testing
-------
The whole chain is tested in https://reviews.apache.org/r/46325/
Thanks,
Alexander Rukletsov
Re: Review Request 46322: Added KillPolicy to scheduler and executor
Kill protobuf messages.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 19, 2016, 12:33 a.m., Ben Mahler wrote:
> > CHANGELOG, lines 61-63
> > <https://reviews.apache.org/r/46322/diff/1/?file=1348354#file1348354line61>
> >
> > I was initially surprised to see MESOS-4908 repeated here, but I suppose the intent was to list all non-deprecation API changes here? If so, we're not doing that already (e.g. MESOS-4909 and MESOS-4949 in 0.29.0 for example).
> >
> > It seems the current approach is that 'new features' may include some API changes, but these aren't repeated in 'Additional API' changes. While it would be great to have a clearer approach, can you follow the existing approach?
Yes, that was the intent, but you right, it is inconsistent to what we do. Will drop these lines in favor of longer description above.
> On April 19, 2016, 12:33 a.m., Ben Mahler wrote:
> > CHANGELOG, line 62
> > <https://reviews.apache.org/r/46322/diff/1/?file=1348354#file1348354line62>
> >
> > no need for the "'s" here:
> >
> > s/scheduler's/scheduler/
> > s/executor's/executor/
Thanks! I'll drop this since I'll remove these lines altogether as per comment above.
> On April 19, 2016, 12:33 a.m., Ben Mahler wrote:
> > include/mesos/executor/executor.proto, lines 90-91
> > <https://reviews.apache.org/r/46322/diff/1/?file=1348355#file1348355line90>
> >
> > All of the .proto comments introduced here would benefit from also mentioning that the kill policy overrides any previously specified kill.kill_policy. Currently it only seems clear that the kill.kill_policy overrides task_info.kill_policy.
> >
> > Perhaps for now, we just explicitly state that the grace period may be "overridden" (or "adjusted"?) in order to give more or less time to a graceful kill that is in progress.
Good point, will adjust the comment.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46322/#review129454
-----------------------------------------------------------
On April 18, 2016, 12:43 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46322/
> -----------------------------------------------------------
>
> (Updated April 18, 2016, 12:43 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A framework may want to override the `KillPolicy` set in `TaskInfo`
> when killing a task, for example to forcefully kill a task which is
> already being killed.
>
>
> Diffs
> -----
>
> CHANGELOG d2e902f8295644c527964123e409be460c2a5789
> include/mesos/executor/executor.proto 338b3638f986244122c2d39c9aca7905c12008ce
> include/mesos/scheduler/scheduler.proto 078c6550f24a3d8ac675251168434130fc3eeef3
> include/mesos/v1/executor/executor.proto 4552fb5d3f9d53affd8fad0abf122fce548973b7
> include/mesos/v1/scheduler/scheduler.proto 8ed9e19a9e5aa19a518b708b0e0d9cfdc038cd11
> src/messages/messages.proto e0f1fca92d3ea8c29c095da31653c317873a934c
>
> Diff: https://reviews.apache.org/r/46322/diff/
>
>
> Testing
> -------
>
> The whole chain is tested in https://reviews.apache.org/r/46325/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 46322: Added KillPolicy to scheduler and executor
Kill protobuf messages.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46322/#review129454
-----------------------------------------------------------
Fix it, then Ship it!
CHANGELOG (line 16)
<https://reviews.apache.org/r/46322/#comment192872>
The task kill policy defined within TaskInfo can now be overridden when the scheduler kills the task. This can be used by schedulers to forcefully kill a task which is already being killed, e.g. if something went wrong during a graceful kill and a forcible kill is desired. Note that it is the executor's responsibility to honor the 'Event.kill.kill_policy' field and override the task's kill policy. To use this feature, schedulers and executors must support HTTP API; use the `--http_command_executor` agent flag to ensure the agent launches the HTTP API based command executor.
CHANGELOG (line 48)
<https://reviews.apache.org/r/46322/#comment192870>
whoops? please commit this separately. But notice that there are a number of inconsistent past vs. present tense wording in the CHANGELOG, poor grammar, etc. I'd rather see a sweep on a section (e.g. 0.29.0) to minimize history churn.
CHANGELOG (lines 61 - 63)
<https://reviews.apache.org/r/46322/#comment192868>
I was initially surprised to see MESOS-4908 repeated here, but I suppose the intent was to list all non-deprecation API changes here? If so, we're not doing that already (e.g. MESOS-4909 and MESOS-4949 in 0.29.0 for example).
It seems the current approach is that 'new features' may include some API changes, but these aren't repeated in 'Additional API' changes. While it would be great to have a clearer approach, can you follow the existing approach?
CHANGELOG (line 62)
<https://reviews.apache.org/r/46322/#comment192873>
no need for the "'s" here:
s/scheduler's/scheduler/
s/executor's/executor/
include/mesos/executor/executor.proto (lines 90 - 91)
<https://reviews.apache.org/r/46322/#comment192875>
All of the .proto comments introduced here would benefit from also mentioning that the kill policy overrides any previously specified kill.kill_policy. Currently it only seems clear that the kill.kill_policy overrides task_info.kill_policy.
Perhaps for now, we just explicitly state that the grace period may be "overridden" (or "adjusted"?) in order to give more or less time to a graceful kill that is in progress.
- Ben Mahler
On April 18, 2016, 12:43 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46322/
> -----------------------------------------------------------
>
> (Updated April 18, 2016, 12:43 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A framework may want to override the `KillPolicy` set in `TaskInfo`
> when killing a task, for example to forcefully kill a task which is
> already being killed.
>
>
> Diffs
> -----
>
> CHANGELOG d2e902f8295644c527964123e409be460c2a5789
> include/mesos/executor/executor.proto 338b3638f986244122c2d39c9aca7905c12008ce
> include/mesos/scheduler/scheduler.proto 078c6550f24a3d8ac675251168434130fc3eeef3
> include/mesos/v1/executor/executor.proto 4552fb5d3f9d53affd8fad0abf122fce548973b7
> include/mesos/v1/scheduler/scheduler.proto 8ed9e19a9e5aa19a518b708b0e0d9cfdc038cd11
> src/messages/messages.proto e0f1fca92d3ea8c29c095da31653c317873a934c
>
> Diff: https://reviews.apache.org/r/46322/diff/
>
>
> Testing
> -------
>
> The whole chain is tested in https://reviews.apache.org/r/46325/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 46322: Added KillPolicy to scheduler and executor
Kill protobuf messages.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46322/
-----------------------------------------------------------
(Updated April 22, 2016, 2:37 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Clarified note in CHANGELOG.
Bugs: MESOS-4908
https://issues.apache.org/jira/browse/MESOS-4908
Repository: mesos
Description
-------
A framework may want to override the `KillPolicy` set in `TaskInfo`
when killing a task, for example to forcefully kill a task which is
already being killed.
Diffs (updated)
-----
CHANGELOG d2e902f8295644c527964123e409be460c2a5789
include/mesos/executor/executor.proto 338b3638f986244122c2d39c9aca7905c12008ce
include/mesos/scheduler/scheduler.proto 078c6550f24a3d8ac675251168434130fc3eeef3
include/mesos/v1/executor/executor.proto 4552fb5d3f9d53affd8fad0abf122fce548973b7
include/mesos/v1/scheduler/scheduler.proto 8ed9e19a9e5aa19a518b708b0e0d9cfdc038cd11
src/messages/messages.proto e0f1fca92d3ea8c29c095da31653c317873a934c
Diff: https://reviews.apache.org/r/46322/diff/
Testing
-------
The whole chain is tested in https://reviews.apache.org/r/46325/
Thanks,
Alexander Rukletsov
Re: Review Request 46322: Added KillPolicy to scheduler and executor
Kill protobuf messages.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46322/
-----------------------------------------------------------
(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
-------
A framework may want to override the `KillPolicy` set in `TaskInfo`
when killing a task, for example to forcefully kill a task which is
already being killed.
Diffs (updated)
-----
CHANGELOG d2e902f8295644c527964123e409be460c2a5789
include/mesos/executor/executor.proto 338b3638f986244122c2d39c9aca7905c12008ce
include/mesos/scheduler/scheduler.proto 078c6550f24a3d8ac675251168434130fc3eeef3
include/mesos/v1/executor/executor.proto 4552fb5d3f9d53affd8fad0abf122fce548973b7
include/mesos/v1/scheduler/scheduler.proto 8ed9e19a9e5aa19a518b708b0e0d9cfdc038cd11
src/messages/messages.proto e0f1fca92d3ea8c29c095da31653c317873a934c
Diff: https://reviews.apache.org/r/46322/diff/
Testing
-------
The whole chain is tested in https://reviews.apache.org/r/46325/
Thanks,
Alexander Rukletsov
Re: Review Request 46322: Added KillPolicy to scheduler and executor
Kill protobuf messages.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46322/
-----------------------------------------------------------
(Updated April 21, 2016, 2:03 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-4908
https://issues.apache.org/jira/browse/MESOS-4908
Repository: mesos
Description
-------
A framework may want to override the `KillPolicy` set in `TaskInfo`
when killing a task, for example to forcefully kill a task which is
already being killed.
Diffs (updated)
-----
CHANGELOG d2e902f8295644c527964123e409be460c2a5789
include/mesos/executor/executor.proto 338b3638f986244122c2d39c9aca7905c12008ce
include/mesos/scheduler/scheduler.proto 078c6550f24a3d8ac675251168434130fc3eeef3
include/mesos/v1/executor/executor.proto 4552fb5d3f9d53affd8fad0abf122fce548973b7
include/mesos/v1/scheduler/scheduler.proto 8ed9e19a9e5aa19a518b708b0e0d9cfdc038cd11
src/messages/messages.proto e0f1fca92d3ea8c29c095da31653c317873a934c
Diff: https://reviews.apache.org/r/46322/diff/
Testing
-------
The whole chain is tested in https://reviews.apache.org/r/46325/
Thanks,
Alexander Rukletsov