mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 56895: Added tests to ensure slave recovery post reboot.
Date Fri, 23 Jun 2017 23:59:50 GMT

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




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

    Why the change? Doesn't look right.



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

    Why the change? Doesn't look right.
    
    ASSERT_EQ is more appropriate since the next line would be invalid if the condition doesn't
hold.



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

    s/postRebootContainerId/containerId/ No need to make such distinction?



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

    OK I see that this is changed in the few version. I'll comment there.



src/tests/slave_recovery_tests.cpp
Lines 2699-2700 (original), 2700-2701 (patched)
<https://reviews.apache.org/r/56895/#comment253046>

    s/No state recovery/No slave state recovery/
    
    But you didn't really directly verify "No slave state recovery" right? 
    
    I think this test just verified that the agent correctly restarted as a new agent? (which
is sufficient IMO).



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

    Mention "reboot"?
    
    Perhaps
    
    s/RecoveryWithSlaveInfoMismatch/RebootWithSlaveInfoMismatch/



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

    One space before comments. 
    
    There are some inconsistent examples in this file which I have fixed now so they don't
get copied over.



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

    The resources being unreserved is not worth noting here?
    
    Perhaps drop the comment altogether? It's pretty clear what the code is doing here.



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

    s/registerSlave/registerSlaveMessage/
    
    Some of the examples in the file are very old and don't reflect our current conventions.
:)



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

    s/Changing/Change/ so the tense is more consistent.



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

    Not "same flags" right? :)



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

    This expectation doesn't provide us much? We know it's recovered through `registerSlaveMessage`
and additionally we know it's register instead of rereigster right?



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

    Blank line and is the comment necessary?
    
    Instead, you could comment on that you are capturing `offers2` in order to get the `slaveId2`
(which is what you reall want to verify)?
    
    The same applies to `offers1`.



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

    This is not used anywhere?


- Jiang Yan Xu


On June 23, 2017, 2:45 p.m., Megha Sharma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56895/
> -----------------------------------------------------------
> 
> (Updated June 23, 2017, 2:45 p.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 90c172b6f7ca0082c3dfa2082050d6c18e4e636c 
> 
> 
> Diff: https://reviews.apache.org/r/56895/diff/12/
> 
> 
> Testing
> -------
> 
> make check done together with 60104 and 60103
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>


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