Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Adam B]

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

Ship it!


Ship It!

- Adam B


On June 4, 2014, 2:51 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 4, 2014, 2:51 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added EXPECT_NO_FUTURE_PROTOBUFS() and 
> EXPECT_NO_FUTURE_MESSAGES() to make sure there's no more 
> SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated June 4, 2014, 9:51 p.m.)


Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.


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


Repository: mesos-git


Description (updated)
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added EXPECT_NO_FUTURE_PROTOBUFS() and 
EXPECT_NO_FUTURE_MESSAGES() to make sure there's no more 
SlaveReregisteredMessages.


Diffs
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 3b10a4c 
  src/tests/mesos.hpp 9ece77a 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


File Attachments


Using DROP_PROTOBUF()
  
https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

> On May 29, 2014, 5:13 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, line 815
> > 
> >
> > I dont think you need this abstraction. Just advance the clock and make 
> > sure there is no registration message sent.
> 
> Yifan Gu wrote:
> This is where I feel confused, because I found that I could not catch a 
> ReregisterSlaveMessage, instead, I could only catch the 
> SlaveReregisteredMessage.
> 
> Yifan Gu wrote:
> Updated:
> 
> Hey Vinod,
> I have tried to set up a Future 
> slaveReregisteredMessage = FUTURE_PROTOBUF(...).
> And tried to test if it's still not ready by 
> CHECK(slaveReregisteredMessage.isPending());
> But I found that once the FUTURE_PROTOBUF is set up, it will fail if 
> there is never called regardless if the future object is statisfied or not, 
> because there is a EXPECT_CALL in FUTURE_MESSAGE in 
> libprocess/include/gmock.hpp.
> 
> Sorry, I am not familiar with the future stuff, so maybe I missed 
> something here. It would be awesome if you could give some suggestions! 
> Thanks!
> 
> Vinod Kone wrote:
> I'm guessing you weren't able to catch "ReregisterSlaveMessage" because 
> you incorrectly set the "from", "to" fields in the FUTURE_PROTOBUF().
> 
> Regarding using future returned FUTURE_PROTOBUF to test for pending, you 
> are right it expects to be called once. But you can use the future from 
> DROP_PROTOBUF without that problem?
> 
> Yifan Gu wrote:
> You are right! I made a silly mistake here. I have replaced the 
> SlaveReregisteredMessage with ReregisterSlaveMessage, Thanks!
> 
> But I tried to use the future returned by DROP_PROTOBUF(), to replace 
> NO_FUTURE_PROTOBUFS(), and assert that the future is not ready at the end. 
> But I got the same result as using FUTURE_PROTOBUF(), I think that is because 
> both FUTURE_PROTOBUF and DROP_PROTOBUF are calling the same 
> process::FutureMessage() in gmock.hpp.
> 
> Please point me out if I am doing wrong.
> 
> I updated two diff files, r10 is the version using DROP_PROTOBUF(), which 
> fails the test.
> r11 is the version using NO_FUTURE_PROTOBUFS(), but replaced the 
> SlavereregisteredMessage to ReregisterSlaveMessage.
> 
> Please point me out if I am doing something wrong! Thank you!  
> 
>
> 
> Yifan Gu wrote:
> Ehh, I uploaded the wrong file for r10, so I just added a diff file...
> 
> Vinod Kone wrote:
> Sorry i meant DROP_PROTOBUFS. But I guess you can't use that to test no 
> messages were sent. Thinking more about this, I do like the NO_FUTURE_* 
> abstraction.

No worries. I assume that BenH has approved of this abstraction (see bellow), 
so I will mark this as fixed for now. If there are any other questions, feel 
free to re-open the issue.
Thanks!


- Yifan


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


On June 4, 2014, 9:46 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 4, 2014, 9:46 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated June 4, 2014, 9:46 p.m.)


Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.


Changes
---

Added EXPECT_ prefix to NO_FUTURE_*


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 3b10a4c 
  src/tests/mesos.hpp 9ece77a 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


