Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 28, 2015, 2:37 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Changes --- Significant reworking to reflect earlier commits: * Some code was split into an earlier review. * Plenty of renaming and tweaking to match protobuf changes. * Commenting changes. * Testing helpers moved into main test file. 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 (updated) --- Endpoint: /maintenance.schedule Registry operation = maintenance::UpdateSchedule Replaces the schedule with the given one. Also sets all scheduled machines into Draining mode. Diffs (updated) - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 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/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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/#review96762 --- Ship it! src/master/http.cpp (line 1407) https://reviews.apache.org/r/37325/#comment152401 This looks like something we could do during comparison or creating a hash but not here? src/master/http.cpp (line 1421) https://reviews.apache.org/r/37325/#comment152404 How about calling it just 'result' and then do a CHECK since it should always be true? src/master/maintenance.hpp (line 84) https://reviews.apache.org/r/37325/#comment152409 s/interval/unavailability/ src/tests/master_maintenance_tests.cpp (line 76) https://reviews.apache.org/r/37325/#comment152411 maintenance::Schedule schedule = maintenance::createSchedule(...); post( ..., JSON::Protobuf(schedule)); Here and everywhere else please. Also, let's move the 'create*' functions to protobuf_utils.h|cpp and also make them return protobufs that we then do JSON::Protobuf on, thanks! - Benjamin Hindman On Aug. 26, 2015, 9:46 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 26, 2015, 9:46 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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/#review96649 --- Not a full review, mostly nit-picks. Will do a second pass. src/master/http.cpp (line 1366) https://reviews.apache.org/r/37325/#comment152266 s/./// src/master/maintenance.hpp (lines 26 - 27) https://reviews.apache.org/r/37325/#comment152262 Please reverse the order and insert a newline. We sort files alphabetically inside the folder, but nested folders go after files. Moreover, these go before stout/* headers. src/master/maintenance.cpp (line 28) https://reviews.apache.org/r/37325/#comment152271 Libprocess headers go before stout. src/master/maintenance.cpp (line 30) https://reviews.apache.org/r/37325/#comment152270 Put this right after `mesos/type_utils.hpp` please! src/tests/maintenance.hpp (line 50) https://reviews.apache.org/r/37325/#comment152272 `MachineInfos` is (currently) confusing, since there is a protobuf with such name. - Alexander Rukletsov On Aug. 26, 2015, 9:46 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 26, 2015, 9:46 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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
---
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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/#review96567 --- Not a full review, but stopping after the validation routines. Sharing early. src/master/http.cpp (line 1377) https://reviews.apache.org/r/37325/#comment152088 We don't use `.` in our error messages. Do you want to include what value was passed for `method` in the error message? src/master/http.cpp (lines 1383 - 1387) https://reviews.apache.org/r/37325/#comment152091 Can we clean this up with something like: ``` master-maintenanceSchedules.empty() ? mesos::maintenance::Schedule() : master-maintenanceSchedules.front(); ``` Either as `schedule = ` or right in the return statement? src/master/http.cpp (line 1401) https://reviews.apache.org/r/37325/#comment152092 new line between the assignment and the if. We do this if the assignment didn't fit in one line. src/master/http.cpp (line 1410) https://reviews.apache.org/r/37325/#comment152096 new line. src/master/http.cpp (lines 1421 - 1423) https://reviews.apache.org/r/37325/#comment152097 Do you want to use a c++11 lambda here instead? Then you can get rid of `continuation` above. src/master/http.cpp (lines 1435 - 1463) https://reviews.apache.org/r/37325/#comment152102 It's not immediately obvious why you wouldn't just clear the maintenanceStatuses data structure and rebuild it, as opposed to keeping track of changes and updating, then removing the right things. I understand you're doing this because in the future this data structure will be holding information for more than 1 schedule. Can you add a comment here as to why we're going through these steps? src/master/http.cpp (line 1443) https://reviews.apache.org/r/37325/#comment152099 s/Draining/`Draining` src/master/http.cpp (line 1448) https://reviews.apache.org/r/37325/#comment152101 I don't know if we require a new line here. I wouldn't mind one. thoughts? src/master/maintenance.hpp (lines 26 - 27) https://reviews.apache.org/r/37325/#comment152103 is this the right order? src/master/maintenance.hpp (line 37) https://reviews.apache.org/r/37325/#comment152106 Do you want to use Doxygen documentation style since this is a new File? :-D src/master/maintenance.hpp (line 38) https://reviews.apache.org/r/37325/#comment152104 backticks (`) around the modes? src/master/maintenance.hpp (line 39) https://reviews.apache.org/r/37325/#comment152105 s/non-Deactivated machines/machines that are not `Deactivated` src/master/maintenance.hpp (line 60) https://reviews.apache.org/r/37325/#comment152107 I believe we like to indent lists like this src/master/maintenance.hpp (line 61) https://reviews.apache.org/r/37325/#comment152108 Can we choose a different word than numbers? Data? TimeSpecs? src/master/maintenance.cpp (lines 55 - 69) https://reviews.apache.org/r/37325/#comment152109 This code is an example of where we could benefit from a helper function if we had a C++ wrapper for these protos :-) src/master/maintenance.cpp (line 73) https://reviews.apache.org/r/37325/#comment152110 s/MaintenanceStatus/`MaintenanceStatus` src/master/maintenance.cpp (line 84) https://reviews.apache.org/r/37325/#comment152111 s/Draining/`Draining` src/master/maintenance.cpp (lines 93 - 96) https://reviews.apache.org/r/37325/#comment152112 Did you explicitly want to alias here just to be used in the next line? Is this left over from when you were using `machineInfo` elsehwere? src/master/maintenance.cpp (lines 116 - 117) https://reviews.apache.org/r/37325/#comment152115 Can you do a `foreach` here instead and iterate over `mutable_windows()` instead? I think that's the only reason you need `i` right? Larger question: why do we need mutable versions if we're just doing validation? src/master/maintenance.cpp (line 121) https://reviews.apache.org/r/37325/#comment152117 Is it missing, or just empty? src/master/maintenance.cpp (line 124) https://reviews.apache.org/r/37325/#comment152118 Change numbers to something more meaningful? src/master/maintenance.cpp (line 126) https://reviews.apache.org/r/37325/#comment152119 indent 2 spaces for expression continuation. new line before if statement. src/master/maintenance.cpp (line 131) https://reviews.apache.org/r/37325/#comment152120 Collect machines from the updated schedule into a set. src/master/maintenance.cpp (line 132) https://reviews.apache.org/r/37325/#comment152121 Same thing here. Can you use a `foreach` on the mutable version? src/master/maintenance.cpp (line 136) https://reviews.apache.org/r/37325/#comment152123 Is this the right place to perform this mutation? I would expect a validation routine to
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
On Aug. 25, 2015, 6:46 p.m., Joris Van Remoortere wrote: src/tests/maintenance.hpp, lines 78-79 https://reviews.apache.org/r/37325/diff/12/?file=1051209#file1051209line78 Why do we have to fall down to second percision here? Can we take an OptionTimeSpec instead? Is the start time really optional? * For testing, I felt it was enough to have second precision. But it wouldn't hurt to have more. * If we interpret the int64_t as nanoseconds, then we'd get both fields. At the same time, it'll match how we represent time (internally) in the Duration class. The only downside is that we'd be writing a lot more zeros in the tests. * I think this is a mistake on my part. In Maintenance.proto (https://reviews.apache.org/r/36571/), I set the unavailability field in the Maintenance window as optional. I'll make these changes momentarily. - Joseph --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/#review96474 --- On Aug. 25, 2015, 10:03 a.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 25, 2015, 10:03 a.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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
On Aug. 26, 2015, 12:03 p.m., Joris Van Remoortere wrote: src/master/http.cpp, line 1448 https://reviews.apache.org/r/37325/diff/12/?file=1051203#file1051203line1448 I don't know if we require a new line here. I wouldn't mind one. thoughts? It'll run over 80 characters without the newline. On Aug. 26, 2015, 12:03 p.m., Joris Van Remoortere wrote: src/master/maintenance.hpp, line 37 https://reviews.apache.org/r/37325/diff/12/?file=1051204#file1051204line37 Do you want to use Doxygen documentation style since this is a new File? :-D Completely forgot about that XD. On Aug. 26, 2015, 12:03 p.m., Joris Van Remoortere wrote: src/master/maintenance.cpp, lines 93-96 https://reviews.apache.org/r/37325/diff/12/?file=1051205#file1051205line93 Did you explicitly want to alias here just to be used in the next line? Is this left over from when you were using `machineInfo` elsehwere? I believe this was a leftover. I'll remove it. On Aug. 26, 2015, 12:03 p.m., Joris Van Remoortere wrote: src/master/maintenance.cpp, line 124 https://reviews.apache.org/r/37325/diff/12/?file=1051205#file1051205line124 Change numbers to something more meaningful? This was literally when we were check the double for being not Not-a-Number :) I'll update it. On Aug. 26, 2015, 12:03 p.m., Joris Van Remoortere wrote: src/master/maintenance.cpp, line 136 https://reviews.apache.org/r/37325/diff/12/?file=1051205#file1051205line136 Is this the right place to perform this mutation? I would expect a validation routine to be side-effect free. I believe this is also the root of why you need mutable versions everywhere? Can we find a better place to do this, and just verify that the hostnames passed in are indeed lowercase? Added a bunch of `lowercaseHostname` helper functions. - Joseph --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/#review96567 --- On Aug. 26, 2015, 2:16 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 26, 2015, 2:16 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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 26, 2015, 2:16 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Changes --- Fixed a few stale comments. 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 (updated) - 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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 26, 2015, 2:46 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Changes --- Change endpoints to use slashes, since it is now supported. 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 (updated) - 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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
On Aug. 26, 2015, 7:03 p.m., Joris Van Remoortere wrote: src/master/maintenance.hpp, lines 26-27 https://reviews.apache.org/r/37325/diff/12/?file=1051204#file1051204line26 is this the right order? I think this was the right order : ). With a newline in-between, it could have become a perfect one. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/#review96567 --- On Aug. 26, 2015, 9:46 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 26, 2015, 9:46 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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
On Aug. 26, 2015, 7:03 p.m., Joris Van Remoortere wrote: src/master/maintenance.cpp, lines 55-69 https://reviews.apache.org/r/37325/diff/12/?file=1051205#file1051205line55 This code is an example of where we could benefit from a helper function if we had a C++ wrapper for these protos :-) Simplifying the structure of protos can do the job as well. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/#review96567 --- On Aug. 26, 2015, 9:46 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 26, 2015, 9:46 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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 25, 2015, 10:03 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Changes --- Adjust some spacing in the tests (correcting the style). 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 (updated) - 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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 25, 2015, 9:10 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Changes --- Update Unavailability to use integer precision. Changes the two Maintenance.hpp files (one for validation and one as a test helper). 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 (updated) - 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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/#review96308 --- Ship it! Ship It! - Guangya Liu On Aug. 24, 2015, 6:48 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 24, 2015, 6:48 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 0432842d77beba024c7895291ca410964bae96be 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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/#review96474 --- src/tests/maintenance.hpp (lines 78 - 79) https://reviews.apache.org/r/37325/#comment151872 Why do we have to fall down to second percision here? Can we take an OptionTimeSpec instead? Is the start time really optional? - 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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 24, 2015, 11:48 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Changes --- Rebased and updated diff. No other changes. 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 (updated) - 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 0432842d77beba024c7895291ca410964bae96be 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
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
On Aug. 12, 2015, 10:31 a.m., Joseph Wu wrote: src/master/maintenance.cpp, lines 126-130 https://reviews.apache.org/r/37325/diff/2/?file=1037747#file1037747line126 Note: Considering the machine field in the master's local state (https://reviews.apache.org/r/37170/diff/2#0.8), it may be helpful/necessary to fill in the hostname/ip for some machines before doing this check. For now, we'll document that Machine(A,B) != Machine(A), even if A resolves to B. - Joseph --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/#review95125 --- On Aug. 24, 2015, 11:48 a.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 24, 2015, 11:48 a.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 0432842d77beba024c7895291ca410964bae96be 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
