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