File Attachments


Using DROP_PROTOBUF()
  
https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

> On June 4, 2014, 9:19 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/gmock.hpp, line 51
> > 
> >
> > This looks great but what about including the prefix EXPECT_?
> > 
> > EXPECT_NO_FUTURE_MESSAGES(name, _, _);
> > 
> > IMHO this will be easier to read. Action's like DROP_MESSAGE don't need 
> > the EXPECT_ prefix and we didn't originally add it to things like 
> > FUTURE_MESSAGE since they return a Future, but for "statements" like this I 
> > think the EXPECT_ prefix makes it more readable.

That's a good point! I agree. This should sound like an expectation. Thanks for 
suggestion! Ben!


- Yifan


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


On June 4, 2014, 9:25 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 4, 2014, 9:25 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

> On June 4, 2014, 7:20 p.m., Adam B wrote:
> > src/tests/slave_tests.cpp, lines 736-739
> > 
> >
> > Should all fit on one line, especially after removing "this->"

Done


> On June 4, 2014, 7:20 p.m., Adam B wrote:
> > src/tests/slave_tests.cpp, lines 747-752
> > 
> >
> > Since you don't really care about using up the entire offer, you can 
> > skip parsing the Resources and just call LaunchTasks(DEFAULT_EXECUTOR_INFO, 
> > 1, 1, 64, "*")
> > Default SlaveFlags setup cpus:2;mem:1024, so anything less than that is 
> > fine.

I see. That will be great.


- Yifan


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


On June 4, 2014, 9:25 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 4, 2014, 9:25 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated June 4, 2014, 9:25 p.m.)


Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.


Changes
---

Fix some style and comments.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 3b10a4c 
  src/tests/mesos.hpp 9ece77a 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


File Attachments


Using DROP_PROTOBUF()
  
https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Benjamin Hindman]

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



3rdparty/libprocess/include/process/gmock.hpp


This looks great but what about including the prefix EXPECT_?

EXPECT_NO_FUTURE_MESSAGES(name, _, _);

IMHO this will be easier to read. Action's like DROP_MESSAGE don't need the 
EXPECT_ prefix and we didn't originally add it to things like FUTURE_MESSAGE 
since they return a Future, but for "statements" like this I think the EXPECT_ 
prefix makes it more readable.


- Benjamin Hindman


On June 4, 2014, 9:18 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 4, 2014, 9:18 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated June 4, 2014, 9:18 p.m.)


Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.


Changes
---

Addressed some comments.
Deleted "this->" and flags.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 3b10a4c 
  src/tests/mesos.hpp 9ece77a 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


File Attachments


Using DROP_PROTOBUF()
  
https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

> On June 4, 2014, 7:20 p.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 719
> > 
> >
> > "this->" is unnecessary here and elsewhere in this function.
> 
> Yifan Gu wrote:
> Yes, I agree. But in slave_recovery_tests.cpp and master_tests.cpp, 
> "this->" is used there, but in gc_tests.cpp it is not used again, this gets 
> me a little bit confused.
> Could you please tell me what is the baseline about whether or not to use 
> "this->" in the tests?
> Or maybe just according to the "style" of the particular test file?
> Thanks in advance!
> 
> Vinod Kone wrote:
> slave_recovery_tests use "this" because they are typed tests and gtest 
> complains otherwise. master tests shouldn't use "this". it was a mistake.

I see, thanks!


- Yifan


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


On June 2, 2014, 11:02 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 2, 2014, 11:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Adam B]

> On June 4, 2014, 12:20 p.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 767
> > 
> >
> > Re: "when the master comes back" - The detector.appoint call doesn't 
> > actually failover the master, it just notifies the slave that there is a 
> > "new" master that it should reregister with. You can probably just cut that 
> > section out so the comment reads "Pause the clock here so the slave will 
> > not send multiple reregister messages before we change its state to 
> > TERMINATING."
> 
> Yifan Gu wrote:
> I see.
> So how about this:
> // Pause the clock here so after detecting a new master,
> // the slave will not send multiple reregister messages
> // before we change its state to TERMINATING

