mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod Kone" <vinodk...@gmail.com>
Subject Re: Review Request 22313: MESOS-886: Prevented slave from launching tasks before containerize's update completes.
Date Tue, 17 Jun 2014 00:28:32 GMT

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



src/slave/slave.cpp
<https://reviews.apache.org/r/22313/#comment80887>

    The slave cannot be in RECOVERING. See the comment on line #1099.



src/slave/slave.cpp
<https://reviews.apache.org/r/22313/#comment80889>

    Kill state == RECOVERING.



src/slave/slave.cpp
<https://reviews.apache.org/r/22313/#comment80912>

    Is this possible? AFAICT, since the task was added to the executor the executor shouldn't
be removed between _runTask() and __runTask(). Even if the executor terminates in between,
this task should've been marked 'terminated' but not 'completed' (i.e., waiting for an ACK)
and hence the executor won't be removed from the framework's map. Since there is a pending
executor, the framework shouldn't be removed.
    
    So this can be a CHECK_NONTULL(framework) with a comment on why it can be a check.



src/slave/slave.cpp
<https://reviews.apache.org/r/22313/#comment80917>

    See my comments above. An executor cannot be removed when it has a pending task.



src/slave/slave.cpp
<https://reviews.apache.org/r/22313/#comment80919>

    also log the executor id, task id and framework id.



src/slave/slave.cpp
<https://reviews.apache.org/r/22313/#comment80920>

    include the "(future.isFailed() ? future.failure() : "discarded")" here. though, "discarded"
here might not be meaningful for frameworks. hmmm.



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/22313/#comment80930>

    I don't see where this test is ensuring that resources are isolated before launching tasks?
    
    I would recommend splitting this into 2 tests
    
    1) ensure resources are updated before task is launched
    
    2) containerizer failed causes task lost to be sent.
    
    i would also recommend writing a test that ensures that framework and executor are not
removed between _runTask and __runTask().



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/22313/#comment80928>

    You should set these expectations just before you launch the second task. Also, why "WillRepeatedly"?
Are you expecting more than 2 updates?


- Vinod Kone


On June 11, 2014, 9:32 a.m., Yifan Gu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22313/
> -----------------------------------------------------------
> 
> (Updated June 11, 2014, 9:32 a.m.)
> 
> 
> Review request for mesos, Ian Downes and Vinod Kone.
> 
> 
> Bugs: MESOS-886
>     https://issues.apache.org/jira/browse/MESOS-886
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Added __runTask() to wait for the completion of containerizer->update() and check
the result before sending RunTaskMessage.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp 34687e5 
>   src/slave/slave.cpp 643c088 
>   src/tests/slave_tests.cpp 2c8f183 
> 
> Diff: https://reviews.apache.org/r/22313/diff/
> 
> 
> Testing
> -------
> 
> SlaveTest, CancelTaskIfContainerizerFails
> 
> Which tests that if the containerizer->update() return a Failure, the task won't be
launched and the scheduler will get TASK_LOST.
> 
> make check
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>


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