mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ben Mahler <benjamin.mah...@gmail.com>
Subject Re: Review Request 46491: Ensured escalated() is not called after reaped() in command executor.
Date Thu, 05 May 2016 18:08:41 GMT

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


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