Sounds great!


- Adam


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


On June 2, 2014, 4:02 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 2, 2014, 4:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Vinod Kone]

> On June 4, 2014, 7:20 p.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 719
> > 
> >
> > "this->" is unnecessary here and elsewhere in this function.
> 
> Yifan Gu wrote:
> Yes, I agree. But in slave_recovery_tests.cpp and master_tests.cpp, 
> "this->" is used there, but in gc_tests.cpp it is not used again, this gets 
> me a little bit confused.
> Could you please tell me what is the baseline about whether or not to use 
> "this->" in the tests?
> Or maybe just according to the "style" of the particular test file?
> Thanks in advance!

slave_recovery_tests use "this" because they are typed tests and gtest 
complains otherwise. master tests shouldn't use "this". it was a mistake.


- Vinod


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


On June 2, 2014, 11:02 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 2, 2014, 11:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

> On June 4, 2014, 7:20 p.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 719
> > 
> >
> > "this->" is unnecessary here and elsewhere in this function.

Yes, I agree. But in slave_recovery_tests.cpp and master_tests.cpp, "this->" is 
used there, but in gc_tests.cpp it is not used again, this gets me a little bit 
confused.
Could you please tell me what is the baseline about whether or not to use 
"this->" in the tests?
Or maybe just according to the "style" of the particular test file?
Thanks in advance!


> On June 4, 2014, 7:20 p.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 767
> > 
> >
> > Re: "when the master comes back" - The detector.appoint call doesn't 
> > actually failover the master, it just notifies the slave that there is a 
> > "new" master that it should reregister with. You can probably just cut that 
> > section out so the comment reads "Pause the clock here so the slave will 
> > not send multiple reregister messages before we change its state to 
> > TERMINATING."

I see.
So how about this:
// Pause the clock here so after detecting a new master,
// the slave will not send multiple reregister messages
// before we change its state to TERMINATING


- Yifan


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


On June 2, 2014, 11:02 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 2, 2014, 11:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Vinod Kone]

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

Ship it!


lgtm modulo adam's comments.

- Vinod Kone


On June 2, 2014, 11:02 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 2, 2014, 11:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Vinod Kone]

> On May 29, 2014, 5:13 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, line 815
> > 
> >
> > I dont think you need this abstraction. Just advance the clock and make 
> > sure there is no registration message sent.
> 
> Yifan Gu wrote:
> This is where I feel confused, because I found that I could not catch a 
> ReregisterSlaveMessage, instead, I could only catch the 
> SlaveReregisteredMessage.
> 
> Yifan Gu wrote:
> Updated:
> 
> Hey Vinod,
> I have tried to set up a Future 
> slaveReregisteredMessage = FUTURE_PROTOBUF(...).
> And tried to test if it's still not ready by 
> CHECK(slaveReregisteredMessage.isPending());
> But I found that once the FUTURE_PROTOBUF is set up, it will fail if 
> there is never called regardless if the future object is statisfied or not, 
> because there is a EXPECT_CALL in FUTURE_MESSAGE in 
> libprocess/include/gmock.hpp.
> 
> Sorry, I am not familiar with the future stuff, so maybe I missed 
> something here. It would be awesome if you could give some suggestions! 
> Thanks!
> 
> Vinod Kone wrote:
> I'm guessing you weren't able to catch "ReregisterSlaveMessage" because 
> you incorrectly set the "from", "to" fields in the FUTURE_PROTOBUF().
> 
> Regarding using future returned FUTURE_PROTOBUF to test for pending, you 
> are right it expects to be called once. But you can use the future from 
> DROP_PROTOBUF without that problem?
> 
> Yifan Gu wrote:
> You are right! I made a silly mistake here. I have replaced the 
> SlaveReregisteredMessage with ReregisterSlaveMessage, Thanks!
> 
> But I tried to use the future returned by DROP_PROTOBUF(), to replace 
> NO_FUTURE_PROTOBUFS(), and assert that the future is not ready at the end. 
> But I got the same result as using FUTURE_PROTOBUF(), I think that is because 
> both FUTURE_PROTOBUF and DROP_PROTOBUF are calling the same 
> process::FutureMessage() in gmock.hpp.
> 
> Please point me out if I am doing wrong.
> 
> I updated two diff files, r10 is the version using DROP_PROTOBUF(), which 
> fails the test.
> r11 is the version using NO_FUTURE_PROTOBUFS(), but replaced the 
> SlavereregisteredMessage to ReregisterSlaveMessage.
> 
> Please point me out if I am doing something wrong! Thank you!  
> 
>
> 
> Yifan Gu wrote:
> Ehh, I uploaded the wrong file for r10, so I just added a diff file...

