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



partial review.


src/master/master.hpp
Lines 867-871 (patched)
<https://reviews.apache.org/r/65449/#comment276422>

    s/toLaunchExecutor/launchExecutor/
    
    More importnatly, we don't typically use this pattern of passing a pointer 
and changing its value for `int` like fields. See below.



src/master/master.cpp
Line 4914 (original), 4918 (patched)
<https://reviews.apache.org/r/65449/#comment276423>

    How about we figure whether to launch an executor or not here and then call 
`addTask`?



src/master/master.cpp
Lines 4986 (patched)
<https://reviews.apache.org/r/65449/#comment276424>

    This comes out as `0 executor launch` or `1 executor launch` which reads 
weird.
    
    how about:
    
    ```
    LOG(INFO) << "Launching task " << task.task_id() << " of framework "
              << *framework << " with resources " << task.resources()
              << " on agent " << *slave << " on "
              << (message.launch_executor() ? " new executor" << " existing 
executor");
    ```



src/master/master.cpp
Line 5143 (original), 5151-5160 (patched)
<https://reviews.apache.org/r/65449/#comment276426>

    ditto. see above.
    
    do the check for executor launch outside the for loop.



src/master/master.cpp
Lines 5177-5179 (patched)
<https://reviews.apache.org/r/65449/#comment276427>

    see above. should be set outside the loop.



src/master/master.cpp
Lines 5193-5194 (patched)
<https://reviews.apache.org/r/65449/#comment276428>

    see above.



src/slave/slave.hpp
Lines 170 (patched)
<https://reviews.apache.org/r/65449/#comment276429>

    s/toLaunchExecutor/launchExecutor/



src/slave/slave.hpp
Lines 180 (patched)
<https://reviews.apache.org/r/65449/#comment276430>

    ditto. here and below.



src/slave/slave.cpp
Lines 1806 (patched)
<https://reviews.apache.org/r/65449/#comment276431>

    I think old masters didn't used to set this. If 1.x masters do it then we 
can do a CHECK.



src/slave/slave.cpp
Lines 1816 (patched)
<https://reviews.apache.org/r/65449/#comment276432>

    you can put fraemworkInfo in the previous line?



src/slave/slave.cpp
Lines 2065 (patched)
<https://reviews.apache.org/r/65449/#comment276433>

    do you need the temporary variable?


- Vinod Kone


On Feb. 1, 2018, 2:03 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65449/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2018, 2:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
> 
> 
> Bugs: MESOS-1720
>     https://issues.apache.org/jira/browse/MESOS-1720
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Master relies on `ExitedExecutorMessage` from the agent to recycle
> executor entry. However, this message won't be sent if the executor
> never actually launched (due to transient error), leaving executor
> info on the master lingering and resource claimed.
> See MESOS-1720.
> 
> This patch fixes this issue by sending the `ExitedExecutorMessage`
> from the agent if the executor is never launched. And by
> setting a new field `launch_executor` in the RunTask(Group)Message,
> the master is able to control the executor creation on the agent.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 
>   src/master/master.cpp b97ebae6ebfd8ae0f73e617d0c55e140b9c3fce7 
>   src/slave/slave.hpp 09c01ebd1b5e8008ba9e7d412042f1db76a2c5a5 
>   src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 
>   src/tests/mock_slave.hpp 29ce7140501888d95d5f2d6c26b752ad276b484a 
>   src/tests/mock_slave.cpp 8357edc7b3a35624c813eccb9ecca9d3b5dbe07c 
>   src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f 
> 
> 
> Diff: https://reviews.apache.org/r/65449/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> Dedicated test in #65448
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Reply via email to