----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52639/#review160203 -----------------------------------------------------------
Looks good minus a comment around simplifying the `GetRecoveredAgents` test. src/tests/api_tests.cpp (lines 1470 - 1472) <https://reviews.apache.org/r/52639/#comment231275> As per my review comment earlier, I would like this test to just ensure that the `recovered` field is correctly populated in the `GetAgents` response. I don't like the idea of this test also checking if the `AGENT_ADDED` event is _also_ received. That seems an orthogonal test not _strictly_ related to this change. Can you modify this test to just test for the `recovered` field instead? This would also make it consistent with the other test you added. Also, s/checks/verifies s/reregistered/reregister src/tests/api_tests.cpp (line 1477) <https://reviews.apache.org/r/52639/#comment231276> Nit: Newline before. src/tests/api_tests.cpp (line 1494) <https://reviews.apache.org/r/52639/#comment231278> s/Make sure/Ensure that s/shown/present src/tests/api_tests.cpp (line 1495) <https://reviews.apache.org/r/52639/#comment231277> s/recovered_agents/GetAgent.recovered_agents` src/tests/api_tests.cpp (line 1523) <https://reviews.apache.org/r/52639/#comment231279> ``` // Ensure that the agent is present in `GetAgent.recovered_agents` while `GetAgent.agents` is empty. ``` src/tests/api_tests.cpp (lines 1541 - 1581) <https://reviews.apache.org/r/52639/#comment231281> Kill this src/tests/api_tests.cpp (line 1583) <https://reviews.apache.org/r/52639/#comment231284> Kill this and instead add a comment before L1587 ``` // Start the agent to make it re-register with the master. ``` src/tests/api_tests.cpp (lines 1593 - 1604) <https://reviews.apache.org/r/52639/#comment231282> Kill this src/tests/api_tests.cpp (line 1606) <https://reviews.apache.org/r/52639/#comment231283> ``` After the agent has successfully re-registered with the master, the `recovered_agents` field of `GetAgents` would be empty. ``` src/tests/master_tests.cpp (lines 4032 - 4033) <https://reviews.apache.org/r/52639/#comment231287> Can we make this consistent with the comment in `GetRecoveredAgents` src/tests/master_tests.cpp (line 4034) <https://reviews.apache.org/r/52639/#comment231288> Nit: s/StateSlavesEndpointsRecoveredSlaves/RecoveredSlaves src/tests/master_tests.cpp (line 4038) <https://reviews.apache.org/r/52639/#comment231289> Nit: Newline before src/tests/master_tests.cpp (lines 4064 - 4065) <https://reviews.apache.org/r/52639/#comment231299> Modify this comment similar to what I had proposed for a similar comment in the earlier test. Ditto for the other occurence. src/tests/master_tests.cpp (line 4066) <https://reviews.apache.org/r/52639/#comment231290> Newline before, since the comment applies to both code blocks. src/tests/master_tests.cpp (line 4077) <https://reviews.apache.org/r/52639/#comment231291> s/parse1/parse src/tests/master_tests.cpp (line 4078) <https://reviews.apache.org/r/52639/#comment231293> Nit: Newline before. Ditto for all the other occurences. src/tests/master_tests.cpp (line 4084) <https://reviews.apache.org/r/52639/#comment231295> Nit: Newline before the multi line statement for readability. Ditto for similar occurences later. src/tests/master_tests.cpp (line 4099) <https://reviews.apache.org/r/52639/#comment231292> s/parse1/parse src/tests/master_tests.cpp (line 4110) <https://reviews.apache.org/r/52639/#comment231296> Kill this and add a comment similar to what I had proposed for the earlier test. - Anand Mazumdar On Dec. 14, 2016, 4:49 a.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52639/ > ----------------------------------------------------------- > > (Updated Dec. 14, 2016, 4:49 a.m.) > > > Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar. > > > Bugs: MESOS-6177 > https://issues.apache.org/jira/browse/MESOS-6177 > > > Repository: mesos > > > Description > ------- > > Added test for `recovered` AgentID and `AGENT_ADDED` after reregister. > > > Diffs > ----- > > src/tests/api_tests.cpp 82c0fc27e5e707adb73faeb26828a2ce3e3feb16 > src/tests/master_tests.cpp b3b5630899082a69119d2cccb6460766d1352963 > > Diff: https://reviews.apache.org/r/52639/diff/ > > > Testing > ------- > > > Thanks, > > Zhitao Li > >
