----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/#review96581 -----------------------------------------------------------
src/tests/maintenance.hpp (line 37) <https://reviews.apache.org/r/37325/#comment152138> should this come before the namespace declarations? src/tests/maintenance.hpp (lines 55 - 56) <https://reviews.apache.org/r/37325/#comment152150> I don't believe we need 2 spaces between functions that are defined inside the class like this. Here and below. src/tests/maintenance.hpp (lines 63 - 66) <https://reviews.apache.org/r/37325/#comment152141> How about using a `foreach`? If it doesn't work on initializer list, mpark has offered to fix this. src/tests/maintenance.hpp (line 70) <https://reviews.apache.org/r/37325/#comment152143> `std::move(array)` ? src/tests/maintenance.hpp (lines 83 - 84) <https://reviews.apache.org/r/37325/#comment152145> I think you're fixing this in an upcoming diff? src/tests/maintenance.hpp (line 85) <https://reviews.apache.org/r/37325/#comment152147> how about `seconds`? I'm guessing this function will be refactored in your next diff. src/tests/maintenance.hpp (line 109) <https://reviews.apache.org/r/37325/#comment152149> `foreach` (as above) src/tests/master_maintenance_tests.cpp (line 28) <https://reviews.apache.org/r/37325/#comment152151> reorder src/tests/master_maintenance_tests.cpp (line 75) <https://reviews.apache.org/r/37325/#comment152152> new line. src/tests/master_maintenance_tests.cpp (line 89) <https://reviews.apache.org/r/37325/#comment152154> s/masterBlob/masterSchedule_ ? src/tests/master_maintenance_tests.cpp (line 93) <https://reviews.apache.org/r/37325/#comment152155> `ASSERT_SOME(masterSchedule);` src/tests/master_maintenance_tests.cpp (line 95) <https://reviews.apache.org/r/37325/#comment152153> ``` ASSERT_EQ( "machine1", masterSchedule.get().windows(0).machine_infos(0).hostname()); ``` src/tests/master_maintenance_tests.cpp (lines 108 - 112) <https://reviews.apache.org/r/37325/#comment152156> ``` schedule = createMaintenanceSchedule({ createMaintenanceWindow({machine1}), createMaintenanceWindow({machine1})}); ``` src/tests/master_maintenance_tests.cpp (line 113) <https://reviews.apache.org/r/37325/#comment152157> new line before this assignment. src/tests/master_maintenance_tests.cpp (line 123) <https://reviews.apache.org/r/37325/#comment152159> new line src/tests/master_maintenance_tests.cpp (line 133) <https://reviews.apache.org/r/37325/#comment152160> new line src/tests/registrar_tests.cpp (line 323) <https://reviews.apache.org/r/37325/#comment152161> `NOTE:` src/tests/registrar_tests.cpp (line 326) <https://reviews.apache.org/r/37325/#comment152163> s/into blocks/into scoped blocks ? src/tests/registrar_tests.cpp (line 333) <https://reviews.apache.org/r/37325/#comment152164> machine1, etc. src/tests/registrar_tests.cpp (lines 343 - 353) <https://reviews.apache.org/r/37325/#comment152166> I would be tempted to introduce a helper function on RegistrarTest that takes a lambda with the body of your test. I think this would de-clutter the code some, and avoid the arguments around un-named scoped blocks. Before you do this though, I'd like to verify with BenH that this would alright: ``` RegistrarTest(flags, state, [=](const Registry& registry) { // body of test, eg: EXPECT_EQ(1, registry.schedules().size()); }); ``` The main concern with this pattern is that we can't do any *ASSERT* like functions in the helper / lambda. Only *EXPECT* like functions. - Joris Van Remoortere On Aug. 25, 2015, 5:03 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37325/ > ----------------------------------------------------------- > > (Updated Aug. 25, 2015, 5:03 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, > Joris Van Remoortere, and Vinod Kone. > > > Bugs: MESOS-2067 and MESOS-3069 > https://issues.apache.org/jira/browse/MESOS-2067 > https://issues.apache.org/jira/browse/MESOS-3069 > > > Repository: mesos > > > Description > ------- > > Endpoint: /maintenance.schedule > > Registry operation = maintenance::UpdateSchedule > Replaces the schedule with the given one. Also sets all scheduled machines > into Draining mode. > > Other changes: > Added a note about the "strict" flag. > > > Diffs > ----- > > src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db > src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44 > src/master/maintenance.hpp PRE-CREATION > src/master/maintenance.cpp PRE-CREATION > src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc > src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf > src/master/registrar.hpp c6a0655c212646618d93c9c85918af482a9ffd50 > src/tests/maintenance.hpp PRE-CREATION > src/tests/master_maintenance_tests.cpp PRE-CREATION > src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166 > > Diff: https://reviews.apache.org/r/37325/diff/ > > > Testing > ------- > > `make check` > > New Tests: > RegistrarTest.UpdateMaintenanceSchedule > Schedules 3 machines, 1 at a time. Rearranges schedules. > Checks that machines are put into Draining mode. Removes machines. > MasterMaintenanceTest.UpdateSchedule > Hits the new endpoint with some valid and invalid schedules. > Only tests a subset of invalid schedules (requires other endpoints to > fully test). > > > Thanks, > > Joseph Wu > >
