> On May 5, 2016, 6:08 p.m., Ben Mahler wrote:
> > src/docker/executor.cpp, lines 285-290
> > <https://reviews.apache.org/r/46491/diff/4/?file=1373528#file1373528line285>
> >
> >     No need for the note here. Can you also move this down into the 
> > conditional that gates it below?
> >     
> >     ```
> >     // Issue the kill signal if the container is running
> >     // and this is the first time we've received the kill.
> >     if (run.isSome() && !terminated && !killing) {
> >     ```
> >     
> >     Why are we issuing a kill to the health pid every time? It seems like 
> > it also needs to be guarded by this condition :(

The reason I've added this note (and also the note above) is to provide some 
context and thought for a reader, especially for someone who is going to 
implement kill policy override in the docker executor. The note below tries to 
explain that there is actually one more state (being terminated but not yet 
terminated) that we ignore for now but may (and should!) add in the future.

Or is your proposal to move this comment into a separate patch?


> On May 5, 2016, 6:08 p.m., Ben Mahler wrote:
> > src/docker/executor.cpp, lines 292-294
> > <https://reviews.apache.org/r/46491/diff/4/?file=1373528#file1373528line292>
> >
> >     This is confusing to me, it just means a retry as far as this patch is 
> > concerned. Can you remove this TODO?

See above.


> On May 5, 2016, 6:08 p.m., Ben Mahler wrote:
> > src/launcher/executor.cpp, lines 509-521
> > <https://reviews.apache.org/r/46491/diff/4/?file=1373529#file1373529line509>
> >
> >     Ditto from above: can you move the terminated check into the condition 
> > and update the comment?

See above.


> On May 5, 2016, 6:08 p.m., Ben Mahler wrote:
> > src/launcher/http_command_executor.cpp, lines 597-609
> > <https://reviews.apache.org/r/46491/diff/4/?file=1373530#file1373530line597>
> >
> >     Ditto from above.

See above.


- Alexander


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46491/#review131880
-----------------------------------------------------------


On May 7, 2016, 12:50 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46491/
> -----------------------------------------------------------
> 
> (Updated May 7, 2016, 12:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Ben Mahler.
> 
> 
> Bugs: MESOS-5240
>     https://issues.apache.org/jira/browse/MESOS-5240
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Capture the state when a task has terminated (i.e., reaped) in a flag,
> and use it to prevent calls to `killTask()` and `escalated()` when
> they are executed after the task has terminated.
> 
> 
> Diffs
> -----
> 
>   src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
>   src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
>   src/launcher/http_command_executor.cpp 
> d2f15b0447d91f3a4cd92f07114cb366647cc7d3 
> 
> Diff: https://reviews.apache.org/r/46491/diff/
> 
> 
> Testing
> -------
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Reply via email to