mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 65111: Added tests to verify the executor is shutdown if no task is delivered.
Date Tue, 30 Jan 2018 00:49:17 GMT

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


Fix it, then Ship it!




Looks great! Just a couple of questions about simplifying the tests below.

Per offline discussion, looks like we need to disable the failover test on windows?


src/tests/slave_tests.cpp
Lines 4577-4578 (patched)
<https://reviews.apache.org/r/65111/#comment276149>

    Hm.. doesn't look like we need custom flags?



src/tests/slave_tests.cpp
Lines 4611-4613 (patched)
<https://reviews.apache.org/r/65111/#comment276152>

    Could you do offers->at(0)?



src/tests/slave_tests.cpp
Lines 4640 (patched)
<https://reviews.apache.org/r/65111/#comment276150>

    I was trying to find the kill task invocation and then realized it was up here. I think
our typical approach here would be to `FutureSatisfy` a `___run` future here and wait for
it below, then issue a kill task, then wait for the kill task update, etc.
    
    So:
    
    ```
    Future<Nothing> ___run;
    EXPECT_CALL(...)
      .WillOnce(DoAll(
         ...,
         FutureSatisfy(&___run));
    
    ...
    
    driver.launchTasks(...);
    
    AWAIT_READY(___run);
    
    driver.killTask(task.task_id());
    
    ...
    ```



src/tests/slave_tests.cpp
Lines 4659-4660 (patched)
<https://reviews.apache.org/r/65111/#comment276151>

    Our style guide allows the following:
    
    ```
    EXPECT_EQ(TaskStatus::REASON_TASK_KILLED_DURING_LAUNCH,
              killTaskStatus->reason());
    
    // Or:
    
    EXPECT_EQ(
        TaskStatus::REASON_TASK_KILLED_DURING_LAUNCH,
        killTaskStatus->reason());
    ```



src/tests/slave_tests.cpp
Lines 4679 (patched)
<https://reviews.apache.org/r/65111/#comment276158>

    Per offline discussion, looks like this one needs to be disabled on windows?



src/tests/slave_tests.cpp
Lines 4685-4686 (patched)
<https://reviews.apache.org/r/65111/#comment276153>

    Ditto here



src/tests/slave_tests.cpp
Lines 4688-4694 (patched)
<https://reviews.apache.org/r/65111/#comment276154>

    Why did you need to create your own mesos containerizer here? Ditto below



src/tests/slave_tests.cpp
Lines 4727-4728 (patched)
<https://reviews.apache.org/r/65111/#comment276155>

    Not used?



src/tests/slave_tests.cpp
Lines 4733 (patched)
<https://reviews.apache.org/r/65111/#comment276156>

    `s/taskRun/___run/`



src/tests/slave_tests.cpp
Lines 4758 (patched)
<https://reviews.apache.org/r/65111/#comment276157>

    executor


- Benjamin Mahler


On Jan. 26, 2018, 11:21 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65111/
> -----------------------------------------------------------
> 
> (Updated Jan. 26, 2018, 11:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-8411
>     https://issues.apache.org/jira/browse/MESOS-8411
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This test verifies that the executor is shutdown if all of its
> initial tasks could not be delivered, even after the executor has been
> registered. See MESOS-8411.
> 
> 
> Diffs
> -----
> 
>   src/tests/mesos.hpp a35c68e8645384f6244d17e37cad71373aba6893 
>   src/tests/mesos.cpp d751b2e9c635eb6a5039678de426467176cda908 
>   src/tests/slave_tests.cpp 20b874481d3818574731fc30ba9df1fc2bcbe900 
> 
> 
> Diff: https://reviews.apache.org/r/65111/diff/3/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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