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

Reply via email to