Sorry i meant DROP_PROTOBUFS. But I guess you can't use that to test no 
messages were sent. Thinking more about this, I do like the NO_FUTURE_* 
abstraction.


- Vinod


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


On June 2, 2014, 11:02 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 2, 2014, 11:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Adam B]

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

Ship it!


Looks great! Only minor nits & suggestions remaining.


src/tests/slave_tests.cpp


"this->" is unnecessary here and elsewhere in this function.



src/tests/slave_tests.cpp


Should all fit on one line, especially after removing "this->"



src/tests/slave_tests.cpp


Since you don't really care about using up the entire offer, you can skip 
parsing the Resources and just call LaunchTasks(DEFAULT_EXECUTOR_INFO, 1, 1, 
64, "*")
Default SlaveFlags setup cpus:2;mem:1024, so anything less than that is 
fine.



src/tests/slave_tests.cpp


Re: "when the master comes back" - The detector.appoint call doesn't 
actually failover the master, it just notifies the slave that there is a "new" 
master that it should reregister with. You can probably just cut that section 
out so the comment reads "Pause the clock here so the slave will not send 
multiple reregister messages before we change its state to TERMINATING."



src/tests/slave_tests.cpp


s/we/the master/



src/tests/slave_tests.cpp


s/_/master.get()/



src/tests/slave_tests.cpp


s/this->//


- Adam B


On June 2, 2014, 4:02 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 2, 2014, 4:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [21968]

All tests passed.

- Mesos ReviewBot


On June 2, 2014, 11:02 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 2, 2014, 11:02 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 3b10a4c 
>   src/tests/mesos.hpp 9ece77a 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated June 2, 2014, 11:02 p.m.)


Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.


Changes
---

rebased.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 3b10a4c 
  src/tests/mesos.hpp 9ece77a 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


File Attachments


Using DROP_PROTOBUF()
  
https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated June 2, 2014, 6:32 p.m.)


Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.


Changes
---

rebase.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 3b10a4c 
  src/tests/mesos.hpp 9ece77a 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


File Attachments


Using DROP_PROTOBUF()
  
https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [21968]

Failed command: git apply --index 21968.patch

Error:
 error: patch failed: src/slave/slave.cpp:800
error: src/slave/slave.cpp: patch does not apply


- Mesos ReviewBot


On June 2, 2014, 7:29 a.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated June 2, 2014, 7:29 a.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated June 2, 2014, 7:29 a.m.)


Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


File Attachments


Using DROP_PROTOBUF()
  
https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [21968]

Failed command: git apply --index 21968.patch

Error:
 error: patch failed: src/slave/slave.cpp:800
error: src/slave/slave.cpp: patch does not apply


- Mesos ReviewBot


On May 30, 2014, 9:45 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated May 30, 2014, 9:45 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

> On May 28, 2014, 9:05 p.m., Yifan Gu wrote:
> > src/tests/slave_tests.cpp, line 799
> > 
> >
> > The comment may not be true, I'm digging into it.

Update:
It's true. If I don't drop the message, the slave will terminate immediately.


