mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.
Date Sat, 07 May 2016 12:55:18 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message