----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56895/#review180319 -----------------------------------------------------------
Ship it! LGTM. Just a few minor style issues. src/tests/slave_recovery_tests.cpp Line 2526 (original), 2526 (patched) <https://reviews.apache.org/r/56895/#comment255430> Doesn't look like we need to set this. src/tests/slave_recovery_tests.cpp Line 2641 (original), 2643 (patched) <https://reviews.apache.org/r/56895/#comment255427> This could be shortened: `recoverState->slave.get()` src/tests/slave_recovery_tests.cpp Lines 2647 (patched) <https://reviews.apache.org/r/56895/#comment255425> Strictly speaking this is EXPECT_* Basically the rule is to prefer `EXPECT_*` and use `ASSERT_*` only when it doesn't make any sense to continue the test if this doesn't hold: https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#assertions Not all of these are a huge deal or it's always straighfoward what "doesn't make sense to continue" means but we should try to be idiomatic whenever possible. :) Note that there are some bad examples in this file. src/tests/slave_recovery_tests.cpp Line 2655 (original), 2661 (patched) <https://reviews.apache.org/r/56895/#comment255424> Strictly speaking this is EXPECT_* src/tests/slave_recovery_tests.cpp Lines 2667-2668 (patched) <https://reviews.apache.org/r/56895/#comment255423> There's `EXPECT_SOME_EQ` to do this in one line. src/tests/slave_recovery_tests.cpp Lines 2698 (patched) <https://reviews.apache.org/r/56895/#comment255431> Doesn't look like we need to set this. src/tests/slave_recovery_tests.cpp Lines 2765 (patched) <https://reviews.apache.org/r/56895/#comment255429> Strictly speaking this is EXPECT_* src/tests/slave_recovery_tests.cpp Lines 2779 (patched) <https://reviews.apache.org/r/56895/#comment255428> Strictly speaking this is EXPECT_* - Jiang Yan Xu On July 11, 2017, 5:13 p.m., Megha Sharma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56895/ > ----------------------------------------------------------- > > (Updated July 11, 2017, 5:13 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 f6eafcbe3d89c7a69c03db0fd7bc10ae73d06584 > > > Diff: https://reviews.apache.org/r/56895/diff/16/ > > > Testing > ------- > > make check done together with 60104 and 60103 > > > Thanks, > > Megha Sharma > >
