Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Jiang Yan Xu <ya...@jxu.me>]

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


Fix it, then Ship it!





src/tests/slave_recovery_tests.cpp
Line 2650 (original), 2656 (patched)
<https://reviews.apache.org/r/56895/#comment254722>

    Since it's the same agent ID, this comment is not relevant anymore?
    
    I'd argue that the whole section
    
    ```
    JSON::Object stats = Metrics();
      EXPECT_EQ(0, stats.values["master/tasks_lost"]);
      EXPECT_EQ(0, stats.values["master/slave_unreachable_scheduled"]);
      EXPECT_EQ(0, stats.values["master/slave_unreachable_completed"]);
      EXPECT_EQ(0, stats.values["master/slave_removals"]);
      EXPECT_EQ(0, stats.values["master/slave_removals/reason_registered"]);
    ```
    
    is not necessary, since you verified `EXPECT_EQ(slaveId1, slaveId2);`.



src/tests/slave_recovery_tests.cpp
Line 2650 (original), 2656 (patched)
<https://reviews.apache.org/r/56895/#comment254723>

    Now that the agent ID doesn't change, this comment is no longer relevant?



src/tests/slave_recovery_tests.cpp
Lines 2670 (patched)
<https://reviews.apache.org/r/56895/#comment254744>

    This line already started to check the executor so the comment above applies to just that one line. 
    
    This comment and the ones below seem a bit redundant.



src/tests/slave_recovery_tests.cpp
Lines 2676 (patched)
<https://reviews.apache.org/r/56895/#comment254737>

    Instead of `containerId` and `_containerId`, perhaps name them `containerId1` and `containerId2` and check that they are the same? (Like how you check  `slaveId1` and `slaveId2` are the same)?



src/tests/slave_recovery_tests.cpp
Lines 2688-2690 (patched)
<https://reviews.apache.org/r/56895/#comment254721>

    This kind of indentation is a bit tricky but let's follow the convention used in this file.
    
    ```
      EXPECT_TRUE(executorState
                    .runs[_containerId.get()]
                    .tasks.contains(task.task_id()));
    ```



src/tests/slave_recovery_tests.cpp
Lines 2697 (patched)
<https://reviews.apache.org/r/56895/#comment254733>

    s/slave/agent/ since this is a new test, let's use new terminology.



src/tests/slave_recovery_tests.cpp
Lines 2699 (patched)
<https://reviews.apache.org/r/56895/#comment254734>

    s/slaveInfo/agent info/
    
    Basically, use "agent info" to refer to this concept and not the variable or type. This particular casing of `slaveInfo` doesn't refer to anything: the type is `SlaveInfo` and there's not a variable `slaveInfo` here.



src/tests/slave_recovery_tests.cpp
Lines 2732 (patched)
<https://reviews.apache.org/r/56895/#comment254746>

    s/slaveId/agent ID/.
    
    `ID` being capital cases or not isn't a big deal but it seems this test already uses `ID` in many place so might as well make it consistent.



src/tests/slave_recovery_tests.cpp
Lines 2757 (patched)
<https://reviews.apache.org/r/56895/#comment254735>

    s/checkInfo/slave info/



src/tests/slave_recovery_tests.cpp
Lines 2768 (patched)
<https://reviews.apache.org/r/56895/#comment254748>

    s/slaveId/agent ID/



src/tests/slave_recovery_tests.cpp
Lines 2789 (patched)
<https://reviews.apache.org/r/56895/#comment254736>

    It's pretty clear what this line is doing so perhaps no need for this comment.


- Jiang Yan Xu


On June 30, 2017, 8:18 a.m., Megha Sharma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56895/
> -----------------------------------------------------------
> 
> (Updated June 30, 2017, 8:18 a.m.)
> 
> 
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6223
>     https://issues.apache.org/jira/browse/MESOS-6223
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests to verify that the state is recovered post reboot and
> agentId is retained given the recovery finishes without errors and
> if the recovery fails due to slaveInfo mismatch then agent no
> state is recoverd making the agent register as a new agent.
> 
> 
> Diffs
> -----
> 
>   src/tests/slave_recovery_tests.cpp f6eafcbe3d89c7a69c03db0fd7bc10ae73d06584 
> 
> 
> Diff: https://reviews.apache.org/r/56895/diff/13/
> 
> 
> Testing
> -------
> 
> make check done together with 60104 and 60103
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>