----------------------------------------------------------- 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 > >
