-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34737/#review85586
-----------------------------------------------------------

Ship it!


Looks good. Main issue is that this can and should be a generally useful test 
not one to verify we don't segfault after the fix. Please reword comments and 
add expectations for response output. Other comments are stylistic and most are 
copied from the other monitor test.


src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/34737/#comment137178>

    I'd state this more generally as a test for correct handling of the 
statistics.json endpoint when monitoring of a container is stopped, i.e., the 
test is useful independently of the bug revealed in MESOS-2771.



src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/34737/#comment137171>

    s/Setup/Set up/



src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/34737/#comment137173>

    No yours but add a macro to tests/mesos.hpp?



src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/34737/#comment137172>

    Can you use the DEFAULT_EXECUTOR_ID macro from tests/mesos.hpp?



src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/34737/#comment137174>

    DEFAULT_EXECUTOR_INFO?



src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/34737/#comment137176>

    s/setup/set up/



src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/34737/#comment137179>

    Again, not yours but
    
    Single line?
    
    AWAIT_READY(monitor.start())?



src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/34737/#comment137184>

    s/Cause/Induce a/



src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/34737/#comment137186>

    "Stop monitoring the container"?



src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/34737/#comment137185>

    AWAIT_READY()?



src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/34737/#comment137188>

    Reword this comment to make it general and not specific to the bug in 
MESOS-2771.



src/tests/monitor_tests.cpp
<https://reviews.apache.org/r/34737/#comment137182>

    To make this test general, please check the expected output in response, 
i.e., the container will *not* be included.


- Ian Downes


