Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60103, 60104, 60105, 56895]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 13, 2017, 4:22 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56895/
> ---
> 
> (Updated July 13, 2017, 4:22 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/17/
> 
> 
> Testing
> ---
> 
> make check done together with 60104 and 60103
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

> On July 12, 2017, 5:47 p.m., Jiang Yan Xu wrote:
> > LGTM. Just a few minor style issues.

Thanks, addressed the comments.


- Megha


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


On July 13, 2017, 4:22 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56895/
> ---
> 
> (Updated July 13, 2017, 4:22 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/17/
> 
> 
> Testing
> ---
> 
> make check done together with 60104 and 60103
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

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

(Updated July 13, 2017, 4:22 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 (updated)
-

  src/tests/slave_recovery_tests.cpp f6eafcbe3d89c7a69c03db0fd7bc10ae73d06584 


Diff: https://reviews.apache.org/r/56895/diff/17/

Changes: https://reviews.apache.org/r/56895/diff/16-17/


Testing
---

make check done together with 60104 and 60103


Thanks,

Megha Sharma

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Jiang Yan Xu]

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


Doesn't look like we need to set this.



src/tests/slave_recovery_tests.cpp
Line 2641 (original), 2643 (patched)


This could be shortened: `recoverState->slave.get()`



src/tests/slave_recovery_tests.cpp
Lines 2647 (patched)


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)


Strictly speaking this is EXPECT_*



src/tests/slave_recovery_tests.cpp
Lines 2667-2668 (patched)


There's `EXPECT_SOME_EQ` to do this in one line.



src/tests/slave_recovery_tests.cpp
Lines 2698 (patched)


Doesn't look like we need to set this.



src/tests/slave_recovery_tests.cpp
Lines 2765 (patched)


Strictly speaking this is EXPECT_*



src/tests/slave_recovery_tests.cpp
Lines 2779 (patched)


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

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60103, 60104, 60105, 56895]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 12, 2017, 12:13 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56895/
> ---
> 
> (Updated July 12, 2017, 12:13 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/16/
> 
> 
> Testing
> ---
> 
> make check done together with 60104 and 60103
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

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

(Updated July 12, 2017, 12:13 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 (updated)
-

  src/tests/slave_recovery_tests.cpp f6eafcbe3d89c7a69c03db0fd7bc10ae73d06584 


Diff: https://reviews.apache.org/r/56895/diff/16/

Changes: https://reviews.apache.org/r/56895/diff/15-16/


Testing
---

make check done together with 60104 and 60103


Thanks,

Megha Sharma

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

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

(Updated July 11, 2017, 10:32 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 (updated)
-

  src/tests/slave_recovery_tests.cpp f6eafcbe3d89c7a69c03db0fd7bc10ae73d06584 


Diff: https://reviews.apache.org/r/56895/diff/15/

Changes: https://reviews.apache.org/r/56895/diff/14-15/


Testing
---

make check done together with 60104 and 60103


Thanks,

Megha Sharma

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

> On July 11, 2017, 5:53 p.m., Jiang Yan Xu wrote:
> > src/tests/slave_recovery_tests.cpp
> > Lines 2762 (patched)
> > 
> >
> > This is unintentional?

My bad, will fix.


> On July 11, 2017, 5:53 p.m., Jiang Yan Xu wrote:
> > src/tests/slave_recovery_tests.cpp
> > Line 2652 (original)
> > 
> >
> > In the case of real reboot with slave info mismatch I think we should 
> > receive a task lost for this so why not set the expetation explicitly 
> > instead of removing it?

Dicussed in person.


> On July 11, 2017, 5:53 p.m., Jiang Yan Xu wrote:
> > src/tests/slave_recovery_tests.cpp
> > Line 2655 (original), 2786 (patched)
> > 
> >
> > Are there no `master/slave_removals` here? Seems like MESOS-5396 
> > doens't apply here.

Discussed in person, removed.


- Megha


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


On July 10, 2017, 11:15 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56895/
> ---
> 
> (Updated July 10, 2017, 11:15 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/14/
> 
> 
> Testing
> ---
> 
> make check done together with 60104 and 60103
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Jiang Yan Xu]

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




src/tests/slave_recovery_tests.cpp
Line 2757 (original), 2747 (patched)


s/slave Info/agent info/ since is this the terminology used (consistently 
elsewhere) in this test.



src/tests/slave_recovery_tests.cpp
Lines 2762 (patched)


This is unintentional?



src/tests/slave_recovery_tests.cpp
Lines 2762 (patched)


This line redundant?



src/tests/slave_recovery_tests.cpp
Line 2652 (original)


In the case of real reboot with slave info mismatch I think we should 
receive a task lost for this so why not set the expetation explicitly instead 
of removing it?



src/tests/slave_recovery_tests.cpp
Line 2655 (original), 2786 (patched)


Are there no `master/slave_removals` here? Seems like MESOS-5396 doens't 
apply here.


- Jiang Yan Xu


On July 10, 2017, 4:15 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56895/
> ---
> 
> (Updated July 10, 2017, 4:15 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/14/
> 
> 
> Testing
> ---
> 
> make check done together with 60104 and 60103
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

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

(Updated July 10, 2017, 11:15 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 (updated)
-

  src/tests/slave_recovery_tests.cpp f6eafcbe3d89c7a69c03db0fd7bc10ae73d06584 


Diff: https://reviews.apache.org/r/56895/diff/14/

Changes: https://reviews.apache.org/r/56895/diff/13-14/


Testing
---

make check done together with 60104 and 60103


Thanks,

Megha Sharma

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [60103, 60104, 60105, 56895]

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

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Jiang Yan Xu]

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


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)


Now that the agent ID doesn't change, this comment is no longer relevant?



src/tests/slave_recovery_tests.cpp
Lines 2670 (patched)


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)


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)


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)


