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