- Yifan


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


On May 30, 2014, 9:45 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated May 30, 2014, 9:45 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

> On May 29, 2014, 4:14 p.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 736
> > 
> >
> > Why needed, to simulate master failover? Can't you just test this 
> > during initial registration?

See below.


> On May 29, 2014, 4:14 p.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 743
> > 
> >
> > Why wait until registered the first time, instead of testing 
> > TERMINATING during original registration?

See below.


- Yifan


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


On May 30, 2014, 9:45 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated May 30, 2014, 9:45 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 30, 2014, 9:45 p.m.)


Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.


Changes
---

Modified the comment( s/SlaveReregisteredMesssage/ReregisterSlaveMessage )


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


File Attachments


Using DROP_PROTOBUF()
  
https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

> On May 29, 2014, 5:13 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, line 815
> > 
> >
> > I dont think you need this abstraction. Just advance the clock and make 
> > sure there is no registration message sent.
> 
> Yifan Gu wrote:
> This is where I feel confused, because I found that I could not catch a 
> ReregisterSlaveMessage, instead, I could only catch the 
> SlaveReregisteredMessage.
> 
> Yifan Gu wrote:
> Updated:
> 
> Hey Vinod,
> I have tried to set up a Future 
> slaveReregisteredMessage = FUTURE_PROTOBUF(...).
> And tried to test if it's still not ready by 
> CHECK(slaveReregisteredMessage.isPending());
> But I found that once the FUTURE_PROTOBUF is set up, it will fail if 
> there is never called regardless if the future object is statisfied or not, 
> because there is a EXPECT_CALL in FUTURE_MESSAGE in 
> libprocess/include/gmock.hpp.
> 
> Sorry, I am not familiar with the future stuff, so maybe I missed 
> something here. It would be awesome if you could give some suggestions! 
> Thanks!
> 
> Vinod Kone wrote:
> I'm guessing you weren't able to catch "ReregisterSlaveMessage" because 
> you incorrectly set the "from", "to" fields in the FUTURE_PROTOBUF().
> 
> Regarding using future returned FUTURE_PROTOBUF to test for pending, you 
> are right it expects to be called once. But you can use the future from 
> DROP_PROTOBUF without that problem?
> 
> Yifan Gu wrote:
> You are right! I made a silly mistake here. I have replaced the 
> SlaveReregisteredMessage with ReregisterSlaveMessage, Thanks!
> 
> But I tried to use the future returned by DROP_PROTOBUF(), to replace 
> NO_FUTURE_PROTOBUFS(), and assert that the future is not ready at the end. 
> But I got the same result as using FUTURE_PROTOBUF(), I think that is because 
> both FUTURE_PROTOBUF and DROP_PROTOBUF are calling the same 
> process::FutureMessage() in gmock.hpp.
> 
> Please point me out if I am doing wrong.
> 
> I updated two diff files, r10 is the version using DROP_PROTOBUF(), which 
> fails the test.
> r11 is the version using NO_FUTURE_PROTOBUFS(), but replaced the 
> SlavereregisteredMessage to ReregisterSlaveMessage.
> 
> Please point me out if I am doing something wrong! Thank you!  
> 
>

Ehh, I uploaded the wrong file for r10, so I just added a diff file...


- Yifan


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


On May 30, 2014, 9:37 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated May 30, 2014, 9:37 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> File Attachments
> 
> 
> Using DROP_PROTOBUF()
>   
> https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 30, 2014, 9:37 p.m.)


Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


File Attachments (updated)


Using DROP_PROTOBUF()
  
https://reviews.apache.org/media/uploaded/files/2014/05/30/7c0e1b69-19c1-4868-865e-d47fbb739585__mesos.diff


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

