Re: Review Request 37314: Maintenance Primitives: Populate master's maintenance-related local state upon recovery from registry.

2015-08-28 Thread Joseph Wu 


> On Aug. 28, 2015, 8:19 a.m., Joris Van Remoortere wrote:
> > include/mesos/type_utils.hpp, line 158
> > 
> >
> > 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
> > 
> >
> > 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.

2015-08-28 Thread Joseph Wu 

---
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.

2015-08-28 Thread Joris Van Remoortere 

---
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)


s/Note\:/NOTE\:/



include/mesos/type_utils.hpp (line 158)


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)


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.

2015-08-27 Thread Joseph Wu 


> On Aug. 26, 2015, 11:05 p.m., Alexander Rukletsov wrote:
> > include/mesos/maintenance/maintenance.hpp, line 29
> > 
> >
> > 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.

2015-08-27 Thread Joseph Wu 

---
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.

2015-08-27 Thread Benjamin Hindman 

---
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)


Let's check for the presence of the fields first see other examples below.



src/master/master.hpp (lines 893 - 896)


hashmap 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.

2015-08-26 Thread Alexander Rukletsov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/#review96643
---



include/mesos/maintenance/maintenance.hpp (line 29)


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)


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)


"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.

2015-08-26 Thread Joseph Wu 

---
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.

2015-08-26 Thread Joris Van Remoortere 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37314/#review96548
---



include/mesos/maintenance/maintenance.hpp (line 29)


`{` on new line



include/mesos/type_utils.hpp (line 146)


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)


expression continuation should be indented by 2, not 4.



src/master/master.hpp (line 893)


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
> 
>

Re: Review Request 37314: Maintenance Primitives: Populate master's maintenance-related local state upon recovery from registry.

2015-08-24 Thread Guangya Liu 

---
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.

2015-08-24 Thread Joseph Wu 

---
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