s/slave/agent/ since this is a new test, let's use new terminology.



src/tests/slave_recovery_tests.cpp
Lines 2699 (patched)


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)


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)


s/checkInfo/slave info/



src/tests/slave_recovery_tests.cpp
Lines 2768 (patched)


s/slaveId/agent ID/



src/tests/slave_recovery_tests.cpp
Lines 2789 (patched)


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

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

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

(Updated June 30, 2017, 3:18 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 (updated)
-

  src/tests/slave_recovery_tests.cpp f6eafcbe3d89c7a69c03db0fd7bc10ae73d06584 


Diff: https://reviews.apache.org/r/56895/diff/13/

Changes: https://reviews.apache.org/r/56895/diff/12-13/


Testing
---

make check done together with 60104 and 60103


Thanks,

Megha Sharma

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Jiang Yan Xu]

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


Why the change? Doesn't look right.



src/tests/slave_recovery_tests.cpp
Line 2590 (original), 2590 (patched)


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)


s/postRebootContainerId/containerId/ No need to make such distinction?



src/tests/slave_recovery_tests.cpp
Lines 2699-2700 (patched)


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)


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)


Mention "reboot"?

Perhaps

s/RecoveryWithSlaveInfoMismatch/RebootWithSlaveInfoMismatch/



src/tests/slave_recovery_tests.cpp
Lines 2735 (patched)


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)


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)


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)


s/Changing/Change/ so the tense is more consistent.



src/tests/slave_recovery_tests.cpp
Lines 2760 (patched)


Not "same flags" right? :)



src/tests/slave_recovery_tests.cpp
Lines 2769 (patched)


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)


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)


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

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60103, 60104, 60105, 56895]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 23, 2017, 9: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, 9: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
> 
>

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

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

(Updated June 23, 2017, 9: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 (updated)
-

  src/tests/slave_recovery_tests.cpp 90c172b6f7ca0082c3dfa2082050d6c18e4e636c 


Diff: https://reviews.apache.org/r/56895/diff/12/

Changes: https://reviews.apache.org/r/56895/diff/11-12/


Testing
---

make check done together with 60104 and 60103


Thanks,

Megha Sharma

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60103, 60104, 60105, 56895]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 23, 2017, 6:01 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, 6:01 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/11/
> 
> 
> Testing
> ---
> 
> make check done together with 60104 and 60103
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

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

(Updated June 23, 2017, 6:01 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 (updated)
-

  src/tests/slave_recovery_tests.cpp 90c172b6f7ca0082c3dfa2082050d6c18e4e636c 


Diff: https://reviews.apache.org/r/56895/diff/11/

Changes: https://reviews.apache.org/r/56895/diff/10-11/


Testing
---

make check done together with 60104 and 60103


Thanks,

Megha Sharma

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60103, 60104, 60105, 56895]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 22, 2017, 9:15 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56895/
> ---
> 
> (Updated June 22, 2017, 9:15 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/10/
> 
> 
> Testing
> ---
> 
> make check done together with 60104 and 60103
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

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

(Updated June 22, 2017, 9:15 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 (updated)
---

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

  src/tests/slave_recovery_tests.cpp 90c172b6f7ca0082c3dfa2082050d6c18e4e636c 


Diff: https://reviews.apache.org/r/56895/diff/10/

Changes: https://reviews.apache.org/r/56895/diff/9-10/


Testing (updated)
---

make check done together with 60104 and 60103


Thanks,

Megha Sharma

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

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

(Updated June 15, 2017, 7:31 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 latest
symlink is cleared and no state is recoverd making the agent
register as a new agent with the master.


Diffs (updated)
-

  src/tests/slave_recovery_tests.cpp 38502584186793686f78ff5f4e03f36a3bf7ad1c 


Diff: https://reviews.apache.org/r/56895/diff/9/

Changes: https://reviews.apache.org/r/56895/diff/8-9/


Testing
---

make check


Thanks,

Megha Sharma

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Megha Sharma]

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

(Updated June 15, 2017, 5:52 p.m.)


Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.


Summary (updated)
-

Added tests to ensure slave recovery post reboot.


Bugs: MESOS-6223
https://issues.apache.org/jira/browse/MESOS-6223


Repository: mesos


Description (updated)
---

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 latest
symlink is cleared and no state is recoverd making the agent
register as a new agent with the master.


Diffs (updated)
-

  src/tests/slave_recovery_tests.cpp 38502584186793686f78ff5f4e03f36a3bf7ad1c 


Diff: https://reviews.apache.org/r/56895/diff/8/

Changes: https://reviews.apache.org/r/56895/diff/7-8/


Testing
---

make check


Thanks,

Megha Sharma