mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gaston Kleiman <gas...@mesosphere.io>
Subject Re: Review Request 63577: Fixed a task status update race in default executor tests.
Date Wed, 08 Nov 2017 21:07:28 GMT

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


Fix it, then Ship it!





src/tests/default_executor_tests.cpp
Lines 360 (patched)
<https://reviews.apache.org/r/63577/#comment267902>

    what happened here? extra ws? reviewboard doesn't sho anything useful =/



src/tests/default_executor_tests.cpp
Lines 391-395 (original), 410-414 (patched)
<https://reviews.apache.org/r/63577/#comment267905>

    I think this test would look cleaner if we used the new matchers for these updates.
    
    The test would then:
    
    1) Set all the expectations.
    2) Launch the tasks.
    3) Wait for the starting/running updates.
    4) Kill a task.
    5) Wait for the killed updates.



src/tests/default_executor_tests.cpp
Lines 414-418 (original), 433-439 (patched)
<https://reviews.apache.org/r/63577/#comment267907>

    Ditto, I'd use the matchers above instead of the hashsets here.
    
    If we decide to keep using the hashsets, I'd name them `taskIds` and `killedtasksIds`.



src/tests/default_executor_tests.cpp
Lines 612 (patched)
<https://reviews.apache.org/r/63577/#comment267909>

    Ditto using the new matchers instead of hashsets.



src/tests/default_executor_tests.cpp
Lines 691-718 (original), 747-768 (patched)
<https://reviews.apache.org/r/63577/#comment267910>

    We can use the matchers here as well.



src/tests/default_executor_tests.cpp
Lines 910-911 (original), 981-982 (patched)
<https://reviews.apache.org/r/63577/#comment267911>

    These two asserts are now redundant (the matcher checks exactly this).



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

    I'd call this one `TaskStatusUpdateTaskIdEq`.
    
    We'll soon also have offer operation status updates, so it will be necessary to disambiguate.



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

    Ditto: `TaskStatusUpdateStateEq`.


- Gaston Kleiman


On Nov. 6, 2017, 5:31 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63577/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2017, 5:31 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/1/
> 
> 
> 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