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