> On May 29, 2014, 5:13 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, line 815
> > 
> >
> > I dont think you need this abstraction. Just advance the clock and make 
> > sure there is no registration message sent.
> 
> Yifan Gu wrote:
> This is where I feel confused, because I found that I could not catch a 
> ReregisterSlaveMessage, instead, I could only catch the 
> SlaveReregisteredMessage.
> 
> Yifan Gu wrote:
> Updated:
> 
> Hey Vinod,
> I have tried to set up a Future 
> slaveReregisteredMessage = FUTURE_PROTOBUF(...).
> And tried to test if it's still not ready by 
> CHECK(slaveReregisteredMessage.isPending());
> But I found that once the FUTURE_PROTOBUF is set up, it will fail if 
> there is never called regardless if the future object is statisfied or not, 
> because there is a EXPECT_CALL in FUTURE_MESSAGE in 
> libprocess/include/gmock.hpp.
> 
> Sorry, I am not familiar with the future stuff, so maybe I missed 
> something here. It would be awesome if you could give some suggestions! 
> Thanks!
> 
> Vinod Kone wrote:
> I'm guessing you weren't able to catch "ReregisterSlaveMessage" because 
> you incorrectly set the "from", "to" fields in the FUTURE_PROTOBUF().
> 
> Regarding using future returned FUTURE_PROTOBUF to test for pending, you 
> are right it expects to be called once. But you can use the future from 
> DROP_PROTOBUF without that problem?

You are right! I made a silly mistake here. I have replaced the 
SlaveReregisteredMessage with ReregisterSlaveMessage, Thanks!

But I tried to use the future returned by DROP_PROTOBUF(), to replace 
NO_FUTURE_PROTOBUFS(), and assert that the future is not ready at the end. But 
I got the same result as using FUTURE_PROTOBUF(), I think that is because both 
FUTURE_PROTOBUF and DROP_PROTOBUF are calling the same process::FutureMessage() 
in gmock.hpp.

Please point me out if I am doing wrong.

I updated two diff files, r10 is the version using DROP_PROTOBUF(), which fails 
the test.
r11 is the version using NO_FUTURE_PROTOBUFS(), but replaced the 
SlavereregisteredMessage to ReregisterSlaveMessage.

Please point me out if I am doing something wrong! Thank you!  


- Yifan


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


On May 30, 2014, 9:34 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated May 30, 2014, 9:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 30, 2014, 9:34 p.m.)


Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Vinod Kone]

> On May 29, 2014, 5:13 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, line 815
> > 
> >
> > I dont think you need this abstraction. Just advance the clock and make 
> > sure there is no registration message sent.
> 
> Yifan Gu wrote:
> This is where I feel confused, because I found that I could not catch a 
> ReregisterSlaveMessage, instead, I could only catch the 
> SlaveReregisteredMessage.
> 
> Yifan Gu wrote:
> Updated:
> 
> Hey Vinod,
> I have tried to set up a Future 
> slaveReregisteredMessage = FUTURE_PROTOBUF(...).
> And tried to test if it's still not ready by 
> CHECK(slaveReregisteredMessage.isPending());
> But I found that once the FUTURE_PROTOBUF is set up, it will fail if 
> there is never called regardless if the future object is statisfied or not, 
> because there is a EXPECT_CALL in FUTURE_MESSAGE in 
> libprocess/include/gmock.hpp.
> 
> Sorry, I am not familiar with the future stuff, so maybe I missed 
> something here. It would be awesome if you could give some suggestions! 
> Thanks!

I'm guessing you weren't able to catch "ReregisterSlaveMessage" because you 
incorrectly set the "from", "to" fields in the FUTURE_PROTOBUF().

Regarding using future returned FUTURE_PROTOBUF to test for pending, you are 
right it expects to be called once. But you can use the future from 
DROP_PROTOBUF without that problem?


- Vinod


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


On May 29, 2014, 11:18 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated May 29, 2014, 11:18 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

> On May 29, 2014, 5:13 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, line 815
> > 
> >
> > I dont think you need this abstraction. Just advance the clock and make 
> > sure there is no registration message sent.
> 
> Yifan Gu wrote:
> This is where I feel confused, because I found that I could not catch a 
> ReregisterSlaveMessage, instead, I could only catch the 
> SlaveReregisteredMessage.

