----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46491/#review131880 -----------------------------------------------------------
Can you move the introduction of 'terminated' into this patch? Currently it is introduced in the previous patch. src/docker/executor.cpp (lines 285 - 290) <https://reviews.apache.org/r/46491/#comment195918> 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 :( src/docker/executor.cpp (lines 292 - 294) <https://reviews.apache.org/r/46491/#comment195920> This is confusing to me, it just means a retry as far as this patch is concerned. Can you remove this TODO? src/docker/executor.cpp (line 296) <https://reviews.apache.org/r/46491/#comment195921> Can you update this per my suggestion in the other comment? I believe you meant it has not terminated yet, as opposed to "being asked to shut down". What does this mean? The executor being asked to shut down? ``` // Issue the kill signal if the container is running // and this is the first time we've received the kill. ``` src/launcher/executor.cpp (lines 509 - 521) <https://reviews.apache.org/r/46491/#comment195922> Ditto from above: can you move the terminated check into the condition and update the comment? src/launcher/executor.cpp (lines 635 - 639) <https://reviews.apache.org/r/46491/#comment195924> Hm.. I'm not sure this level of detail should be in the comment, I was thinking something like: ``` // Although we cancel the escalation timer when the process // is reaped, it may have already fired. Ignore it. if (terminated) { } ``` How about taking out the comment entirely? The reader that encounters this condition is likely not going to pause to consider why it's here, because clearly if the escalation occurs and the task is terminated we don't have to do anything. Why bother explaining the non-local timer subtlety here? ``` if (terminated) { return; } ``` src/launcher/http_command_executor.cpp (lines 597 - 609) <https://reviews.apache.org/r/46491/#comment195925> Ditto from above. src/launcher/http_command_executor.cpp (lines 712 - 717) <https://reviews.apache.org/r/46491/#comment195926> Ditto from above. - Ben Mahler On May 5, 2016, 3:37 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46491/ > ----------------------------------------------------------- > > (Updated May 5, 2016, 3:37 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 > ------- > > In command executor, capture the state when a task is killed (i.e., > reaped) in a flag and use this flag to prevent calls to `killTask()` > and `escalated()` when they are executed after the task is killed. > > > 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 > >
