Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

2017-01-03 Thread Anand Mazumdar 

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


Fix it, then Ship it!




Looks good minus a few minor issues. I would fix them while committing.


src/tests/api_tests.cpp (line 1494)


s/Ensure/Ensure that
to be consistent with the comment on L1523



src/tests/master_tests.cpp (lines 4145 - 4146)


Let's make it consistent with a similar comment in the earlier test. Also, 
a newline after the comment since it applies to both the following code blocks.

```cpp
// After the agent has successfully re-registered with the master, the 
`recovered_slaves` field would be empty in both `/state` and `/slave` endpoints.
```



src/tests/master_tests.cpp (line 4158)


s/parse1/parse



src/tests/master_tests.cpp (line 4180)


s/parse1/parse


- Anand Mazumdar


On Dec. 30, 2016, 11:05 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52639/
> ---
> 
> (Updated Dec. 30, 2016, 11:05 p.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 e3138699aa883919329aee47a6d93b5a9a9794b2 
>   src/tests/master_tests.cpp 2d0cd8244ded44e76f0eee3d87327ff526db5208 
> 
> Diff: https://reviews.apache.org/r/52639/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

2016-12-30 Thread Zhitao Li 

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

(Updated Dec. 30, 2016, 11:05 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.


Changes
---

Review comments.


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 (updated)
-

  src/tests/api_tests.cpp e3138699aa883919329aee47a6d93b5a9a9794b2 
  src/tests/master_tests.cpp 2d0cd8244ded44e76f0eee3d87327ff526db5208 

Diff: https://reviews.apache.org/r/52639/diff/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

2016-12-27 Thread Anand Mazumdar 

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


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)


Nit: Newline before.



src/tests/api_tests.cpp (line 1494)


s/Make sure/Ensure that
s/shown/present



src/tests/api_tests.cpp (line 1495)


s/recovered_agents/GetAgent.recovered_agents`



src/tests/api_tests.cpp (line 1523)


```
// Ensure that the agent is present in `GetAgent.recovered_agents` while 
`GetAgent.agents` is empty.
```



src/tests/api_tests.cpp (lines 1541 - 1581)


Kill this



src/tests/api_tests.cpp (line 1583)


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)


Kill this



src/tests/api_tests.cpp (line 1606)


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


Can we make this consistent with the comment in `GetRecoveredAgents`



src/tests/master_tests.cpp (line 4034)


Nit: s/StateSlavesEndpointsRecoveredSlaves/RecoveredSlaves



src/tests/master_tests.cpp (line 4038)


Nit: Newline before



src/tests/master_tests.cpp (lines 4064 - 4065)


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)


Newline before, since the comment applies to both code blocks.



src/tests/master_tests.cpp (line 4077)


s/parse1/parse



src/tests/master_tests.cpp (line 4078)


Nit: Newline before. Ditto for all the other occurences.



src/tests/master_tests.cpp (line 4084)


Nit: Newline before the multi line statement for readability.

Ditto for similar occurences later.



src/tests/master_tests.cpp (line 4099)


s/parse1/parse



src/tests/master_tests.cpp (line 4110)


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

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

2016-12-13 Thread Zhitao Li 


> On Dec. 13, 2016, 12:47 a.m., Anand Mazumdar wrote:
> > src/tests/api_tests.cpp, line 1493
> > 
> >
> > Can you do this outside of this scope as it's common to all subsequent 
> > blocks? It would then be consistent to all the other tests in this file.

I think I will do this in a separate sweeping patch instead of this one.


- Zhitao


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


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

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

2016-12-13 Thread Zhitao Li 

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


Changes
---

Split test as requested.


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 (updated)
-

  src/tests/api_tests.cpp 82c0fc27e5e707adb73faeb26828a2ce3e3feb16 
  src/tests/master_tests.cpp b3b5630899082a69119d2cccb6460766d1352963 

Diff: https://reviews.apache.org/r/52639/diff/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

2016-12-12 Thread Anand Mazumdar 

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



Did an initial pass on the test. Refer to my comments around splitting this 
test into two. Would take a detailed look again post the split.


src/tests/api_tests.cpp (line 1465)


hmm, let's make this test limited to testing if the `recovered` field is 
correctly propagated in the `GetAgents` response
- When the master/agent are started up.
- Terminate the agent and restart master (1 recovered agent, 0 active agent)
- Restart the agent now (0 recovered agent, 1 active agent)
This is exactly the same split as you already.

Let's create another test in `src/tests/master_tests.cpp` that does the 
same for `/slaves` and the `/state` endpoint.



src/tests/api_tests.cpp (line 1467)


Can you remove the `Step xxx` prefix? None of the other tests in this file 
have it.



src/tests/api_tests.cpp (line 1472)


Kill this comment and the one earlier around starting a master. They are 
self-explanatory.



src/tests/api_tests.cpp (line 1493)


Can you do this outside of this scope as it's common to all subsequent 
blocks? It would then be consistent to all the other tests in this file.


- Anand Mazumdar


On Nov. 1, 2016, 5:30 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52639/
> ---
> 
> (Updated Nov. 1, 2016, 5:30 p.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 f87130d7554f98265ff6da39ea1d366839193e85 
> 
> Diff: https://reviews.apache.org/r/52639/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

2016-11-01 Thread Zhitao Li 

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

(Updated Nov. 1, 2016, 5:30 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.


Changes
---

Added test for `/slaves` endpoint too.


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 (updated)
-

  src/tests/api_tests.cpp f87130d7554f98265ff6da39ea1d366839193e85 

Diff: https://reviews.apache.org/r/52639/diff/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

2016-10-21 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [53095, 52637, 52638, 52765, 52639]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Oct. 21, 2016, 6:41 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52639/
> ---
> 
> (Updated Oct. 21, 2016, 6:41 p.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 f87130d7554f98265ff6da39ea1d366839193e85 
> 
> Diff: https://reviews.apache.org/r/52639/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

2016-10-21 Thread Zhitao Li 

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

(Updated Oct. 21, 2016, 6:41 p.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 (updated)
-

  src/tests/api_tests.cpp f87130d7554f98265ff6da39ea1d366839193e85 

Diff: https://reviews.apache.org/r/52639/diff/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

2016-10-12 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [52637, 52638, 52765, 52639]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Oct. 12, 2016, 12:18 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52639/
> ---
> 
> (Updated Oct. 12, 2016, 12:18 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Xiaojian Huang.
> 
> 
> 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 39f3f6641cfb4b2e05cbaa1e8156e8a2f480f87a 
> 
> Diff: https://reviews.apache.org/r/52639/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

2016-10-11 Thread Zhitao Li 

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

(Updated Oct. 12, 2016, 12:18 a.m.)


Review request for mesos, Anand Mazumdar and Xiaojian Huang.


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 (updated)
-

  src/tests/api_tests.cpp 39f3f6641cfb4b2e05cbaa1e8156e8a2f480f87a 

Diff: https://reviews.apache.org/r/52639/diff/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

2016-10-07 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [52637, 52638, 52639]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Oct. 7, 2016, 4:03 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52639/
> ---
> 
> (Updated Oct. 7, 2016, 4:03 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Xiaojian Huang.
> 
> 
> 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 39f3f6641cfb4b2e05cbaa1e8156e8a2f480f87a 
> 
> Diff: https://reviews.apache.org/r/52639/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>