You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2015/02/17 17:23:06 UTC

Review Request 31114: Fixed flaky MasterTest.SlavesEndpointTwoSlaves caused by incorrect FUTURE_PROTOBUF usage.

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

Review request for mesos, Ben Mahler, Till Toenshoff, and Vinod Kone.


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


Repository: mesos-incubating


Description
-------

Sets the "to" end to all calls to `FUTURE_PROTOBUF` within the MasterTest.SlavesEndpointTwoSlaves. 

Failing to do that causes the test to fail on very rare ocasions, due to the first slave registering twice to the server (this is normal behavior according to [slave.cpp](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1087) and [master.cpp](https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2912).

It those ocasions, where the first slave registers twice, the master's sends a reponse to both registration requests before the second slave's turn, thus setting the future which was expecting data for the second slave with incorrect data.


Diffs
-----

  src/tests/master_tests.cpp 18eabd4 

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


Testing
-------

/bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure


Thanks,

Alexander Rojas


Re: Review Request 31114: Fixed flaky MasterTest.SlavesEndpointTwoSlaves caused by incorrect FUTURE_PROTOBUF usage.

Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31114/#review72723
-----------------------------------------------------------

Ship it!


Thanks for the follow up, Alexander. I will commit this shortly (while fixing a tiny nit).


src/tests/master_tests.cpp
<https://reviews.apache.org/r/31114/#comment118779>

    Kill this blank line please.


- Till Toenshoff


On Feb. 17, 2015, 4:23 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31114/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2015, 4:23 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-2355
>     https://issues.apache.org/jira/browse/MESOS-2355
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> Sets the "to" end to all calls to `FUTURE_PROTOBUF` within the MasterTest.SlavesEndpointTwoSlaves. 
> 
> Failing to do that causes the test to fail on very rare ocasions, due to the first slave registering twice to the server (this is normal behavior according to [slave.cpp](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1087) and [master.cpp](https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2912).
> 
> It those ocasions, where the first slave registers twice, the master's sends a reponse to both registration requests before the second slave's turn, thus setting the future which was expecting data for the second slave with incorrect data.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_tests.cpp 18eabd4 
> 
> Diff: https://reviews.apache.org/r/31114/diff/
> 
> 
> Testing
> -------
> 
> /bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


Re: Review Request 31114: Fixed flaky MasterTest.SlavesEndpointTwoSlaves caused by incorrect FUTURE_PROTOBUF usage.

Posted by Vinod Kone <vi...@gmail.com>.

> On Feb. 17, 2015, 6:13 p.m., Vinod Kone wrote:
> > src/tests/master_tests.cpp, lines 1616-1624
> > <https://reviews.apache.org/r/31114/diff/2/?file=866348#file866348line1616>
> >
> >     Hmm. I don't understand the fix here.
> >     
> >     First off, any expectations should be set *before* the corresponding actions (e.g., StartSlave).
> >     
> >     I think you just want to move the AWAIT_READY(slaveRegisteredMessage) before the second StartSlave()? IOW,
> >     
> >     ```
> >     // Start the first slave and wait for it to be registered.
> >     Future<SlaveRegisteredMessage> slave1RegisteredMessage =
> >       FUTURE_PROTOBUF(SlaveRegisteredMessage(), master.get(), _);
> >     
> >     StartSlave();
> >     AWAIT_READY(slave1RegisteredMessage);
> >     
> >     // Start the second slave and wait for it to be registered.
> >     Future<SlaveRegisteredMessage> slave2RegisteredMessage =
> >       FUTURE_PROTOBUF(SlaveRegisteredMessage(), master.get(), _);
> >     
> >     StartSlave();
> >     AWAIT_READY(slave2RegisteredMessage);
> >     ```
> 
> Alexander Rojas wrote:
>     That doesn't work, since the future will be ready before the second state starts by the second `SlaveRegisteredMessage` sent to the first slave. If you want to try it, just run `./bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure` on the master and you will see what I mean.
> 
> Alexander Rojas wrote:
>     Sorry about how short the first message was. You are right in the sense that the expectations should be set before, but they break the test eventually. Assuming the code you posted, consider the following timeline:
>     
>     1. Expectation on `slave1RegisteredMessage` set.
>     2. First slave starts, it registers to the server.
>     3. The delay for retry registration ([here](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1092)) is almost 0, slave sends a second registration request to the master.
>     4. `AWAIT_READY(slave1RegisteredMessage)`, message arrives to the slave, future set, first slave registered.
>     5. The master replies to the request of 3.
>     6. Expectation on `slave2RegisteredMessage` set.
>     7. Message from master arrives from 3, Future `slave2RegisteredMessage` set.
>     8. Second slave starts.
>     9. `AWAIT_READY(slave2RegisteredMessage)` doesn't wait, it was fulfilled in 6. Filled with data not related to the second slave. Test fails, we are not sure slave2 is running.

I see. Can you use a 'Not()' matcher (e.g., Not(slave1.get()) for the 2nd expectation?


- Vinod


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


On Feb. 17, 2015, 4:36 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31114/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2015, 4:36 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-2355
>     https://issues.apache.org/jira/browse/MESOS-2355
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> Sets the "to" end to all calls to `FUTURE_PROTOBUF` within the MasterTest.SlavesEndpointTwoSlaves. 
> 
> Failing to do that causes the test to fail on very rare ocasions, due to the first slave registering twice to the server (this is normal behavior according to [slave.cpp](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1087) and [master.cpp](https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2912).
> 
> It those ocasions, where the first slave registers twice, the master's sends a reponse to both registration requests before the second slave's turn, thus setting the future which was expecting data for the second slave with incorrect data.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_tests.cpp 18eabd4 
> 
> Diff: https://reviews.apache.org/r/31114/diff/
> 
> 
> Testing
> -------
> 
> /bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


Re: Review Request 31114: Fixed flaky MasterTest.SlavesEndpointTwoSlaves caused by incorrect FUTURE_PROTOBUF usage.

Posted by Alexander Rojas <al...@mesosphere.io>.

> On Feb. 17, 2015, 7:13 p.m., Vinod Kone wrote:
> > src/tests/master_tests.cpp, lines 1616-1624
> > <https://reviews.apache.org/r/31114/diff/2/?file=866348#file866348line1616>
> >
> >     Hmm. I don't understand the fix here.
> >     
> >     First off, any expectations should be set *before* the corresponding actions (e.g., StartSlave).
> >     
> >     I think you just want to move the AWAIT_READY(slaveRegisteredMessage) before the second StartSlave()? IOW,
> >     
> >     ```
> >     // Start the first slave and wait for it to be registered.
> >     Future<SlaveRegisteredMessage> slave1RegisteredMessage =
> >       FUTURE_PROTOBUF(SlaveRegisteredMessage(), master.get(), _);
> >     
> >     StartSlave();
> >     AWAIT_READY(slave1RegisteredMessage);
> >     
> >     // Start the second slave and wait for it to be registered.
> >     Future<SlaveRegisteredMessage> slave2RegisteredMessage =
> >       FUTURE_PROTOBUF(SlaveRegisteredMessage(), master.get(), _);
> >     
> >     StartSlave();
> >     AWAIT_READY(slave2RegisteredMessage);
> >     ```
> 
> Alexander Rojas wrote:
>     That doesn't work, since the future will be ready before the second state starts by the second `SlaveRegisteredMessage` sent to the first slave. If you want to try it, just run `./bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure` on the master and you will see what I mean.

Sorry about how short the first message was. You are right in the sense that the expectations should be set before, but they break the test eventually. Assuming the code you posted, consider the following timeline:

1. Expectation on `slave1RegisteredMessage` set.
2. First slave starts, it registers to the server.
3. The delay for retry registration ([here](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1092)) is almost 0, slave sends a second registration request to the master.
4. `AWAIT_READY(slave1RegisteredMessage)`, message arrives to the slave, future set, first slave registered.
5. The master replies to the request of 3.
6. Expectation on `slave2RegisteredMessage` set.
7. Message from master arrives from 3, Future `slave2RegisteredMessage` set.
8. Second slave starts.
9. `AWAIT_READY(slave2RegisteredMessage)` doesn't wait, it was fulfilled in 6. Filled with data not related to the second slave. Test fails, we are not sure slave2 is running.


- Alexander


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


On Feb. 17, 2015, 5:36 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31114/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2015, 5:36 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-2355
>     https://issues.apache.org/jira/browse/MESOS-2355
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> Sets the "to" end to all calls to `FUTURE_PROTOBUF` within the MasterTest.SlavesEndpointTwoSlaves. 
> 
> Failing to do that causes the test to fail on very rare ocasions, due to the first slave registering twice to the server (this is normal behavior according to [slave.cpp](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1087) and [master.cpp](https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2912).
> 
> It those ocasions, where the first slave registers twice, the master's sends a reponse to both registration requests before the second slave's turn, thus setting the future which was expecting data for the second slave with incorrect data.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_tests.cpp 18eabd4 
> 
> Diff: https://reviews.apache.org/r/31114/diff/
> 
> 
> Testing
> -------
> 
> /bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


Re: Review Request 31114: Fixed flaky MasterTest.SlavesEndpointTwoSlaves caused by incorrect FUTURE_PROTOBUF usage.

Posted by Alexander Rojas <al...@mesosphere.io>.

> On Feb. 17, 2015, 7:13 p.m., Vinod Kone wrote:
> > src/tests/master_tests.cpp, lines 1616-1624
> > <https://reviews.apache.org/r/31114/diff/2/?file=866348#file866348line1616>
> >
> >     Hmm. I don't understand the fix here.
> >     
> >     First off, any expectations should be set *before* the corresponding actions (e.g., StartSlave).
> >     
> >     I think you just want to move the AWAIT_READY(slaveRegisteredMessage) before the second StartSlave()? IOW,
> >     
> >     ```
> >     // Start the first slave and wait for it to be registered.
> >     Future<SlaveRegisteredMessage> slave1RegisteredMessage =
> >       FUTURE_PROTOBUF(SlaveRegisteredMessage(), master.get(), _);
> >     
> >     StartSlave();
> >     AWAIT_READY(slave1RegisteredMessage);
> >     
> >     // Start the second slave and wait for it to be registered.
> >     Future<SlaveRegisteredMessage> slave2RegisteredMessage =
> >       FUTURE_PROTOBUF(SlaveRegisteredMessage(), master.get(), _);
> >     
> >     StartSlave();
> >     AWAIT_READY(slave2RegisteredMessage);
> >     ```
> 
> Alexander Rojas wrote:
>     That doesn't work, since the future will be ready before the second state starts by the second `SlaveRegisteredMessage` sent to the first slave. If you want to try it, just run `./bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure` on the master and you will see what I mean.
> 
> Alexander Rojas wrote:
>     Sorry about how short the first message was. You are right in the sense that the expectations should be set before, but they break the test eventually. Assuming the code you posted, consider the following timeline:
>     
>     1. Expectation on `slave1RegisteredMessage` set.
>     2. First slave starts, it registers to the server.
>     3. The delay for retry registration ([here](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1092)) is almost 0, slave sends a second registration request to the master.
>     4. `AWAIT_READY(slave1RegisteredMessage)`, message arrives to the slave, future set, first slave registered.
>     5. The master replies to the request of 3.
>     6. Expectation on `slave2RegisteredMessage` set.
>     7. Message from master arrives from 3, Future `slave2RegisteredMessage` set.
>     8. Second slave starts.
>     9. `AWAIT_READY(slave2RegisteredMessage)` doesn't wait, it was fulfilled in 6. Filled with data not related to the second slave. Test fails, we are not sure slave2 is running.
> 
> Vinod Kone wrote:
>     I see. Can you use a 'Not()' matcher (e.g., Not(slave1.get()) for the 2nd expectation?

Thank's for the tip about the 'Not()' matcher. The fix now actually looks elegant.


- Alexander


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


On Feb. 17, 2015, 5:36 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31114/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2015, 5:36 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-2355
>     https://issues.apache.org/jira/browse/MESOS-2355
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> Sets the "to" end to all calls to `FUTURE_PROTOBUF` within the MasterTest.SlavesEndpointTwoSlaves. 
> 
> Failing to do that causes the test to fail on very rare ocasions, due to the first slave registering twice to the server (this is normal behavior according to [slave.cpp](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1087) and [master.cpp](https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2912).
> 
> It those ocasions, where the first slave registers twice, the master's sends a reponse to both registration requests before the second slave's turn, thus setting the future which was expecting data for the second slave with incorrect data.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_tests.cpp 18eabd4 
> 
> Diff: https://reviews.apache.org/r/31114/diff/
> 
> 
> Testing
> -------
> 
> /bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


Re: Review Request 31114: Fixed flaky MasterTest.SlavesEndpointTwoSlaves caused by incorrect FUTURE_PROTOBUF usage.

Posted by Alexander Rojas <al...@mesosphere.io>.

> On Feb. 17, 2015, 7:13 p.m., Vinod Kone wrote:
> > src/tests/master_tests.cpp, lines 1616-1624
> > <https://reviews.apache.org/r/31114/diff/2/?file=866348#file866348line1616>
> >
> >     Hmm. I don't understand the fix here.
> >     
> >     First off, any expectations should be set *before* the corresponding actions (e.g., StartSlave).
> >     
> >     I think you just want to move the AWAIT_READY(slaveRegisteredMessage) before the second StartSlave()? IOW,
> >     
> >     ```
> >     // Start the first slave and wait for it to be registered.
> >     Future<SlaveRegisteredMessage> slave1RegisteredMessage =
> >       FUTURE_PROTOBUF(SlaveRegisteredMessage(), master.get(), _);
> >     
> >     StartSlave();
> >     AWAIT_READY(slave1RegisteredMessage);
> >     
> >     // Start the second slave and wait for it to be registered.
> >     Future<SlaveRegisteredMessage> slave2RegisteredMessage =
> >       FUTURE_PROTOBUF(SlaveRegisteredMessage(), master.get(), _);
> >     
> >     StartSlave();
> >     AWAIT_READY(slave2RegisteredMessage);
> >     ```

That doesn't work, since the future will be ready before the second state starts by the second `SlaveRegisteredMessage` sent to the first slave. If you want to try it, just run `./bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure` on the master and you will see what I mean.


- Alexander


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


On Feb. 17, 2015, 5:36 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31114/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2015, 5:36 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-2355
>     https://issues.apache.org/jira/browse/MESOS-2355
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> Sets the "to" end to all calls to `FUTURE_PROTOBUF` within the MasterTest.SlavesEndpointTwoSlaves. 
> 
> Failing to do that causes the test to fail on very rare ocasions, due to the first slave registering twice to the server (this is normal behavior according to [slave.cpp](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1087) and [master.cpp](https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2912).
> 
> It those ocasions, where the first slave registers twice, the master's sends a reponse to both registration requests before the second slave's turn, thus setting the future which was expecting data for the second slave with incorrect data.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_tests.cpp 18eabd4 
> 
> Diff: https://reviews.apache.org/r/31114/diff/
> 
> 
> Testing
> -------
> 
> /bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


Re: Review Request 31114: Fixed flaky MasterTest.SlavesEndpointTwoSlaves caused by incorrect FUTURE_PROTOBUF usage.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31114/#review72741
-----------------------------------------------------------



src/tests/master_tests.cpp
<https://reviews.apache.org/r/31114/#comment118797>

    Hmm. I don't understand the fix here.
    
    First off, any expectations should be set *before* the corresponding actions (e.g., StartSlave).
    
    I think you just want to move the AWAIT_READY(slaveRegisteredMessage) before the second StartSlave()? IOW,
    
    ```
    // Start the first slave and wait for it to be registered.
    Future<SlaveRegisteredMessage> slave1RegisteredMessage =
      FUTURE_PROTOBUF(SlaveRegisteredMessage(), master.get(), _);
    
    StartSlave();
    AWAIT_READY(slave1RegisteredMessage);
    
    // Start the second slave and wait for it to be registered.
    Future<SlaveRegisteredMessage> slave2RegisteredMessage =
      FUTURE_PROTOBUF(SlaveRegisteredMessage(), master.get(), _);
    
    StartSlave();
    AWAIT_READY(slave2RegisteredMessage);
    ```


- Vinod Kone


On Feb. 17, 2015, 4:36 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31114/
> -----------------------------------------------------------
> 
> (Updated Feb. 17, 2015, 4:36 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-2355
>     https://issues.apache.org/jira/browse/MESOS-2355
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> Sets the "to" end to all calls to `FUTURE_PROTOBUF` within the MasterTest.SlavesEndpointTwoSlaves. 
> 
> Failing to do that causes the test to fail on very rare ocasions, due to the first slave registering twice to the server (this is normal behavior according to [slave.cpp](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1087) and [master.cpp](https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2912).
> 
> It those ocasions, where the first slave registers twice, the master's sends a reponse to both registration requests before the second slave's turn, thus setting the future which was expecting data for the second slave with incorrect data.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_tests.cpp 18eabd4 
> 
> Diff: https://reviews.apache.org/r/31114/diff/
> 
> 
> Testing
> -------
> 
> /bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


Re: Review Request 31114: Fixed flaky MasterTest.SlavesEndpointTwoSlaves caused by incorrect FUTURE_PROTOBUF usage.

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31114/#review72948
-----------------------------------------------------------


Patch looks great!

Reviews applied: [31114]

All tests passed.

- Mesos ReviewBot


On Feb. 18, 2015, 9:05 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31114/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2015, 9:05 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-2355
>     https://issues.apache.org/jira/browse/MESOS-2355
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> Sets the "to" end to all calls to `FUTURE_PROTOBUF` within the MasterTest.SlavesEndpointTwoSlaves. 
> 
> Failing to do that causes the test to fail on very rare ocasions, due to the first slave registering twice to the server (this is normal behavior according to [slave.cpp](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1087) and [master.cpp](https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2912).
> 
> It those ocasions, where the first slave registers twice, the master's sends a reponse to both registration requests before the second slave's turn, thus setting the future which was expecting data for the second slave with incorrect data.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_tests.cpp e69a7fb39e93a9a999e0991985f5d060b72aac65 
> 
> Diff: https://reviews.apache.org/r/31114/diff/
> 
> 
> Testing
> -------
> 
> /bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


Re: Review Request 31114: Fixed flaky MasterTest.SlavesEndpointTwoSlaves caused by incorrect FUTURE_PROTOBUF usage.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31114/#review72972
-----------------------------------------------------------

Ship it!


Ship It!

- Vinod Kone


On Feb. 18, 2015, 9:05 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31114/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2015, 9:05 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-2355
>     https://issues.apache.org/jira/browse/MESOS-2355
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> -------
> 
> Sets the "to" end to all calls to `FUTURE_PROTOBUF` within the MasterTest.SlavesEndpointTwoSlaves. 
> 
> Failing to do that causes the test to fail on very rare ocasions, due to the first slave registering twice to the server (this is normal behavior according to [slave.cpp](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1087) and [master.cpp](https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2912).
> 
> It those ocasions, where the first slave registers twice, the master's sends a reponse to both registration requests before the second slave's turn, thus setting the future which was expecting data for the second slave with incorrect data.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_tests.cpp e69a7fb39e93a9a999e0991985f5d060b72aac65 
> 
> Diff: https://reviews.apache.org/r/31114/diff/
> 
> 
> Testing
> -------
> 
> /bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


Re: Review Request 31114: Fixed flaky MasterTest.SlavesEndpointTwoSlaves caused by incorrect FUTURE_PROTOBUF usage.

Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31114/
-----------------------------------------------------------

(Updated Feb. 18, 2015, 10:05 a.m.)


Review request for mesos, Ben Mahler, Till Toenshoff, and Vinod Kone.


Changes
-------

Uses a `Not()` matcher to prevent catching messages intended to different destinataries.


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


Repository: mesos-incubating


Description
-------

Sets the "to" end to all calls to `FUTURE_PROTOBUF` within the MasterTest.SlavesEndpointTwoSlaves. 

Failing to do that causes the test to fail on very rare ocasions, due to the first slave registering twice to the server (this is normal behavior according to [slave.cpp](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1087) and [master.cpp](https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2912).

It those ocasions, where the first slave registers twice, the master's sends a reponse to both registration requests before the second slave's turn, thus setting the future which was expecting data for the second slave with incorrect data.


Diffs (updated)
-----

  src/tests/master_tests.cpp e69a7fb39e93a9a999e0991985f5d060b72aac65 

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


Testing
-------

/bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure


Thanks,

Alexander Rojas


Re: Review Request 31114: Fixed flaky MasterTest.SlavesEndpointTwoSlaves caused by incorrect FUTURE_PROTOBUF usage.

Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31114/
-----------------------------------------------------------

(Updated Feb. 17, 2015, 5:36 p.m.)


Review request for mesos, Ben Mahler, Till Toenshoff, and Vinod Kone.


Changes
-------

Till's comments.


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


Repository: mesos-incubating


Description
-------

Sets the "to" end to all calls to `FUTURE_PROTOBUF` within the MasterTest.SlavesEndpointTwoSlaves. 

Failing to do that causes the test to fail on very rare ocasions, due to the first slave registering twice to the server (this is normal behavior according to [slave.cpp](https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1087) and [master.cpp](https://github.com/apache/mesos/blob/master/src/master/master.cpp#L2912).

It those ocasions, where the first slave registers twice, the master's sends a reponse to both registration requests before the second slave's turn, thus setting the future which was expecting data for the second slave with incorrect data.


Diffs (updated)
-----

  src/tests/master_tests.cpp 18eabd4 

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


Testing
-------

/bin/mesos-tests.sh --gtest_filter="MasterTest.SlavesEndpoint*" --gtest_repeat=1000 --gtest_break_on_failure


Thanks,

Alexander Rojas