Updated:

Hey Vinod,
I have tried to set up a Future 
slaveReregisteredMessage = FUTURE_PROTOBUF(...).
And tried to test if it's still not ready by 
CHECK(slaveReregisteredMessage.isPending());
But I found that once the FUTURE_PROTOBUF is set up, it will fail if there is 
never called regardless if the future object is statisfied or not, because 
there is a EXPECT_CALL in FUTURE_MESSAGE in libprocess/include/gmock.hpp.

Sorry, I am not familiar with the future stuff, so maybe I missed something 
here. It would be awesome if you could give some suggestions! Thanks!


- Yifan


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


On May 29, 2014, 11:18 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated May 29, 2014, 11:18 p.m.)
> 
> 
> Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 29, 2014, 11:18 p.m.)


Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.


Changes
---

Remove an unnecessary future.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 29, 2014, 10:51 p.m.)


Review request for mesos, Adam B, Niklas Nielsen, and Vinod Kone.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

> On May 29, 2014, 5:13 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, lines 753-768
> > 
> >
> > Use the LaunchTasks action to simplify this. See allocator_tests and 
> > gc_tests for examples.

Thanks! I copied that from the gc_tests.


> On May 29, 2014, 5:13 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, line 815
> > 
> >
> > I dont think you need this abstraction. Just advance the clock and make 
> > sure there is no registration message sent.

This is where I feel confused, because I found that I could not catch a 
ReregisterSlaveMessage, instead, I could only catch the 
SlaveReregisteredMessage.


- Yifan


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


On May 29, 2014, 10:40 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated May 29, 2014, 10:40 p.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

> On May 29, 2014, 4:14 p.m., Adam B wrote:
> > src/tests/slave_tests.cpp, lines 725-726
> > 
> >
> > This could instead go below, just before StartSlave, or above the exec 
> > definition (first of 3 parameters to StartSlave). I just don't like to see 
> > the exec/flags/detector parameter setup broken up.

Fixed.


> On May 29, 2014, 4:14 p.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 814
> > 
> >
> > "expectation" and explain in the comment what you're expecting

Added a few more explaination.


> On May 29, 2014, 4:14 p.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 745
> > 
> >
> > Why create/run a task at all?

I have tested that if there is no tasks running, the slave will terminate 
immediately, which is not what I expect.
Besides, in order to run tasks on the slave, the slave must have registered 
with the master first... So I did the failover and re-registeration stuff..


- Yifan


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


On May 29, 2014, 10:40 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated May 29, 2014, 10:40 p.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 29, 2014, 10:40 p.m.)


Review request for mesos, Adam B and Niklas Nielsen.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 29, 2014, 10:37 p.m.)


Review request for mesos, Adam B and Niklas Nielsen.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 29, 2014, 10:35 p.m.)


Review request for mesos, Adam B and Niklas Nielsen.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Vinod Kone]

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



src/tests/slave_tests.cpp


s/and a slave//.

Put the "// Start a slave" comment below where you start the slave.



src/tests/slave_tests.cpp


Use the LaunchTasks action to simplify this. See allocator_tests and 
gc_tests for examples.



src/tests/slave_tests.cpp


s/ShutdownExecutorMessages/ShutdownExecutorMessage/



src/tests/slave_tests.cpp


I dont think you need this abstraction. Just advance the clock and make 
sure there is no registration message sent.


- Vinod Kone


On May 28, 2014, 10:18 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated May 28, 2014, 10:18 p.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Adam B]

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


Looks great, but I wonder if we can simplify the unit test a little (test on 
register, not reregister)


3rdparty/libprocess/include/process/gmock.hpp


This strategy seems reasonable to me. The only other alternative I can 
think of is to get the future from FUTURE_MESSAGE, and then ensure that it is 
never satisfied/ready by the end of your test. We should probably ask BenH for 
his opinion.



