Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 11, 2015, 10:57 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Change endpoint name in master to api/v1/scheduler Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs (updated) - src/master/master.hpp bb7c8e9e05be829a6b9aa3100a714b2359854d96 src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/#review94941 --- Looking. Minor issues. Please make sure when you fix an issue, you fix it in all the tests. src/tests/http_api_tests.cpp (lines 72 - 93) https://reviews.apache.org/r/37082/#comment149653 Can you add a TODO to move these out into common helpers? src/tests/http_api_tests.cpp (lines 141 - 143) https://reviews.apache.org/r/37082/#comment149655 Instead of repeating this comment and these 2 lines in every test, add a CreaterMasterFlags() overload to the fixture. src/tests/http_api_tests.cpp (line 178) https://reviews.apache.org/r/37082/#comment149654 do you need the temp variable here? here and subsequent tests. src/tests/http_api_tests.cpp (lines 199 - 200) https://reviews.apache.org/r/37082/#comment149667 ditto. src/tests/http_api_tests.cpp (line 221) https://reviews.apache.org/r/37082/#comment149656 s/subcribedId/frameworkId/ src/tests/http_api_tests.cpp (line 243) https://reviews.apache.org/r/37082/#comment149657 kill this. ASSERT_SOME() guarantees that it is not an error. src/tests/http_api_tests.cpp (line 245) https://reviews.apache.org/r/37082/#comment149662 ditto. src/tests/http_api_tests.cpp (lines 248 - 249) https://reviews.apache.org/r/37082/#comment149658 swap the order of the arguments. when writing ASSERT, EXPECT the convention is that the first argument should be the expected value and the second one is the actual value. this is because of the way gmock prints the error message. please fix this here and everywhere else in this file. src/tests/http_api_tests.cpp (line 275) https://reviews.apache.org/r/37082/#comment149660 kill this. src/tests/http_api_tests.cpp (lines 279 - 280) https://reviews.apache.org/r/37082/#comment149661 ditto. src/tests/http_api_tests.cpp (line 287) https://reviews.apache.org/r/37082/#comment149664 s/pid/PID/ src/tests/http_api_tests.cpp (line 288) https://reviews.apache.org/r/37082/#comment149666 s/http/HTTP/ src/tests/http_api_tests.cpp (lines 292 - 293) https://reviews.apache.org/r/37082/#comment149668 ditto. src/tests/http_api_tests.cpp (line 310) https://reviews.apache.org/r/37082/#comment149669 s/http/HTTP/ src/tests/http_api_tests.cpp (line 322) https://reviews.apache.org/r/37082/#comment149670 s/a/an/ src/tests/http_api_tests.cpp (line 357) https://reviews.apache.org/r/37082/#comment149671 ditto. src/tests/http_api_tests.cpp (lines 366 - 367) https://reviews.apache.org/r/37082/#comment149672 ditto. src/tests/http_api_tests.cpp (line 406) https://reviews.apache.org/r/37082/#comment149673 s/'force'/the 'force'/ - Vinod Kone On Aug. 11, 2015, 5:03 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 11, 2015, 5:03 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs - src/master/master.hpp 10cc100b1cf10aa7a8a9979a2562a9c5a9e55bd6 src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 11, 2015, 5:03 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Change to use the RecordIO Decoder + Minor cleanup of tests Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs (updated) - src/master/master.hpp 10cc100b1cf10aa7a8a9979a2562a9c5a9e55bd6 src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 11, 2015, 8:11 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Review comments Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs (updated) - src/master/master.hpp 6bd05b1f364affeb4f23baa7cdf3a0d00c45a2c6 src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/#review95052 --- Ship it! Ship It! - Vinod Kone On Aug. 11, 2015, 10:57 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 11, 2015, 10:57 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs - src/master/master.hpp bb7c8e9e05be829a6b9aa3100a714b2359854d96 src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/#review94568 --- Ship it! Ship It! - Vinod Kone On Aug. 7, 2015, 2:56 a.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 7, 2015, 2:56 a.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs - src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 7, 2015, 2:56 a.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Review comments by Vinod Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs (updated) - src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
On Aug. 7, 2015, 12:21 a.m., Vinod Kone wrote: src/tests/http_api_tests.cpp, line 187 https://reviews.apache.org/r/37082/diff/3-4/?file=1032065#file1032065line187 No CHECK's in test code please. It will crash the program. Use ASSERT_SOME() instead. here and everywhere else. Whoops, my bad. Fixed. - Anand --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/#review94468 --- On Aug. 6, 2015, 4:26 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 6, 2015, 4:26 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs - src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/#review94468 --- src/tests/http_api_tests.cpp (line 182) https://reviews.apache.org/r/37082/#comment149089 AWAIT_READY guarantees that event.isReady(). no need for this check? here and everywhere. src/tests/http_api_tests.cpp (line 183) https://reviews.apache.org/r/37082/#comment149091 No CHECK's in test code please. It will crash the program. Use ASSERT_SOME() instead. here and everywhere else. - Vinod Kone On Aug. 6, 2015, 4:26 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 6, 2015, 4:26 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs - src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/#review94422 --- Patch looks great! Reviews applied: [37080, 36720, 37082] All tests passed. - Mesos ReviewBot On Aug. 6, 2015, 4:26 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 6, 2015, 4:26 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs - src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 6, 2015, 4:26 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Rerun reviewbot again with the enum fix for gcc in r36720. Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs - src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
On Aug. 5, 2015, 11:52 p.m., Vinod Kone wrote: src/tests/http_api_tests.cpp, line 145 https://reviews.apache.org/r/37082/diff/3/?file=1032065#file1032065line145 don't need to do this anymore now that the default framework info sets the user, right? Yep, it got committed yesterday. Fixed :) - Anand --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/#review94292 --- On Aug. 5, 2015, midnight, Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 5, 2015, midnight) Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs - src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/#review94292 --- Tests look good! Mainly stylistic issues and generous use of 'auto'. Please make sure you fix all the tests when you fix a given issue (I might not have caught all of them in all the tests). src/tests/http_api_tests.cpp (line 69) https://reviews.apache.org/r/37082/#comment148841 I would have this return a TryEvent src/tests/http_api_tests.cpp (line 73) https://reviews.apache.org/r/37082/#comment148842 what if this fails? see above: this should return an Error. src/tests/http_api_tests.cpp (line 79) https://reviews.apache.org/r/37082/#comment148843 what if this fails? just return parse here. src/tests/http_api_tests.cpp (line 91) https://reviews.apache.org/r/37082/#comment148840 2 blank lines between outer elements. src/tests/http_api_tests.cpp (line 97) https://reviews.apache.org/r/37082/#comment148844 2 blank lines. src/tests/http_api_tests.cpp (line 126) https://reviews.apache.org/r/37082/#comment148845 2 blank lines. src/tests/http_api_tests.cpp (line 127) https://reviews.apache.org/r/37082/#comment148848 s/client/scheduler/ src/tests/http_api_tests.cpp (lines 135 - 136) https://reviews.apache.org/r/37082/#comment148850 Put this line close to where it is used, above #150. src/tests/http_api_tests.cpp (line 145) https://reviews.apache.org/r/37082/#comment148867 don't need to do this anymore now that the default framework info sets the user, right? src/tests/http_api_tests.cpp (line 164) https://reviews.apache.org/r/37082/#comment148868 why auto? it's not clear at all what the return type is here. src/tests/http_api_tests.cpp (line 174) https://reviews.apache.org/r/37082/#comment148870 ditto. no auto please. src/tests/http_api_tests.cpp (lines 185 - 186) https://reviews.apache.org/r/37082/#comment148877 This comment and test name is misleading because this is not testing failover. This test is just testing subscription retry (simulating the situation of a ZK blip). Mind fixing it? src/tests/http_api_tests.cpp (line 192) https://reviews.apache.org/r/37082/#comment148885 ditto. src/tests/http_api_tests.cpp (line 201) https://reviews.apache.org/r/37082/#comment148872 ditto. src/tests/http_api_tests.cpp (line 219) https://reviews.apache.org/r/37082/#comment148873 ditto. src/tests/http_api_tests.cpp (line 229) https://reviews.apache.org/r/37082/#comment148874 ditto. src/tests/http_api_tests.cpp (line 238) https://reviews.apache.org/r/37082/#comment148878 new line here. src/tests/http_api_tests.cpp (line 244) https://reviews.apache.org/r/37082/#comment148876 ditto. src/tests/http_api_tests.cpp (line 251) https://reviews.apache.org/r/37082/#comment148879 ditto. src/tests/http_api_tests.cpp (line 258) https://reviews.apache.org/r/37082/#comment148880 this is not a failover. please rephrase. src/tests/http_api_tests.cpp (lines 265 - 266) https://reviews.apache.org/r/37082/#comment148881 EXPECT_EQ(arg1, arg2); src/tests/http_api_tests.cpp (line 273) https://reviews.apache.org/r/37082/#comment148882 s/http/HTTP/ src/tests/http_api_tests.cpp (line 281) https://reviews.apache.org/r/37082/#comment148883 ditto. src/tests/http_api_tests.cpp (line 285) https://reviews.apache.org/r/37082/#comment148884 ditto. src/tests/http_api_tests.cpp (line 299) https://reviews.apache.org/r/37082/#comment148887 s/http/HTTP/ src/tests/http_api_tests.cpp (line 311) https://reviews.apache.org/r/37082/#comment14 s/http/HTTP/ src/tests/http_api_tests.cpp (lines 317 - 318) https://reviews.apache.org/r/37082/#comment148889 reorder. src/tests/http_api_tests.cpp (lines 351 - 353) https://reviews.apache.org/r/37082/#comment148891 you don't need to pull out value. we have equality operator defined for FrameworkID. also alignment. src/tests/http_api_tests.cpp (line 362) https://reviews.apache.org/r/37082/#comment148892 s/pid/PID/ s/http/HTTP/ src/tests/http_api_tests.cpp (line 365) https://reviews.apache.org/r/37082/#comment148894 s/UpdatePidToHttpSchedulerForceNotSetFailure/UpdatePidToHttpSchedulerWithoutForce/ src/tests/http_api_tests.cpp (line 376) https://reviews.apache.org/r/37082/#comment148895 ditto. src/tests/http_api_tests.cpp (line 395) https://reviews.apache.org/r/37082/#comment148896 s/http/HTTP/ s/framework/framework without setting 'force' field/ src/tests/http_api_tests.cpp (line 419) https://reviews.apache.org/r/37082/#comment148897 ditto. src/tests/http_api_tests.cpp (line 429) https://reviews.apache.org/r/37082/#comment148898 ditto. src/tests/http_api_tests.cpp (line
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 4, 2015, 10:02 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Rebased from master after Ben's latest changes. Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs (updated) - src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 5, 2015, midnight) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Rebased from master. Also fixed tests that broke after we introduced the validation logic. Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs (updated) - src/tests/http_api_tests.cpp 586d11288828fe9997e54f5dbd7d28c200e973f5 Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
Re: Review Request 37082: Tests for subscribe/failover functionality for http based framework
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37082/ --- (Updated Aug. 4, 2015, 5:15 p.m.) Review request for mesos, Ben Mahler and Vinod Kone. Changes --- Updated depends on Repository: mesos Description --- This implements the tests for http framework subscribe/failover/upgrade from a pid based framework. The test are parameterized on content type and hence test both json/protobuf responses. Diffs - src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac Diff: https://reviews.apache.org/r/37082/diff/ Testing --- make check Thanks, Anand Mazumdar
