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:43:27 UTC
Re: Review Request 37314: Maintenance Primitives: Populate master's
maintenance-related local state upon recovery from registry.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/
-----------------------------------------------------------
(Updated Aug. 24, 2015, 11:43 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-3045
https://issues.apache.org/jira/browse/MESOS-3045
Repository: mesos
Description
-------
Note: Tests will be added with the related HTTP endpoints and registrar operations, later in this review chain.
Diffs (updated)
-----
include/mesos/maintenance/maintenance.hpp PRE-CREATION
include/mesos/type_utils.hpp dafe1df0cb5d0b83ca0579068916fe7fda848f02
src/master/master.hpp 0432842d77beba024c7895291ca410964bae96be
src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
Diff: https://reviews.apache.org/r/37314/diff/
Testing
-------
`make check`
Thanks,
Joseph Wu
Re: Review Request 37314: Maintenance Primitives: Populate master's
maintenance-related local state upon recovery from registry.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/#review96302
-----------------------------------------------------------
Ship it!
Ship It!
- Guangya Liu
On Aug. 24, 2015, 6:43 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37314/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2015, 6:43 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-3045
> https://issues.apache.org/jira/browse/MESOS-3045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Note: Tests will be added with the related HTTP endpoints and registrar operations, later in this review chain.
>
>
> Diffs
> -----
>
> include/mesos/maintenance/maintenance.hpp PRE-CREATION
> include/mesos/type_utils.hpp dafe1df0cb5d0b83ca0579068916fe7fda848f02
> src/master/master.hpp 0432842d77beba024c7895291ca410964bae96be
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
>
> Diff: https://reviews.apache.org/r/37314/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 37314: Maintenance Primitives: Populate master's
maintenance-related local state upon recovery from registry.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/#review96748
-----------------------------------------------------------
Ship it!
include/mesos/type_utils.hpp (lines 152 - 153)
<https://reviews.apache.org/r/37314/#comment152386>
Let's check for the presence of the fields first see other examples below.
src/master/master.hpp (lines 893 - 896)
<https://reviews.apache.org/r/37314/#comment152388>
hashmap<MachineID, MachineInfo> machines;
struct {
std::list<...> schedules;
} maintenance;
maintenance.schedules...;
- Benjamin Hindman
On Aug. 26, 2015, 6:49 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37314/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2015, 6:49 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-3045
> https://issues.apache.org/jira/browse/MESOS-3045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Note: Tests will be added with the related HTTP endpoints and registrar operations, later in this review chain.
>
>
> Diffs
> -----
>
> include/mesos/maintenance/maintenance.hpp PRE-CREATION
> include/mesos/type_utils.hpp dafe1df0cb5d0b83ca0579068916fe7fda848f02
> src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
>
> Diff: https://reviews.apache.org/r/37314/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 37314: Maintenance Primitives: Populate master's
maintenance-related local state upon recovery from registry.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Aug. 26, 2015, 11:05 p.m., Alexander Rukletsov wrote:
> > include/mesos/maintenance/maintenance.hpp, line 29
> > <https://reviews.apache.org/r/37314/diff/6/?file=1053339#file1053339line29>
> >
> > If it's not a protobuf, remove `Info` suffix. The suffix is there only to avoid collisions between proto and non-proto classes. Thanks!
This was merged into the new and improved `MachineInfo` protobuf. (Same for your other points :).
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/#review96643
-----------------------------------------------------------
On Aug. 27, 2015, 6:07 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37314/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 6:07 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-3045
> https://issues.apache.org/jira/browse/MESOS-3045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Note: Tests will be added with the related HTTP endpoints and registrar operations, later in this review chain.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 8d27ef439d8bfb30a10b2f32fcd893f0af08ed75
> src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
>
> Diff: https://reviews.apache.org/r/37314/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 37314: Maintenance Primitives: Populate master's
maintenance-related local state upon recovery from registry.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/#review96643
-----------------------------------------------------------
include/mesos/maintenance/maintenance.hpp (line 29)
<https://reviews.apache.org/r/37314/#comment152246>
If it's not a protobuf, remove `Info` suffix. The suffix is there only to avoid collisions between proto and non-proto classes. Thanks!
include/mesos/maintenance/maintenance.hpp (lines 29 - 38)
<https://reviews.apache.org/r/37314/#comment152249>
I'm getting slightly confused with multiple `Maintenance*` messages and structs : ). How about we combine all similar structs into one protobuf and will re-use it everywhere? Decomposition and microkernels are great, as soon as there not so many connections between shards.
How about a message in `maintenance.proto`:
```
message MaintenanceInfo {
required MachineInfo = 1;
optional Unavailability unavailability = 2
optional Mode = 3;
}
```
which then substitutes `MaintenanceStatus` and this struct here? Moreover, it can also replace `Window`. Recovery code will become simpler as well.
include/mesos/maintenance/maintenance.hpp (line 36)
<https://reviews.apache.org/r/37314/#comment152247>
"Window" in the comment is misleading, as there is a protobuf message, which is for multiple machines.
Also, shouldn't `MachineInfo` be here as well? Can be handy, once an element is extracted from the hash.
- Alexander Rukletsov
On Aug. 26, 2015, 6:49 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37314/
> -----------------------------------------------------------
>
> (Updated Aug. 26, 2015, 6:49 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-3045
> https://issues.apache.org/jira/browse/MESOS-3045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Note: Tests will be added with the related HTTP endpoints and registrar operations, later in this review chain.
>
>
> Diffs
> -----
>
> include/mesos/maintenance/maintenance.hpp PRE-CREATION
> include/mesos/type_utils.hpp dafe1df0cb5d0b83ca0579068916fe7fda848f02
> src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
>
> Diff: https://reviews.apache.org/r/37314/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 37314: Maintenance Primitives: Populate master's
maintenance-related local state upon recovery from registry.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/
-----------------------------------------------------------
(Updated Aug. 28, 2015, 10:38 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
Changes
-------
Rebased.
Bugs: MESOS-3045
https://issues.apache.org/jira/browse/MESOS-3045
Repository: mesos
Description
-------
Note: Tests will be added with the related HTTP endpoints and registrar operations, later in this review chain.
Diffs (updated)
-----
include/mesos/type_utils.hpp 1c8f95b894285140a228ab4202851ee391ffdcc6
src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
Diff: https://reviews.apache.org/r/37314/diff/
Testing
-------
`make check`
Thanks,
Joseph Wu
Re: Review Request 37314: Maintenance Primitives: Populate master's
maintenance-related local state upon recovery from registry.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Aug. 28, 2015, 8:19 a.m., Joris Van Remoortere wrote:
> > include/mesos/type_utils.hpp, line 158
> > <https://reviews.apache.org/r/37314/diff/7/?file=1057072#file1057072line158>
> >
> > You have this on a new line to call it out for readability right? It would fit above otherwise.
Yup, that's right.
> On Aug. 28, 2015, 8:19 a.m., Joris Van Remoortere wrote:
> > include/mesos/type_utils.hpp, line 593
> > <https://reviews.apache.org/r/37314/diff/7/?file=1057072#file1057072line593>
> >
> > You don't need the `std::` in `std::size_t`. I know you followed the pattern that got committed recently, it got followed up with this change :-)
Got it. I didn't see the change until I rebased a few moments ago.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/#review96870
-----------------------------------------------------------
On Aug. 28, 2015, 10:38 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37314/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 10:38 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-3045
> https://issues.apache.org/jira/browse/MESOS-3045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Note: Tests will be added with the related HTTP endpoints and registrar operations, later in this review chain.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 1c8f95b894285140a228ab4202851ee391ffdcc6
> src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
>
> Diff: https://reviews.apache.org/r/37314/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 37314: Maintenance Primitives: Populate master's
maintenance-related local state upon recovery from registry.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/#review96870
-----------------------------------------------------------
Ship it!
include/mesos/type_utils.hpp (line 153)
<https://reviews.apache.org/r/37314/#comment152557>
s/Note\:/NOTE\:/
include/mesos/type_utils.hpp (line 158)
<https://reviews.apache.org/r/37314/#comment152558>
You have this on a new line to call it out for readability right? It would fit above otherwise.
include/mesos/type_utils.hpp (line 593)
<https://reviews.apache.org/r/37314/#comment152559>
You don't need the `std::` in `std::size_t`. I know you followed the pattern that got committed recently, it got followed up with this change :-)
- Joris Van Remoortere
On Aug. 28, 2015, 1:07 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37314/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2015, 1:07 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-3045
> https://issues.apache.org/jira/browse/MESOS-3045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Note: Tests will be added with the related HTTP endpoints and registrar operations, later in this review chain.
>
>
> Diffs
> -----
>
> include/mesos/type_utils.hpp 8d27ef439d8bfb30a10b2f32fcd893f0af08ed75
> src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
>
> Diff: https://reviews.apache.org/r/37314/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 37314: Maintenance Primitives: Populate master's
maintenance-related local state upon recovery from registry.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/
-----------------------------------------------------------
(Updated Aug. 27, 2015, 6:07 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
Changes
-------
Rebase and change the `==` operator and hashing function. Also match changes to protobufs, from previous reviews.
Bugs: MESOS-3045
https://issues.apache.org/jira/browse/MESOS-3045
Repository: mesos
Description
-------
Note: Tests will be added with the related HTTP endpoints and registrar operations, later in this review chain.
Diffs (updated)
-----
include/mesos/type_utils.hpp 8d27ef439d8bfb30a10b2f32fcd893f0af08ed75
src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
Diff: https://reviews.apache.org/r/37314/diff/
Testing
-------
`make check`
Thanks,
Joseph Wu
Re: Review Request 37314: Maintenance Primitives: Populate master's
maintenance-related local state upon recovery from registry.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/
-----------------------------------------------------------
(Updated Aug. 26, 2015, 11:49 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
Changes
-------
Commenting, spacing, and renaming of a local variable.
Bugs: MESOS-3045
https://issues.apache.org/jira/browse/MESOS-3045
Repository: mesos
Description
-------
Note: Tests will be added with the related HTTP endpoints and registrar operations, later in this review chain.
Diffs (updated)
-----
include/mesos/maintenance/maintenance.hpp PRE-CREATION
include/mesos/type_utils.hpp dafe1df0cb5d0b83ca0579068916fe7fda848f02
src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc
src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
Diff: https://reviews.apache.org/r/37314/diff/
Testing
-------
`make check`
Thanks,
Joseph Wu
Re: Review Request 37314: Maintenance Primitives: Populate master's
maintenance-related local state upon recovery from registry.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/#review96548
-----------------------------------------------------------
include/mesos/maintenance/maintenance.hpp (line 29)
<https://reviews.apache.org/r/37314/#comment152070>
`{` on new line
include/mesos/type_utils.hpp (line 146)
<https://reviews.apache.org/r/37314/#comment152018>
Can you add another copy of the comment regarding How MachineInfos are expected to be matched here?
I know this seems like overkill, but the IP <-> Hostname thing bites a lot of people. Let's take advantage of as many opportunities as possible to help them out if they start browsing the code.
include/mesos/type_utils.hpp (line 149)
<https://reviews.apache.org/r/37314/#comment152069>
expression continuation should be indented by 2, not 4.
src/master/master.hpp (line 893)
<https://reviews.apache.org/r/37314/#comment152074>
Here is another place where the wording is a little inconsistent.
We can call it `maintenanceInfos` or change the type name. Let's try and follow the variable name matching the type name when possible.
- Joris Van Remoortere
On Aug. 24, 2015, 6:43 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37314/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2015, 6:43 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-3045
> https://issues.apache.org/jira/browse/MESOS-3045
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Note: Tests will be added with the related HTTP endpoints and registrar operations, later in this review chain.
>
>
> Diffs
> -----
>
> include/mesos/maintenance/maintenance.hpp PRE-CREATION
> include/mesos/type_utils.hpp dafe1df0cb5d0b83ca0579068916fe7fda848f02
> src/master/master.hpp 0432842d77beba024c7895291ca410964bae96be
> src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf
>
> Diff: https://reviews.apache.org/r/37314/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Joseph Wu
>
>