-----------------------------------------------------------
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
> 
>

Reply via email to