You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2015/08/24 20:48:49 UTC
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.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Aug. 26, 2015, 11:05 p.m., Alexander Rukletsov wrote:
> > Not a full review, mostly nit-picks. Will do a second pass.
Awesome. Thanks!
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37325/#review96649
-----------------------------------------------------------
On Aug. 28, 2015, 2:37 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> 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.
>
>
> Diffs
> -----
>
> 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.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37325/
-----------------------------------------------------------
(Updated Aug. 26, 2015, 2:15 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
Changes
-------
* Rename some local variable to match prior reviews.
* Change maintenance test helpers to take Duration to construct Unavailability.
* Changed validation to not mutate object. Added lowercase helper.
* Lots and lots of renaming + spacing changes.
* Some modification of how some loops are done.
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.
Posted by Joseph Wu <jo...@mesosphere.io>.
> 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 Option<TimeSpec> 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.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
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 Option<TimeSpec> 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.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for
scheduling agents for maintenance.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> 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.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> 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.
Posted by Joseph Wu <jo...@mesosphere.io>.
> 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.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Aug. 26, 2015, 7: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?
>
> Joseph Wu wrote:
> It'll run over 80 characters without the newline.
Sorry, i meant between statements. Not to go over 80 characters.
- Joris
-----------------------------------------------------------
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.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
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 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?
src/master/maintenance.cpp (line 139)
<https://reviews.apache.org/r/37325/#comment152122>
new line after multi-line expression.
src/master/maintenance.cpp (line 143)
<https://reviews.apache.org/r/37325/#comment152124>
`Check that the machine is unique.` ?
src/master/maintenance.cpp (line 145)
<https://reviews.apache.org/r/37325/#comment152129>
can you add a single-quote around the Machine name:
`'" + machineInfo.DebugString() + "' is`
src/master/maintenance.cpp (line 146)
<https://reviews.apache.org/r/37325/#comment152125>
2 space indent for expression continuation.
src/master/maintenance.cpp (line 153)
<https://reviews.apache.org/r/37325/#comment152127>
How about:
`Ensure no `Deactivated` machine is removed from the schedule.` ?
src/master/maintenance.cpp (line 154)
<https://reviews.apache.org/r/37325/#comment152126>
s/Deactivated/`Deactivated`
src/master/maintenance.cpp (line 157)
<https://reviews.apache.org/r/37325/#comment152128>
can you add a single-quote around the Machine name:
`'" + key.DebugString() + "' is`
src/master/maintenance.cpp (line 158)
<https://reviews.apache.org/r/37325/#comment152130>
2 space indent for expression continuation.
src/master/maintenance.cpp (lines 170 - 172)
<https://reviews.apache.org/r/37325/#comment152131>
2 space indent for expression continuation.
src/master/maintenance.cpp (line 176)
<https://reviews.apache.org/r/37325/#comment152132>
Maybe add the value of the data to the message?
Can we find something better than `numbers`?
src/master/maintenance.cpp (line 185)
<https://reviews.apache.org/r/37325/#comment152133>
Comments end with a `.`
Let's try and do this mutation elsewhere and just verify that the name is indeed lowercase here.
That way we can get rid of the mutable aspects throughout this entire validation section.
- 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.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
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