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

Reply via email to