src/tests/slave_tests.cpp


This could instead go below, just before StartSlave, or above the exec 
definition (first of 3 parameters to StartSlave). I just don't like to see the 
exec/flags/detector parameter setup broken up.



src/tests/slave_tests.cpp


Why needed, to simulate master failover? Can't you just test this during 
initial registration?



src/tests/slave_tests.cpp


Why wait until registered the first time, instead of testing TERMINATING 
during original registration?



src/tests/slave_tests.cpp


Why create/run a task at all?



src/tests/slave_tests.cpp


Unnecessary, Times(1) is implied



src/tests/slave_tests.cpp


Please provide a non-empty task name



src/tests/slave_tests.cpp


Unnecessary



src/tests/slave_tests.cpp


Interesting trick. Explain..



src/tests/slave_tests.cpp


"expectation" and explain in the comment what you're expecting



src/tests/slave_tests.cpp


You might want to set this expectation a little earlier (right after your 
await_ready), so you don't miss an earlier message.


- Adam B


On May 28, 2014, 3:18 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated May 28, 2014, 3:18 p.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 28, 2014, 10:18 p.m.)


Review request for mesos, Adam B and Niklas Nielsen.


Changes
---

Delete header file for stout/duration.

Ensure the slave is in TERMINATING when calling doReliableRegistration().


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 28, 2014, 10:15 p.m.)


Review request for mesos, Adam B and Niklas Nielsen.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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



src/tests/slave_tests.cpp


The comment may not be true, I'm digging into it.


- Yifan Gu


On May 28, 2014, 7:26 p.m., Yifan Gu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21968/
> ---
> 
> (Updated May 28, 2014, 7:26 p.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-878
> https://issues.apache.org/jira/browse/MESOS-878
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Added a sentence to check the slave's state in doReliableRegistration() to 
> make sure that the function returns when the slave's state is TERMINATING.
> Also, to write the test, I need to make sure there's no additional 
> doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
> NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gmock.hpp daba7e3 
>   src/slave/slave.cpp 7fbedb1 
>   src/tests/mesos.hpp 1b0f358 
>   src/tests/slave_tests.cpp 80fe3cf 
> 
> Diff: https://reviews.apache.org/r/21968/diff/
> 
> 
> Testing
> ---
> 
> Slave_tests.cpp:TerminatingSlaveDoesNotReregister.
> 
> 
> Thanks,
> 
> Yifan Gu
> 
>

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 28, 2014, 7:26 p.m.)


Review request for mesos, Adam B and Niklas Nielsen.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 28, 2014, 6:59 p.m.)


Review request for mesos, Adam B and Niklas Nielsen.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 28, 2014, 6:16 a.m.)


Review request for mesos, Adam B and Niklas Nielsen.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing (updated)
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister.


Thanks,

Yifan Gu

Re: Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

(Updated May 28, 2014, 6:14 a.m.)


Review request for mesos, Adam B and Niklas Nielsen.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister


Thanks,

Yifan Gu

Review Request 21968: Fix issue #878 Slave should not register with the master when in TERMINATING.

[Yifan Gu]

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

Review request for mesos, Adam B and Niklas Nielsen.


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


Repository: mesos-git


Description
---

Added a sentence to check the slave's state in doReliableRegistration() to make 
sure that the function returns when the slave's state is TERMINATING.
Also, to write the test, I need to make sure there's no additional 
doReliableRegistration() calls, so I added NO_FUTURE_PROTOBUFS() and 
NO_FUTURE_MESSAGES() to make sure there's no more SlaveReregisteredMessages.


Diffs
-

  3rdparty/libprocess/include/process/gmock.hpp daba7e3 
  src/slave/slave.cpp 7fbedb1 
  src/tests/mesos.hpp 1b0f358 
  src/tests/slave_tests.cpp 80fe3cf 

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


Testing
---

Slave_tests.cpp:TerminatingSlaveDoesNotReregister


Thanks,

Yifan Gu