On May 27, 2015, 4:14 p.m., Niklas Nielsen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34737/
> -----------------------------------------------------------
> 
> (Updated May 27, 2015, 4:14 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-2771
>     https://issues.apache.org/jira/browse/MESOS-2771
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added test to verify fix for MESOS-2771
> 
> 
> Diffs
> -----
> 
>   src/tests/monitor_tests.cpp ca3b7f4a85824697a4e6701285f69ba337506147 
> 
> Diff: https://reviews.apache.org/r/34737/diff/
> 
> 
> Testing
> -------
> 
> Test verifies fix for MESOS-2771, where references to executor infos and 
> container ids could get invalidated if monitoring of a container stopped 
> during a statistics collection.
> 
> Tested with make check before and after fix.
> 
> Before:
> 
> I0527 15:51:57.943672 388640768 process.cpp:2201] Cleaning up 
> __http__(1)@10.0.68.255:50624
> I0527 15:51:57.943717 387567616 process.cpp:2094] Resuming 
> __gc__@10.0.68.255:50624 at 2015-05-27 22:51:57.943728896+00:00
> PC: @        0x11122bb3d std::__1::operator<< <>()
> *** SIGSEGV (@0x0) received by PID 42293 (TID 0x7fff7cdb2300) stack trace: ***
>     @     0x7fff8eaebf1a _sigtramp
>     @     0x7fd785800ee8 (unknown)
>     @        0x1112e97d0 mesos::operator<<()
>     @        0x111b41731 
> mesos::internal::slave::ResourceMonitorProcess::usage()::$_0::operator()()
>     @        0x111b415ed 
> _ZZNK7process6FutureIN5mesos8internal5slave15ResourceMonitor5UsageEE8onFailedIZNS3_22ResourceMonitorProcess5usageENS1_11ContainerIDEE3$_0vEERKS6_OT_NS6_6PreferEENUlRKNSt3__112basic_stringIcNSG_11char_traitsIcEENSG_9allocatorIcEEEEE_clESO_
>     @        0x111b4152c 
> _ZNSt3__110__function6__funcIZNK7process6FutureIN5mesos8internal5slave15ResourceMonitor5UsageEE8onFailedIZNS6_22ResourceMonitorProcess5usageENS4_11ContainerIDEE3$_0vEERKS9_OT_NS9_6PreferEEUlRKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEE_NSM_ISR_EEFvSQ_EEclESQ_
>     @        0x112106544 std::__1::function<>::operator()()
>     @        0x1112067ab 
> _ZN7process8internal3runINSt3__18functionIFvRKNS2_12basic_stringIcNS2_11char_traitsIcEENS2_9allocatorIcEEEEEEEJRS9_EEEvRKNS2_6vectorIT_NS7_ISG_EEEEDpOT0_
>     @        0x111b86400 process::Future<>::fail()
>     @        0x111ba05bd process::Promise<>::fail()
>     @        0x111b9722c process::internal::thenf<>()
>     @        0x111b9f823 
> _ZNSt3__110__function6__funcINS_6__bindIPFvRKNS_8functionIFN7process6FutureIN5mesos8internal5slave15ResourceMonitor5UsageEEERKNS6_18ResourceStatisticsEEEERKNS_10shared_ptrINS4_7PromiseISA_EEEERKNS5_ISC_EEEJSI_RSM_RNS_12placeholders4__phILi1EEEEEENS_9allocatorISZ_EEFvSR_EEclESR_
>     @        0x1121db254 std::__1::function<>::operator()()
>     @        0x111b9c90d 
> _ZZNK7process6FutureIN5mesos18ResourceStatisticsEE5onAnyIRNSt3__18functionIFvRKS3_EEEvEES8_OT_NS3_6PreferEENUlS8_E_clES8_
>     @        0x111b9c84c 
> _ZNSt3__110__function6__funcIZNK7process6FutureIN5mesos18ResourceStatisticsEE5onAnyIRNS_8functionIFvRKS6_EEEvEESA_OT_NS6_6PreferEEUlSA_E_NS_9allocatorISH_EESB_EclESA_
>     @        0x10e73b0d4 std::__1::function<>::operator()()
>     @        0x10e73a50b 
> _ZN7process8internal3runINSt3__18functionIFvRKNS_6FutureIN5mesos18ResourceStatisticsEEEEEEJRS7_EEEvRKNS2_6vectorIT_NS2_9allocatorISE_EEEEDpOT0_
>     @        0x10efd83c1 process::Future<>::fail()
>     @        0x10efdd49d 
> _ZZNK7process6FutureIN5mesos18ResourceStatisticsEE8onFailedINSt3__16__bindIMS3_FbRKNS5_12basic_stringIcNS5_11char_traitsIcEENS5_9allocatorIcEEEEEJRS3_RNS5_12placeholders4__phILi1EEEEEEbEERKS3_OT_NS3_6PreferEENUlSE_E_clESE_
>     @        0x10efdd1fc 
> _ZNSt3__110__function6__funcIZNK7process6FutureIN5mesos18ResourceStatisticsEE8onFailedINS_6__bindIMS6_FbRKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEEJRS6_RNS_12placeholders4__phILi1EEEEEEbEERKS6_OT_NS6_6PreferEEUlSG_E_NSC_ISU_EEFvSG_EEclESG_
>     @        0x10e5b2304 std::__1::function<>::operator()()
>     @        0x10efdb9f9 process::Future<>::onFailed()
>     @        0x10efdb6c8 
> _ZNK7process6FutureIN5mesos18ResourceStatisticsEE8onFailedINSt3__16__bindIMS3_FbRKNS5_12basic_stringIcNS5_11char_traitsIcEENS5_9allocatorIcEEEEEJRS3_RNS5_12placeholders4__phILi1EEEEEEbEERKS3_OT_NS3_6PreferE
>     @        0x10efd9145 
> _ZNK7process6FutureIN5mesos18ResourceStatisticsEE8onFailedINSt3__16__bindIMS3_FbRKNS5_12basic_stringIcNS5_11char_traitsIcEENS5_9allocatorIcEEEEEJRS3_RNS5_12placeholders4__phILi1EEEEEEEERKS3_OT_
>     @        0x10efd8b43 process::Promise<>::associate()
>     @        0x10efd81bd process::Promise<>::set()
>     @        0x10efd684e 
> mesos::internal::tests::MonitorTest_UsageFailure_Test::TestBody()
>     @        0x10f4155b3 
> testing::internal::HandleSehExceptionsInMethodIfSupported<>()
>     @        0x10f3fd667 
> testing::internal::HandleExceptionsInMethodIfSupported<>()
>     @        0x10f3d7135 testing::Test::Run()
>     @        0x10f3d80eb testing::TestInfo::Run()
>     @        0x10f3d8cd7 testing::TestCase::Run()
> ^C^C
> [1]    42293 segmentation fault  GLOG_v=2 ./bin/mesos-tests.sh 
> --gtest_filter="MonitorTest.UsageFailure"
> 
> After:
> 
> [ RUN      ] MonitorTest.UsageFailure
> I0527 15:55:41.984792 2094736128 process.cpp:2084] Spawned process 
> __limiter__(2)@10.0.68.255:50644
> I0527 15:55:41.984801 242876416 process.cpp:2094] Resuming 
> __limiter__(2)@10.0.68.255:50644 at 2015-05-27 22:55:41.984818944+00:00
> I0527 15:55:41.984833 243949568 process.cpp:2094] Resuming 
> monitor@10.0.68.255:50644 at 2015-05-27 22:55:41.984848128+00:00
> I0527 15:55:41.984894 2094736128 process.cpp:2084] Spawned process 
> monitor@10.0.68.255:50644
> I0527 15:55:41.984953 242876416 process.cpp:2094] Resuming 
> help@10.0.68.255:50644 at 2015-05-27 22:55:41.984967936+00:00
> I0527 15:55:41.985086 245559296 process.cpp:2094] Resuming 
> monitor@10.0.68.255:50644 at 2015-05-27 22:55:41.985101824+00:00
> I0527 15:55:41.985436 242876416 process.cpp:2094] Resuming 
> __gc__@10.0.68.255:50644 at 2015-05-27 22:55:41.985452800+00:00
> I0527 15:55:41.985443 246632448 process.cpp:2094] Resuming 
> __latch__(1)@10.0.68.255:50644 at 2015-05-27 22:55:41.985459200+00:00
> I0527 15:55:41.985455 2094736128 process.cpp:2084] Spawned process 
> __latch__(1)@10.0.68.255:50644
> I0527 15:55:41.985589 2094736128 process.cpp:2084] Spawned process 
> __waiter__(1)@10.0.68.255:50644
> I0527 15:55:41.985604 246095872 process.cpp:2094] Resuming 
> __waiter__(1)@10.0.68.255:50644 at 2015-05-27 22:55:41.985619968+00:00
> I0527 15:55:41.986281 244486144 process.cpp:2094] Resuming 
> monitor@10.0.68.255:50644 at 2015-05-27 22:55:41.986298112+00:00
> I0527 15:55:41.986320 244486144 process.cpp:2704] Handling HTTP event for 
> process 'monitor' with path: '/monitor/statistics.json'
> I0527 15:55:41.986418 243412992 process.cpp:2094] Resuming 
> __http__(1)@10.0.68.255:50644 at 2015-05-27 22:55:41.986438912+00:00
> I0527 15:55:41.986419 245559296 process.cpp:2094] Resuming 
> __gc__@10.0.68.255:50644 at 2015-05-27 22:55:41.986437120+00:00
> I0527 15:55:41.986429 244486144 process.cpp:2084] Spawned process 
> __http__(1)@10.0.68.255:50644
> I0527 15:55:41.986556 245559296 process.cpp:2094] Resuming 
> __http__(1)@10.0.68.255:50644 at 2015-05-27 22:55:41.986571008+00:00
> I0527 15:55:41.986595 246095872 process.cpp:2094] Resuming 
> __limiter__(2)@10.0.68.255:50644 at 2015-05-27 22:55:41.986610944+00:00
> I0527 15:55:41.986896 243412992 process.cpp:2094] Resuming 
> __latch__(1)@10.0.68.255:50644 at 2015-05-27 22:55:41.986912000+00:00
> I0527 15:55:41.986939 243412992 process.cpp:2201] Cleaning up 
> __latch__(1)@10.0.68.255:50644
> I0527 15:55:41.987043 242876416 process.cpp:2094] Resuming 
> __waiter__(1)@10.0.68.255:50644 at 2015-05-27 22:55:41.987060992+00:00
> I0527 15:55:41.987062 245022720 process.cpp:2094] Resuming 
> __gc__@10.0.68.255:50644 at 2015-05-27 22:55:41.987087104+00:00
> I0527 15:55:41.987117 242876416 process.cpp:2201] Cleaning up 
> __waiter__(1)@10.0.68.255:50644
> I0527 15:55:41.987154 246632448 process.cpp:2094] Resuming 
> (1)@10.0.68.255:50644 at 2015-05-27 22:55:41.987164928+00:00
> I0527 15:55:41.987150 243412992 process.cpp:2094] Resuming 
> __gc__@10.0.68.255:50644 at 2015-05-27 22:55:41.987164928+00:00
> I0527 15:55:41.987179 244486144 process.cpp:2084] Spawned process 
> (1)@10.0.68.255:50644
> W0527 15:55:41.987236 2094736128 monitor.cpp:127] Failed to get resource 
> usage for  container container for executor executor of framework framework: 
> Injected failure
> I0527 15:55:41.987267 246632448 process.cpp:2201] Cleaning up 
> (1)@10.0.68.255:50644
> I0527 15:55:41.987316 242876416 process.cpp:2094] Resuming 
> __gc__@10.0.68.255:50644 at 2015-05-27 22:55:41.987325952+00:00
> I0527 15:55:41.987344 243949568 process.cpp:2094] Resuming 
> __latch__(2)@10.0.68.255:50644 at 2015-05-27 22:55:41.987360000+00:00
> I0527 15:55:41.987383 2094736128 process.cpp:2084] Spawned process 
> __latch__(2)@10.0.68.255:50644
> I0527 15:55:41.987475 2094736128 process.cpp:2084] Spawned process 
> __waiter__(2)@10.0.68.255:50644
> I0527 15:55:41.987481 246632448 process.cpp:2094] Resuming 
> __waiter__(2)@10.0.68.255:50644 at 2015-05-27 22:55:41.987494912+00:00
> I0527 15:55:41.987632 246095872 process.cpp:2094] Resuming 
> __http__(1)@10.0.68.255:50644 at 2015-05-27 22:55:41.987646208+00:00
> I0527 15:55:41.987908 242876416 process.cpp:2094] Resuming 
> __http__(1)@10.0.68.255:50644 at 2015-05-27 22:55:41.987922944+00:00
> I0527 15:55:41.987939 242876416 process.cpp:2201] Cleaning up 
> __http__(1)@10.0.68.255:50644
> I0527 15:55:41.988015 243412992 process.cpp:2094] Resuming 
> __gc__@10.0.68.255:50644 at 2015-05-27 22:55:41.988030976+00:00
> I0527 15:55:41.988276 242876416 process.cpp:2094] Resuming 
> __latch__(2)@10.0.68.255:50644 at 2015-05-27 22:55:41.988286976+00:00
> I0527 15:55:41.988303 242876416 process.cpp:2201] Cleaning up 
> __latch__(2)@10.0.68.255:50644
> I0527 15:55:41.988381 243949568 process.cpp:2094] Resuming 
> __gc__@10.0.68.255:50644 at 2015-05-27 22:55:41.988404992+00:00
> I0527 15:55:41.988381 245022720 process.cpp:2094] Resuming 
> __waiter__(2)@10.0.68.255:50644 at 2015-05-27 22:55:41.988413184+00:00
> I0527 15:55:41.988435 245022720 process.cpp:2201] Cleaning up 
> __waiter__(2)@10.0.68.255:50644
> I0527 15:55:41.988528 245559296 process.cpp:2094] Resuming 
> monitor@10.0.68.255:50644 at 2015-05-27 22:55:41.988540928+00:00
> I0527 15:55:41.988564 245559296 process.cpp:2201] Cleaning up 
> monitor@10.0.68.255:50644
> I0527 15:55:41.988675 245022720 process.cpp:2094] Resuming 
> __limiter__(2)@10.0.68.255:50644 at 2015-05-27 22:55:41.988684032+00:00
> I0527 15:55:41.988703 245022720 process.cpp:2201] Cleaning up 
> __limiter__(2)@10.0.68.255:50644
> [       OK ] MonitorTest.UsageFailure (4 ms)
> [----------] 1 test from MonitorTest (4 ms total)
> 
> 
> Thanks,
> 
> Niklas Nielsen
> 
>

Reply via email to