mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 63577: Fixed a task status update race in default executor tests.
Date Thu, 16 Nov 2017 11:45:53 GMT

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


Fix it, then Ship it!





src/tests/default_executor_tests.cpp
Lines 303-305 (patched)
<https://reviews.apache.org/r/63577/#comment268782>

    Let's declare those later, together with `testing::Sequence` for task2.



src/tests/default_executor_tests.cpp
Lines 314-317 (original), 330-339 (patched)
<https://reviews.apache.org/r/63577/#comment268781>

    Indentation



src/tests/default_executor_tests.cpp
Lines 364-372 (patched)
<https://reviews.apache.org/r/63577/#comment268783>

    Indentation again. Could you please carefully check other places in the patch?



src/tests/default_executor_tests.cpp
Lines 434-442 (patched)
<https://reviews.apache.org/r/63577/#comment268780>

    Indentation.



src/tests/default_executor_tests.cpp
Lines 404-418 (original), 466-471 (patched)
<https://reviews.apache.org/r/63577/#comment268784>

    I'd combine these in one section:
    ```
    // Only tasks in the first group should be killed.
    AWAIT_READY(killedUpdate1);
    AWAIT_READY(killedUpdate2);
    ASSERT_TRUE(killedUpdate3.isPending());
    ```



src/tests/default_executor_tests.cpp
Lines 513-514 (original), 558-560 (patched)
<https://reviews.apache.org/r/63577/#comment268785>

    Again, please move these down right before setting `EXPECT_CALL` for task2.



src/tests/default_executor_tests.cpp
Lines 523-526 (original), 585-594 (patched)
<https://reviews.apache.org/r/63577/#comment268786>

    Indentation.



src/tests/default_executor_tests.cpp
Lines 619-629 (patched)
<https://reviews.apache.org/r/63577/#comment268787>

    Indentation.



src/tests/default_executor_tests.cpp
Lines 652-653 (original), 717-719 (patched)
<https://reviews.apache.org/r/63577/#comment268788>

    Ditto.



src/tests/default_executor_tests.cpp
Lines 744-753 (patched)
<https://reviews.apache.org/r/63577/#comment268789>

    Ditto.



src/tests/default_executor_tests.cpp
Lines 666-698 (original), 796-803 (patched)
<https://reviews.apache.org/r/63577/#comment268793>

    The sequence is a bit weird : ). A curious reader might ask themselves why you arrange
`AWAIT_`s this way. I'd suggest either doing
    ```
    AWAIT_READY(startingUpdate1);
    AWAIT_READY(runningUpdate1);
    AWAIT_READY(failedUpdate1);
    
    AWAIT_READY(startingUpdate2);
    AWAIT_READY(runningUpdate2);
    AWAIT_READY(killedUpdate2);
    ```
    or
    ```
    AWAIT_READY(startingUpdate1);
    AWAIT_READY(startingUpdate2);
    
    AWAIT_READY(runningUpdate1);
    AWAIT_READY(runningUpdate2);
    
    AWAIT_READY(failedUpdate1);
    AWAIT_READY(killedUpdate2);
    ```
    Or add a comment explaining why the sequence you suggest is preferable



src/tests/default_executor_tests.cpp
Lines 867-868 (original), 952-953 (patched)
<https://reviews.apache.org/r/63577/#comment268794>

    Ditto, move below please



src/tests/default_executor_tests.cpp
Lines 926-928 (original), 1027-1029 (patched)
<https://reviews.apache.org/r/63577/#comment268796>

    What happens if a scheduler gets `TASK_FAILED` status update before destruction? Will
the test fail with an "uninteresting mock function call"? Or there will be no failure because
the expectation are specific to the state and we just get a warning in the log? I believe
the latter, but could you please check this?



src/tests/default_executor_tests.cpp
Line 1093 (original), 1195-1196 (patched)
<https://reviews.apache.org/r/63577/#comment268797>

    Not sure you need this comment now, the code is self describing. If you want to keep it,
please add the same comment to all cases above : )



src/tests/default_executor_tests.cpp
Lines 1200-1202 (patched)
<https://reviews.apache.org/r/63577/#comment268798>

    Ditto, move closer to task2's expectations.



src/tests/mesos.hpp
Lines 2935-2941 (original), 2935-2941 (patched)
<https://reviews.apache.org/r/63577/#comment268801>

    This guy does the same as the one below, but for the old driver. Let's rename it to `TaskStatusStateEq`
for consistency (in a separate patch).
    
    Also, I'm not sure we need this long comment. Let's keep the first sentence only.
    
    And s/task/taskInfo



src/tests/mesos.hpp
Lines 2945 (patched)
<https://reviews.apache.org/r/63577/#comment268802>

    s/task/taskInfo



src/tests/mesos.hpp
Lines 2952 (patched)
<https://reviews.apache.org/r/63577/#comment268803>

    s/state/taskState


- Alexander Rukletsov


On Nov. 15, 2017, 6:21 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63577/
> -----------------------------------------------------------
> 
> (Updated Nov. 15, 2017, 6:21 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gaston Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously in the test `DefaultExecutorTest.KillMultipleTasks` and
> `DefaultExecutorTest.KillTaskGroupOnTaskFailure`, when launching a
> task group which has multiple tasks, we expected the scheduler will
> receive all the TASK_STARTING status updates before receiving any
> TASK_RUNNING status updates. However this is not guaranteed, e.g.,
> it is possible for the scheduler to receive TASK_RUNNING for the
> first task before receiving TASK_STARTING for the second task.
> 
> So in this patch, we used `Sequence` to guarantee the order of
> TASK_STARTING and TASK_RUNNING for each task but do not care about
> the order between tasks.
> 
> The following 3 tests have their own solutions to handle this issue,
> in this patch, I updated them to use the above solution.
>   `DefaultExecutorTest.KillTask`
>   `DefaultExecutorTest.CommitSuicideOnKillTask`
>   `DefaultExecutorTest.ROOT_ContainerStatusForTask`
> 
> 
> Diffs
> -----
> 
>   src/tests/default_executor_tests.cpp 65113985acc0a8fac00374b074ef568aaa22c7ac 
>   src/tests/mesos.hpp e25defeb55608136e77363aa48cf820092a13a59 
> 
> 
> Diff: https://reviews.apache.org/r/63577/diff/3/
> 
> 
> Testing
> -------
> 
> This patch touched 5 tests:
> 1. KillTask
> 2. KillMultipleTasks
> 3. KillTaskGroupOnTaskFailure
> 4. CommitSuicideOnKillTask
> 5. ROOT_ContainerStatusForTask
> 
> I ran each of them repeatedly (20 times), and all of them succeeded.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


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