You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Adam B <ad...@mesosphere.io> on 2015/06/27 02:46:41 UTC
Review Request 35958: Updated SlaveTest.PingTimeoutNoPings test to use
custom timeout values.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35958/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Repository: mesos
Description
-------
Updated SlaveTest.PingTimeoutNoPings test to use custom timeout values.
Diffs
-----
src/tests/slave_tests.cpp e9002e807b500503c5bccf7ce638d98643f229ed
Diff: https://reviews.apache.org/r/35958/diff/
Testing
-------
Modified test passes after configurable ping timeout patch is applied.
Thanks,
Adam B
Re: Review Request 35958: Updated SlaveTest.PingTimeoutNoPings test
to use custom timeout values.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35958/
-----------------------------------------------------------
(Updated June 29, 2015, 8:09 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
BenM's feedback.
Repository: mesos
Description
-------
Updated SlaveTest.PingTimeoutNoPings test to use custom timeout values.
Diffs (updated)
-----
src/tests/slave_tests.cpp 90bbc81
Diff: https://reviews.apache.org/r/35958/diff/
Testing
-------
Modified test passes after configurable ping timeout patch is applied.
Thanks,
Adam B
Re: Review Request 35958: Updated SlaveTest.PingTimeoutNoPings test
to use custom timeout values.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35958/#review89609
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29507, 35958]
All tests passed.
- Mesos ReviewBot
On June 27, 2015, 12:46 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35958/
> -----------------------------------------------------------
>
> (Updated June 27, 2015, 12:46 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated SlaveTest.PingTimeoutNoPings test to use custom timeout values.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp e9002e807b500503c5bccf7ce638d98643f229ed
>
> Diff: https://reviews.apache.org/r/35958/diff/
>
>
> Testing
> -------
>
> Modified test passes after configurable ping timeout patch is applied.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 35958: Updated SlaveTest.PingTimeoutNoPings test
to use custom timeout values.
Posted by Adam B <ad...@mesosphere.io>.
> On June 29, 2015, 6:33 p.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, lines 1467-1472
> > <https://reviews.apache.org/r/35958/diff/1/?file=993804#file993804line1467>
> >
> > Any reason not to just be explicit and use:
> >
> > ```
> > // Set custom timeout values to verify the 'Connection' message.
> >
> > master::Flags masterFlags = CreateMasterFlags();
> >
> > masterFlags.slave_ping_timeout = Seconds(10);
> > masterFlags.max_slave_ping_timeouts = 2u;
> > ```
> >
> > Avoids the need for math in the comment :)
Fair enough. I hate maths.
> On June 29, 2015, 6:33 p.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, lines 1471-1472
> > <https://reviews.apache.org/r/35958/diff/1/?file=993804#file993804line1471>
> >
> > Mind moving this down or just inlining it in the EXPECT below? If you keep it, how about s/masterTimeout/totalTimeout/ ?
Keeping it, since it's also used in the Clock::advance(). Renamed to `totalTimeout`.
> On June 29, 2015, 6:33 p.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, lines 1490-1491
> > <https://reviews.apache.org/r/35958/diff/1/?file=993804#file993804line1490>
> >
> > How about pulling out Connection?
> >
> > ```
> > ASSERT_TRUE(slaveRegisteredMessage.get().has_connection());
> >
> > MasterSlaveConnection connection = slaveRegisteredMessage.get().connection();
> > EXPECT_EQ(totalTimeout, Duration(connection.total_ping_timeout_seconds());
> > ```
Done.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35958/#review89843
-----------------------------------------------------------
On June 26, 2015, 5:46 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35958/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 5:46 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated SlaveTest.PingTimeoutNoPings test to use custom timeout values.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp e9002e807b500503c5bccf7ce638d98643f229ed
>
> Diff: https://reviews.apache.org/r/35958/diff/
>
>
> Testing
> -------
>
> Modified test passes after configurable ping timeout patch is applied.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 35958: Updated SlaveTest.PingTimeoutNoPings test
to use custom timeout values.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35958/#review89843
-----------------------------------------------------------
Ship it!
Thank you!
src/tests/slave_tests.cpp (lines 1467 - 1472)
<https://reviews.apache.org/r/35958/#comment142684>
Any reason not to just be explicit and use:
```
// Set custom timeout values to verify the 'Connection' message.
master::Flags masterFlags = CreateMasterFlags();
masterFlags.slave_ping_timeout = Seconds(10);
masterFlags.max_slave_ping_timeouts = 2u;
```
Avoids the need for math in the comment :)
src/tests/slave_tests.cpp (lines 1471 - 1472)
<https://reviews.apache.org/r/35958/#comment142685>
Mind moving this down or just inlining it in the EXPECT below? If you keep it, how about s/masterTimeout/totalTimeout/ ?
src/tests/slave_tests.cpp (lines 1490 - 1491)
<https://reviews.apache.org/r/35958/#comment142687>
How about pulling out Connection?
```
ASSERT_TRUE(slaveRegisteredMessage.get().has_connection());
MasterSlaveConnection connection = slaveRegisteredMessage.get().connection();
EXPECT_EQ(totalTimeout, Duration(connection.total_ping_timeout_seconds());
```
- Ben Mahler
On June 27, 2015, 12:46 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35958/
> -----------------------------------------------------------
>
> (Updated June 27, 2015, 12:46 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated SlaveTest.PingTimeoutNoPings test to use custom timeout values.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp e9002e807b500503c5bccf7ce638d98643f229ed
>
> Diff: https://reviews.apache.org/r/35958/diff/
>
>
> Testing
> -------
>
> Modified test passes after configurable ping timeout patch is applied.
>
>
> Thanks,
>
> Adam B
>
>