Re: Review Request 36404: Added support for peek() to process::io

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36999, 36646, 36404]

All tests passed.

- Mesos ReviewBot


On Aug. 27, 2015, 5:19 a.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36404/
> ---
> 
> (Updated Aug. 27, 2015, 5:19 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-2964
> https://issues.apache.org/jira/browse/MESOS-2964
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/io.hpp 
> 975923f40f82357f31b89428f24d01df6a8ac9fc 
>   3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 
>   3rdparty/libprocess/src/tests/io_tests.cpp 
> c642bab9e2845668767ad237985cb9ce1109 
> 
> Diff: https://reviews.apache.org/r/36404/diff/
> 
> 
> Testing
> ---
> 
> - Added a test case for process::io::peek
> - make check
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Re: Review Request 37821: Join threads in libprocess when shutting down.

[Neil Conway]

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



3rdparty/libprocess/src/process.cpp (line 2210)


Somewhat race-prone: we might see "shutting_down.load() == false", proceed 
to deliver the inbound message, and yet the shutdown code can proceed 
concurrently. After a bit of poking I couldn't find a situation in which that 
would be problematic, but maybe worth exploring if there's a known data 
race/hang...


- Neil Conway


On Aug. 27, 2015, 4:28 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> ---
> 
> (Updated Aug. 27, 2015, 4:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and 
> switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Join threads in libprocess when shutting down.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/event_loop.hpp 
> 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30 
>   3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840 
>   3rdparty/libprocess/src/libevent.cpp 
> d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5 
>   3rdparty/libprocess/src/process.cpp 
> 755187c8761137cb2bf2f7295b29a63f63c68bc6 
> 
> Diff: https://reviews.apache.org/r/37821/diff/
> 
> 
> Testing
> ---
> 
> After configuring with both "../configure" and "../configure 
> --enable-libevent --enable-ssl":
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.

[Alexander Rukletsov]

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


s/.///



src/master/maintenance.hpp (lines 26 - 27)


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)


Libprocess headers go before stout.



src/master/maintenance.cpp (line 30)


Put this right after `` please!



src/tests/maintenance.hpp (line 50)


`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 37314: Maintenance Primitives: Populate master's maintenance-related local state upon recovery from registry.

[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 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.

[Alexander Rukletsov]

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



include/mesos/maintenance/maintenance.proto (lines 31 - 37)


IIUC, here you group a bunch of machines based on time intervals. Why this 
grouping is necessary? We can simplify the design significantly and reduce the 
number of protobufs if we represent schedule by a set of maintenance events, 
each per machine, which may occasionally overlap in time.

See my comment in https://reviews.apache.org/r/37314 with a proposal for a 
joined "maintenance event" message.



include/mesos/maintenance/maintenance.proto (line 56)


I'm not sure `0` is a valid protobuf tag.



include/mesos/mesos.proto (line 123)


Minor nit: capitalize NOTE.



include/mesos/mesos.proto (line 136)


Why do we need this message? I've checked several following patches and 
haven't seen any use cases. Shall I look further up review chain?



src/Makefile.am (line 913)


Looks like we maintain alphabetical order here.



src/master/registry.proto (line 45)


Suffix `Info` is used mostly to avoid collisions with non-proto classes, 
therefore feel free to omit `_info` in the name here.


- Alexander Rukletsov


On Aug. 26, 2015, 6:35 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36571/
> ---
> 
> (Updated Aug. 26, 2015, 6:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3066
> https://issues.apache.org/jira/browse/MESOS-3066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * MachineInfo - Describes a single box that holds one or more agents.
> * MachineInfos - A list of boxes.
> * maintenance::Window - A set of machines and a planned downtime period.
> * maintenance::Schedule - A set of maintenance windows.
> * maintenance::Mode - An enum for the three states of maintenance: Normal, 
> Draining, Deactivated.
> * Registry::MaintenanceStatus - Holds the maintenance mode of a machine.
> 
> 
> Diffs
> -
> 
>   include/mesos/maintenance/maintenance.hpp PRE-CREATION 
>   include/mesos/maintenance/maintenance.proto PRE-CREATION 
>   include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 
> 
> Diff: https://reviews.apache.org/r/36571/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37655: MESOS-3299: Add protobuf to represent time with integer precision.

[Alexander Rukletsov]

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


Do you plan to migrate existing protobufs to `TimeSpec` or use it for new 
protobufs only?

- Alexander Rukletsov


On Aug. 26, 2015, 10:56 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37655/
> ---
> 
> (Updated Aug. 26, 2015, 10:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Ben Mahler, Artem 
> Harutyunyan, Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3299
> https://issues.apache.org/jira/browse/MESOS-3299
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of using doubles, seconds and nanoseconds can be represented like
> `struct timespec`, with one field for seconds and one for nanoseconds.
> 
> This will be important if frameworks need to compare times to make decisions 
> (such as for maintenance primitives).
> 
> Note about the naming:
> 
> * Time will conflict with the Time class.
> * Most denominations of time (Seconds, Minutes, Hours, etc) will conflict 
> with Duration.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
>   include/mesos/v1/mesos.proto 382b978dca769757171c5558b7f259870592c321 
> 
> Diff: https://reviews.apache.org/r/37655/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37821: Join threads in libprocess when shutting down.

[Greg Mann]

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


Sorry folks, there seems to be some kind of data race here... after running:

"make check"

if I then run:

"for n in {1..300}; do echo $n; 3rdparty/libprocess/tests; done"

it will hang eventually, always right when the tests are exiting. I missed this 
previously because I was unaware that using the "--gtest_repeat" flag doesn't 
seem to trigger libprocess's process initialization/shutdown with each 
iteration. I'll have a look and try to figure out what's causing this.

- Greg Mann


On Aug. 27, 2015, 4:28 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> ---
> 
> (Updated Aug. 27, 2015, 4:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and 
> switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Join threads in libprocess when shutting down.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/event_loop.hpp 
> 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30 
>   3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840 
>   3rdparty/libprocess/src/libevent.cpp 
> d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5 
>   3rdparty/libprocess/src/process.cpp 
> 755187c8761137cb2bf2f7295b29a63f63c68bc6 
> 
> Diff: https://reviews.apache.org/r/37821/diff/
> 
> 
> Testing
> ---
> 
> After configuring with both "../configure" and "../configure 
> --enable-libevent --enable-ssl":
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 37821: Join threads in libprocess when shutting down.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37821]

All tests passed.

- Mesos ReviewBot


On Aug. 27, 2015, 4:28 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> ---
> 
> (Updated Aug. 27, 2015, 4:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and 
> switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Join threads in libprocess when shutting down.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/event_loop.hpp 
> 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30 
>   3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840 
>   3rdparty/libprocess/src/libevent.cpp 
> d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5 
>   3rdparty/libprocess/src/process.cpp 
> 755187c8761137cb2bf2f7295b29a63f63c68bc6 
> 
> Diff: https://reviews.apache.org/r/37821/diff/
> 
> 
> Testing
> ---
> 
> After configuring with both "../configure" and "../configure 
> --enable-libevent --enable-ssl":
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 37168: MESOS-3063

[Klaus Ma]

The tests failed because script.cpp can not find test script which has been 
updated; I'm working to check it, although the root cause is un-clear now.


[ RUN  ] ExamplesTest.DynamicReservationFramework
../../src/tests/script.cpp:66: Failure
Failed
Failed to locate script: No such file or directory
[  FAILED  ] ExamplesTest.DynamicReservationFramework (0 ms)



On 2015年08月27日 12:19, Mesos ReviewBot wrote:

[ RUN  ] ExamplesTest.DynamicReservationFramework
../../src/tests/script.cpp:66: Failure
Failed
Failed to locate script: No such file or directory
[  FAILED  ] ExamplesTest.DynamicReservationFramework (0 ms)


--
Klaus Ma (马达) | PMP | kl...@cguru.net

Re: Review Request 37823: libprocess: Update vendored copy of google-glog to 0.3.4

[Neil Conway]

Happy to help out :) Although the particular issue I ran into has
already been reported:

https://reviews.apache.org/r/37370/
https://issues.apache.org/jira/browse/MESOS-3270

Neil

On Wed, Aug 26, 2015 at 8:11 PM, Artem Harutyunyan  wrote:
>
> This is an automatically generated e-mail. To reply, visit: 
> https://reviews.apache.org/r/37823/
>
> cmake is still under heavy development, so it's not supposed to work yet. We 
> could you some cycles though if there was someone interested in helping us 
> ship it :-).
>
>
> - Artem Harutyunyan
>
>
> On August 26th, 2015, 5:37 p.m. PDT, Neil Conway wrote:
>
> Review request for mesos.
> By Neil Conway.
>
> Updated Aug. 26, 2015, 5:37 p.m.
>
> Bugs: MESOS-3322
> Repository: mesos
>
> Description
>
> This means we no longer need to apply a custom patch to glog.
> Fixes MESOS-3322.
>
> Testing
>
> Rebuilt, "make check". Briefly tested building with cmake, although that 
> seems broken on my machine for reasons unrelated to glog.
>
> Diffs
>
> 3rdparty/libprocess/3rdparty/CMakeLists.txt 
> (997cc0d0e316e316136d4746e50e9e292a82b36b)
> 3rdparty/libprocess/3rdparty/Makefile.am 
> (eb34251d24b1e5d1540151b59cf1062ca85aeb03)
> 3rdparty/libprocess/3rdparty/glog-0.3.3.patch 
> (76b8c0fe3b4615371e265bab713d62c896b7c3d6)
> 3rdparty/libprocess/3rdparty/glog-0.3.3.tar.gz 
> (92fa52d296fea2884bd58d6010b0f869611999f7)
> 3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz (PRE-CREATION)
> 3rdparty/libprocess/3rdparty/versions.am 
> (f44c7153166225279b973615ef0441c6f945da5b)
> 3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> (12506a1369de005285268f895f365aba0c560f78)
>
> View Diff

Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.

[Alexander Rukletsov]

> On Aug. 26, 2015, 7:03 p.m., Joris Van Remoortere wrote:
> > src/master/maintenance.cpp, lines 55-69
> > 
> >
> > 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.

[Alexander Rukletsov]

> On Aug. 26, 2015, 7:03 p.m., Joris Van Remoortere wrote:
> > src/master/maintenance.hpp, lines 26-27
> > 
> >
> > 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 37655: MESOS-3299: Add protobuf to represent time with integer precision.

[Neil Conway]

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



include/mesos/mesos.proto (line 101)


It might be useful to specify what the epoch is (e.g., 
seconds-since-Unix-epoch).



include/mesos/mesos.proto (line 102)


BTW, have you considered using a simpler representation, such as just a 
single int64 holding # of nanoseconds (or microseconds) since the Unix epoch? 
Using nanoseconds you'd be able to express ~292 years -- maybe that's not a 
wide enough range?


- Neil Conway


On Aug. 26, 2015, 10:56 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37655/
> ---
> 
> (Updated Aug. 26, 2015, 10:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Ben Mahler, Artem 
> Harutyunyan, Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3299
> https://issues.apache.org/jira/browse/MESOS-3299
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of using doubles, seconds and nanoseconds can be represented like
> `struct timespec`, with one field for seconds and one for nanoseconds.
> 
> This will be important if frameworks need to compare times to make decisions 
> (such as for maintenance primitives).
> 
> Note about the naming:
> 
> * Time will conflict with the Time class.
> * Most denominations of time (Seconds, Minutes, Hours, etc) will conflict 
> with Duration.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
>   include/mesos/v1/mesos.proto 382b978dca769757171c5558b7f259870592c321 
> 
> Diff: https://reviews.apache.org/r/37655/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36404: Added support for peek() to process::io

[Artem Harutyunyan]

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

(Updated Aug. 26, 2015, 10:19 p.m.)


Review request for mesos, Joris Van Remoortere and Joseph Wu.


Changes
---

Addressed comments.


Bugs: MESOS-2964
https://issues.apache.org/jira/browse/MESOS-2964


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/include/process/io.hpp 
975923f40f82357f31b89428f24d01df6a8ac9fc 
  3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 
  3rdparty/libprocess/src/tests/io_tests.cpp 
c642bab9e2845668767ad237985cb9ce1109 

Diff: https://reviews.apache.org/r/36404/diff/


Testing
---

- Added a test case for process::io::peek
- make check


Thanks,

Artem Harutyunyan

Re: Review Request 37830: Added a test for converting JSON arrays to Resources.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37826, 37827, 37830]

All tests passed.

- Mesos ReviewBot


On Aug. 27, 2015, 3:38 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37830/
> ---
> 
> (Updated Aug. 27, 2015, 3:38 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
> 
> Diff: https://reviews.apache.org/r/37830/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37821: Join threads in libprocess when shutting down.

[Greg Mann]

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

(Updated Aug. 27, 2015, 4:28 a.m.)


Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched 
to 'mcypark'.


Changes
---

Addressed comments.


Bugs: MESOS-3158
https://issues.apache.org/jira/browse/MESOS-3158


Repository: mesos


Description
---

Join threads in libprocess when shutting down.


Diffs (updated)
-

  3rdparty/libprocess/src/event_loop.hpp 
36a4cd2b1ff59f6922173ad17115bf80cc3c8f30 
  3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840 
  3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5 
  3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6 

Diff: https://reviews.apache.org/r/37821/diff/


Testing
---

After configuring with both "../configure" and "../configure --enable-libevent 
--enable-ssl":

make check


Thanks,

Greg Mann

Re: Review Request 37821: Join threads in libprocess when shutting down.

[Greg Mann]

> On Aug. 27, 2015, 2:12 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2213
> > 
> >
> > This leaks "request".

Thanks Neil! Comments addressed.


- Greg


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


On Aug. 27, 2015, 4:28 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> ---
> 
> (Updated Aug. 27, 2015, 4:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and 
> switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Join threads in libprocess when shutting down.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/event_loop.hpp 
> 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30 
>   3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840 
>   3rdparty/libprocess/src/libevent.cpp 
> d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5 
>   3rdparty/libprocess/src/process.cpp 
> 755187c8761137cb2bf2f7295b29a63f63c68bc6 
> 
> Diff: https://reviews.apache.org/r/37821/diff/
> 
> 
> Testing
> ---
> 
> After configuring with both "../configure" and "../configure 
> --enable-libevent --enable-ssl":
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 37189: Added std::hash template specializations.

[Michael Park]

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


Sorry I keep finding more stuff, but the same changes will need to be made to 
`include/mesos/v1/mesos.hpp`. The changes were introduced after you started 
working on this. Thanks!


include/mesos/type_utils.hpp (line 395)


We keep 2 newlines between top-level definitions, here and below.


- Michael Park


On Aug. 26, 2015, 3:45 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37189/
> ---
> 
> (Updated Aug. 26, 2015, 3:45 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Michael Park.
> 
> 
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added std::hash template specializations.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp 92a0b4674d6058e27044f990c07dee922567fda6 
>   src/linux/cgroups.hpp 91ccfd046256d29504e93003de83c04f048c9b78 
>   src/master/metrics.hpp c51887ee1e072a063d629a735a3c6a2212e616ef 
>   src/messages/log.hpp 94e38c189703ff22c412b86429e052e942e75e75 
>   src/module/manager.hpp cab67a8b1ee7164bb3cb2a8b24e1a6e05b40fd19 
>   src/module/manager.cpp 862b71fa9d1e303e98de3767d69c9805e7b26dcc 
>   src/tests/containerizer.hpp 24b014f44d9eec56840e18cf39fbf9100f2c0711 
>   src/tests/fetcher_tests.cpp 81e70368f7a6164e9649964881aa5a688ef222e8 
>   src/tests/module.hpp 850c2002882be52e25f191f17785698deb5513c1 
> 
> Diff: https://reviews.apache.org/r/37189/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 37830: Added a test for converting JSON arrays to Resources.

[Alexander Rukletsov]

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

Review request for mesos and Michael Park.


Bugs: MESOS-3312
https://issues.apache.org/jira/browse/MESOS-3312


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 

Diff: https://reviews.apache.org/r/37830/diff/


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 37827: Added a test for converting JSON arrays to repeated protobufs.

[Alexander Rukletsov]

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

(Updated Aug. 27, 2015, 3:38 a.m.)


Review request for mesos and Michael Park.


Changes
---

Extracted Mesos test.


Summary (updated)
-

Added a test for converting JSON arrays to repeated protobufs.


Bugs: MESOS-3312
https://issues.apache.org/jira/browse/MESOS-3312


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
c56d6a3098293eb3659b3066f10e875927ec3ac3 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
cfc2803e42284f641879fb24bce1282215c8ea52 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
a1d4084661345f9367c75f9db61279f032b93e69 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
bbd36d39e9588eb8eea6d739451ad3bab029ca08 

Diff: https://reviews.apache.org/r/37827/diff/


Testing (updated)
---

make check (Mac OS 10.10.4)

**NOTE**: Filed [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) 
to clean up protobuf generation.


Thanks,

Alexander Rukletsov

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

[Alexander Rukletsov]

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

(Updated Aug. 27, 2015, 3:38 a.m.)


Review request for mesos and Michael Park.


Changes
---

Rebased.


Summary (updated)
-

Introduced conversion of JSON arrays to repeated protobufs.


Bugs: MESOS-3312
https://issues.apache.org/jira/browse/MESOS-3312


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
57d5fdf45273c620655b44b5f5572290cffa4bf6 

Diff: https://reviews.apache.org/r/37826/diff/


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 37168: MESOS-3063

[Klaus Ma]

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

(Updated 八月 27, 2015, 3:33 a.m.)


Review request for mesos.


Bugs: MESOS-3063
https://issues.apache.org/jira/browse/MESOS-3063


Repository: mesos


Description
---

MESOS-3063 (Add an example framework using dynamic reservation)


Diffs (updated)
-

  src/Makefile.am 7b620ff 
  src/examples/dynamic_reservation_executor.cpp PRE-CREATION 
  src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
  src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
  src/tests/examples_tests.cpp 3f56b30 
  src/tests/flags.hpp 3644956 
  src/tests/script.cpp bcc1fab 

Diff: https://reviews.apache.org/r/37168/diff/


Testing (updated)
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 37827: Added tests for converting JSON arrays to repeated protobufs.

[Alexander Rukletsov]

> On Aug. 27, 2015, 2:22 a.m., haosdent huang wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h, line 38
> > 
> >
> > Need adjust order here?

Not sure I follow, what exactly do you mean?


- Alexander


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


On Aug. 27, 2015, 2:10 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37827/
> ---
> 
> (Updated Aug. 27, 2015, 2:10 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> c56d6a3098293eb3659b3066f10e875927ec3ac3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
> cfc2803e42284f641879fb24bce1282215c8ea52 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
> a1d4084661345f9367c75f9db61279f032b93e69 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
> bbd36d39e9588eb8eea6d739451ad3bab029ca08 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
> 
> Diff: https://reviews.apache.org/r/37827/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37823: libprocess: Update vendored copy of google-glog to 0.3.4

[Artem Harutyunyan]

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


cmake is still under heavy development, so it's not supposed to work yet. We 
could you some cycles though if there was someone interested in helping us ship 
it :-).

- Artem Harutyunyan


On Aug. 26, 2015, 5:37 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37823/
> ---
> 
> (Updated Aug. 26, 2015, 5:37 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3322
> https://issues.apache.org/jira/browse/MESOS-3322
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This means we no longer need to apply a custom patch to glog.
> Fixes MESOS-3322.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 
> 997cc0d0e316e316136d4746e50e9e292a82b36b 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> eb34251d24b1e5d1540151b59cf1062ca85aeb03 
>   3rdparty/libprocess/3rdparty/glog-0.3.3.patch 
> 76b8c0fe3b4615371e265bab713d62c896b7c3d6 
>   3rdparty/libprocess/3rdparty/glog-0.3.3.tar.gz 
> 92fa52d296fea2884bd58d6010b0f869611999f7 
>   3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz PRE-CREATION 
>   3rdparty/libprocess/3rdparty/versions.am 
> f44c7153166225279b973615ef0441c6f945da5b 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> 12506a1369de005285268f895f365aba0c560f78 
> 
> Diff: https://reviews.apache.org/r/37823/diff/
> 
> 
> Testing
> ---
> 
> Rebuilt, "make check". Briefly tested building with cmake, although that 
> seems broken on my machine for reasons unrelated to glog.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37824: mesos: Updates for google-glog upgrade to 0.3.4

[Neil Conway]

On Wed, Aug 26, 2015 at 6:37 PM, Mesos ReviewBot
 wrote:
> Failed command: ./support/apply-review.sh -n -r 37823
>
> Error:
>  2015-08-27 01:37:49 URL:https://reviews.apache.org/r/37823/diff/raw/ 
> [9017/9017] -> "37823.patch" [1]
> error: missing binary patch data for 
> '3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz'
> error: binary patch does not apply to 
> '3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz'
> error: 3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz: patch does not apply
> Failed to apply patch

Weird; running that command locally works fine:

./support/apply-review.sh -n -r 37823
2015-08-26 19:25:05 URL:https://reviews.apache.org/r/37823/diff/raw/
[9017/9017] -> "37823.patch" [1]
Successfully applied: libprocess: Update vendored copy of google-glog to 0.3.4

This means we no longer need to apply a custom patch to glog.
Fixes MESOS-3322.


Review: https://reviews.apache.org/r/37823
No files to lint

[master 582cb8f] libprocess: Update vendored copy of google-glog to 0.3.4
 Author: Neil Conway 
 7 files changed, 4 insertions(+), 152 deletions(-)
 delete mode 100644 3rdparty/libprocess/3rdparty/glog-0.3.3.patch
 delete mode 100644 3rdparty/libprocess/3rdparty/glog-0.3.3.tar.gz
 create mode 100644 3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz

Neil

Re: Review Request 37827: Added tests for converting JSON arrays to repeated protobufs.

[haosdent huang]

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



3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h (line 38)


Need adjust order here?



src/tests/resources_tests.cpp (line 147)


This file seems need move out this review and create a new one.


- haosdent huang


On Aug. 27, 2015, 2:10 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37827/
> ---
> 
> (Updated Aug. 27, 2015, 2:10 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> c56d6a3098293eb3659b3066f10e875927ec3ac3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
> cfc2803e42284f641879fb24bce1282215c8ea52 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
> a1d4084661345f9367c75f9db61279f032b93e69 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
> bbd36d39e9588eb8eea6d739451ad3bab029ca08 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
> 
> Diff: https://reviews.apache.org/r/37827/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37784: Remove the redundant check in HierarchicalDRFAlocator.

[Yong Qiao Wang]

> On Aug. 26, 2015, 11:13 p.m., Joseph Wu wrote:
> > We probably want to keep the check.  It's good defensive coding.

Thanks Joseph Wu for your comments.

But Mesos now always hardcode the * role in roleSorter, so do we real need to 
have this obviously useless check?


- Yong Qiao


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


On Aug. 26, 2015, 2:54 a.m., Yong Qiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37784/
> ---
> 
> (Updated Aug. 26, 2015, 2:54 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3301
> https://issues.apache.org/jira/browse/MESOS-3301
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove the redundant check in HierarchicalDRFAlocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
> 
> Diff: https://reviews.apache.org/r/37784/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Re: Review Request 37827: Added tests for converting JSON arrays to repeated protobufs.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [37826, 37827]

Failed command: ./support/apply-review.sh -n -r 37827

Error:
 2015-08-27 02:17:45 URL:https://reviews.apache.org/r/37827/diff/raw/ 
[26973/26973] -> "37827.patch" [1]
Successfully applied: Added tests for converting JSON arrays to repeated 
protobufs.

See summary.


Review: https://reviews.apache.org/r/37827
Checking 2 files using filter 
--filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/operators,+whitespace/semicolon,+whitespace/tab,+whitespace/todo
Total errors found: 0
ERROR: Commit spanning multiple projects.

Please use separate commits for mesos, libprocess and stout.

Paths grouped by project:
mesos:
  src/tests/resources_tests.cpp
stout:
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto
Failed to commit patch

- Mesos ReviewBot


On Aug. 27, 2015, 2:10 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37827/
> ---
> 
> (Updated Aug. 27, 2015, 2:10 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> c56d6a3098293eb3659b3066f10e875927ec3ac3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
> cfc2803e42284f641879fb24bce1282215c8ea52 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
> a1d4084661345f9367c75f9db61279f032b93e69 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
> bbd36d39e9588eb8eea6d739451ad3bab029ca08 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
> 
> Diff: https://reviews.apache.org/r/37827/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37821: Join threads in libprocess when shutting down.

[Neil Conway]

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



3rdparty/libprocess/src/event_loop.hpp (line 45)


Might be good to document that this method is async (i.e., we don't 
guarantee that the event loop is stopped when the function returns).



3rdparty/libprocess/src/libev.cpp (line 36)


The comment above says this code block "defines the initial values for all 
of the declarations made in libev.hpp"; unfortunately this doesn't apply to 
shutdown_watcher.



3rdparty/libprocess/src/process.cpp (line 2211)


This leaks "request".


- Neil Conway


On Aug. 27, 2015, 12:14 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> ---
> 
> (Updated Aug. 27, 2015, 12:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and 
> switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Join threads in libprocess when shutting down.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/event_loop.hpp 
> 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30 
>   3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840 
>   3rdparty/libprocess/src/libevent.cpp 
> d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5 
>   3rdparty/libprocess/src/process.cpp 
> 755187c8761137cb2bf2f7295b29a63f63c68bc6 
> 
> Diff: https://reviews.apache.org/r/37821/diff/
> 
> 
> Testing
> ---
> 
> After configuring with both "../configure" and "../configure 
> --enable-libevent --enable-ssl":
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 37827: Added tests for converting JSON arrays to repeated protobufs.

[Alexander Rukletsov]

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

Review request for mesos and Michael Park.


Bugs: MESOS-3312
https://issues.apache.org/jira/browse/MESOS-3312


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
c56d6a3098293eb3659b3066f10e875927ec3ac3 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
cfc2803e42284f641879fb24bce1282215c8ea52 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
a1d4084661345f9367c75f9db61279f032b93e69 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
bbd36d39e9588eb8eea6d739451ad3bab029ca08 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 

Diff: https://reviews.apache.org/r/37827/diff/


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Review Request 37826: Introduced conversion of JSON arrays into repeated protobufs.

[Alexander Rukletsov]

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

Review request for mesos and Michael Park.


Bugs: MESOS-3312
https://issues.apache.org/jira/browse/MESOS-3312


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
57d5fdf45273c620655b44b5f5572290cffa4bf6 

Diff: https://reviews.apache.org/r/37826/diff/


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 37505: Fix broken health check in docker executor.

[haosdent huang]

> On Aug. 26, 2015, 4:46 p.m., Timothy Chen wrote:
> > src/health-check/main.cpp, line 332
> > 
> >
> > So I think that we can not need to modify the healthcheck at all if we 
> > provide the health check command a full command path to run each time, 
> > which the docker executor constructs the full path based on executable and 
> > container name.
> > 
> > Let me know if you think there is a reason we still need to.

Thank you, let me update it.


- haosdent


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


On Aug. 23, 2015, 9:44 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37505/
> ---
> 
> (Updated Aug. 23, 2015, 9:44 a.m.)
> 
> 
> Review request for mesos, Adam B and Timothy Chen.
> 
> 
> Bugs: MESOS-3136
> https://issues.apache.org/jira/browse/MESOS-3136
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix broken health check in docker executor.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
>   src/docker/docker.hpp 38e5299ad38b9e20501387f2193b0fa448e49e3e 
>   src/docker/docker.cpp 1367de8a7bbbda6348a30e4ef4c616378e450250 
>   src/docker/executor.cpp 256d53d59d5cda63bbeb8c987ce0019e24b9fb77 
>   src/health-check/main.cpp 97b25716335ec5719c1100bd73d06b7fc98036c9 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/slave/containerizer/docker.cpp a17e4f21e7f5a1dfd47699ec84c7a48fd82294ad 
>   src/tests/health_check_tests.cpp 157a56aa06677d8b7a2cef53b29ed05cb4b5d8ea 
> 
> Diff: https://reviews.apache.org/r/37505/diff/
> 
> 
> Testing
> ---
> 
> # Add two new test cases, HealthCheckTest.ROOT_DOCKER_DockerHealthyTask and 
> HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
> # Docker health check command is run through "docker exec"
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthyTask" --verbose
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange" 
> --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37824: mesos: Updates for google-glog upgrade to 0.3.4

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [37823]

Failed command: ./support/apply-review.sh -n -r 37823

Error:
 2015-08-27 01:37:49 URL:https://reviews.apache.org/r/37823/diff/raw/ 
[9017/9017] -> "37823.patch" [1]
error: missing binary patch data for 
'3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz'
error: binary patch does not apply to 
'3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz'
error: 3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Aug. 27, 2015, 12:37 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37824/
> ---
> 
> (Updated Aug. 27, 2015, 12:37 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3322
> https://issues.apache.org/jira/browse/MESOS-3322
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See MESOS-3322.
> 
> 
> Diffs
> -
> 
>   LICENSE c3aaa437af10533132698df3348114195d338965 
>   src/python/native/ext_modules.py.in 
> 4682e5eed0f7be23fb48ef628e1bebc7741431d7 
> 
> Diff: https://reviews.apache.org/r/37824/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37821: Join threads in libprocess when shutting down.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37821]

All tests passed.

- Mesos ReviewBot


On Aug. 27, 2015, 12:14 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37821/
> ---
> 
> (Updated Aug. 27, 2015, 12:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and 
> switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3158
> https://issues.apache.org/jira/browse/MESOS-3158
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Join threads in libprocess when shutting down.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/event_loop.hpp 
> 36a4cd2b1ff59f6922173ad17115bf80cc3c8f30 
>   3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840 
>   3rdparty/libprocess/src/libevent.cpp 
> d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5 
>   3rdparty/libprocess/src/process.cpp 
> 755187c8761137cb2bf2f7295b29a63f63c68bc6 
> 
> Diff: https://reviews.apache.org/r/37821/diff/
> 
> 
> Testing
> ---
> 
> After configuring with both "../configure" and "../configure 
> --enable-libevent --enable-ssl":
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 37824: mesos: Updates for google-glog upgrade to 0.3.4

[Neil Conway]

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

Review request for mesos.


Bugs: MESOS-3322
https://issues.apache.org/jira/browse/MESOS-3322


Repository: mesos


Description
---

See MESOS-3322.


Diffs
-

  LICENSE c3aaa437af10533132698df3348114195d338965 
  src/python/native/ext_modules.py.in 4682e5eed0f7be23fb48ef628e1bebc7741431d7 

Diff: https://reviews.apache.org/r/37824/diff/


Testing
---


Thanks,

Neil Conway

Review Request 37823: libprocess: Update vendored copy of google-glog to 0.3.4

[Neil Conway]

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

Review request for mesos.


Bugs: MESOS-3322
https://issues.apache.org/jira/browse/MESOS-3322


Repository: mesos


Description
---

This means we no longer need to apply a custom patch to glog.
Fixes MESOS-3322.


Diffs
-

  3rdparty/libprocess/3rdparty/CMakeLists.txt 
997cc0d0e316e316136d4746e50e9e292a82b36b 
  3rdparty/libprocess/3rdparty/Makefile.am 
eb34251d24b1e5d1540151b59cf1062ca85aeb03 
  3rdparty/libprocess/3rdparty/glog-0.3.3.patch 
76b8c0fe3b4615371e265bab713d62c896b7c3d6 
  3rdparty/libprocess/3rdparty/glog-0.3.3.tar.gz 
92fa52d296fea2884bd58d6010b0f869611999f7 
  3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz PRE-CREATION 
  3rdparty/libprocess/3rdparty/versions.am 
f44c7153166225279b973615ef0441c6f945da5b 
  3rdparty/libprocess/cmake/ProcessConfigure.cmake 
12506a1369de005285268f895f365aba0c560f78 

Diff: https://reviews.apache.org/r/37823/diff/


Testing
---

Rebuilt, "make check". Briefly tested building with cmake, although that seems 
broken on my machine for reasons unrelated to glog.


Thanks,

Neil Conway

Re: Review Request 37200: Refactored DockerImage struct to store a list of layer ids instead of linked list of DockerLayers.

[Lily Chen]

> On Aug. 26, 2015, 5:57 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 222
> > 
> >
> > Use explicit capture.

All the variables in the function need to be captured in this lambda.


- Lily


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


On Aug. 26, 2015, 5:57 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37200/
> ---
> 
> (Updated Aug. 26, 2015, 5:57 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> Timothy Chen, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored DockerImage struct to store a list of layer ids instead of linked 
> list of DockerLayers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37787: Reworked Jenkins build script. Added test images for ubuntu-14.04-clang-3.6, ubuntu-12.04-gcc-4.8, centos-6.6-gcc-4.8.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37787]

All tests passed.

- Mesos ReviewBot


On Aug. 26, 2015, 11:32 p.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37787/
> ---
> 
> (Updated Aug. 26, 2015, 11:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-3313
> https://issues.apache.org/jira/browse/MESOS-3313
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reworked Jenkins build script. Added test images for ubuntu-14.04-clang-3.6, 
> ubuntu-12.04-gcc-4.8, centos-6.6-gcc-4.8.
> 
> 
> Diffs
> -
> 
>   support/docker/centos-6.6-gcc-4.8/Dockerfile PRE-CREATION 
>   support/docker/centos-6.6-gcc-4.8/RPM-GPG-KEY-cern PRE-CREATION 
>   support/docker/centos-6.6-gcc-4.8/epel-apache-maven.repo PRE-CREATION 
>   support/docker/centos-6.6-gcc-4.8/slc6-devtoolset.repo PRE-CREATION 
>   support/docker/centos-6.6-gcc-4.8/wandisco-svn.repo PRE-CREATION 
>   support/docker/centos-7.1-gcc-4.8/Dockerfile PRE-CREATION 
>   support/docker/ubuntu-12.04-gcc-4.8/Dockerfile PRE-CREATION 
>   support/docker/ubuntu-14.04-clang-3.6/Dockerfile PRE-CREATION 
>   support/docker/ubuntu-14.04-gcc-4.8/Dockerfile PRE-CREATION 
>   support/jenkins_build_docker.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37787/diff/
> 
> 
> Testing
> ---
> 
> `jenkins-build-docker.sh` is a reworked version of the original 
> `jenkins-build.sh` that is ran by Jenkins buildbot for building and testing 
> Mesos distributions. 
> 
> Features:
>  * Runs libevent, SSL and ROOT tests.
>  * Easily add OS/compiler Docker images for testing Mesos.
>  * Exclude tests on per-image basis.
>  * Easily reproduce the test image locally.
>  * Three new test images (ubuntu-14.04-clang-3.6, ubuntu-12.04-gcc-4.8, 
> centos-6.6-gcc-4.8).
> 
> How to run
> 
> The following environment variables have to be set for the script to run:
>  * OS - OS name/version. Currently images are available for ubuntu-14.04, 
> ubuntu-12.04, centos-7.1, centos-6.6.
>  * CONFIGURATION - ./configure flags (e.g. '--enable-libevent').
>  * COMPILER - Compiler name/version. Currently available images include 
> gcc-4.8 (default value) on all platforms, clang-3.6 on ubuntu-14.04.
> 
> Examples:
> `OS=ubuntu-14.04 CONFIGURATION='--enable-ssl --enable-libevent' 
> COMPILER=clang-3.6 ./jenkins_build_docker.sh`
> `OS=centos-7.1 CONFIGURATION='--enable-ssl --enable-libevent' 
> ./jenkins_build_docker.sh`
> 
> NOTE: Mesos Python module has a known issue on centos-6.6 ( 
> https://issues.apache.org/jira/browse/MESOS-3314 ), so **for now centos-6.6 
> should not be enabled in Jenkins**.
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Review Request 37821: Join threads in libprocess when shutting down.

[Greg Mann]

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

Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and switched 
to 'mcypark'.


Bugs: MESOS-3158
https://issues.apache.org/jira/browse/MESOS-3158


Repository: mesos


Description
---

Join threads in libprocess when shutting down.


Diffs
-

  3rdparty/libprocess/src/event_loop.hpp 
36a4cd2b1ff59f6922173ad17115bf80cc3c8f30 
  3rdparty/libprocess/src/libev.cpp 97a2694f9b10bc61841443b21f4f96055493e840 
  3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5 
  3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6 

Diff: https://reviews.apache.org/r/37821/diff/


Testing
---

After configuring with both "../configure" and "../configure --enable-libevent 
--enable-ssl":

make check


Thanks,

Greg Mann

Re: Review Request 34142: AppC provisioner.

[Jiang Yan Xu]

> On July 1, 2015, 5:40 p.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/appc.cpp, line 203
> > 
> >
> > Maybe generalize the Failure message to something like "Provisioner and 
> > Image type mismatch"

Changed to `return Failure("Unsupported container image type: " + 
stringify(image.type()));`


> On July 1, 2015, 5:40 p.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 484-487
> > 
> >
> > Do we want to remove everything on a failure? This could make finding 
> > the failure point difficult.

We can probably leave the rootfs dir behind to make it easier to debug.

But another issue is that we cannot actually directly remove the rootfs here 
because it could be in a state that additionally procedures are necessary to 
reverse the effect of a partially provisioned rootfs. (e.g. If multiple mounts 
are required for some backends such as overlayfs and the failure occurred 
between them).

The important thing is that the provisioner is not supposed to know all these 
things, the backends are.

Added a TODO to sort this out.


- Jiang Yan


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


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34142/
> ---
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8c102fb7d1f79ee768cb06de3a976ea12f958712 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34142/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 37812: Used linux filesystem isolator by default if possible.

[Timothy Chen]

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



src/slave/containerizer/mesos/containerizer.cpp (line 145)


This is quite subtle, I think we should log and also put some documentation 
around slave that we only choose linux filesystem by default with root.


- Timothy Chen


On Aug. 26, 2015, 9:04 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37812/
> ---
> 
> (Updated Aug. 26, 2015, 9:04 p.m.)
> 
> 
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used linux filesystem isolator by default if possible.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 377de50da90edd64dab812fda3730fa4d7d63b13 
> 
> Diff: https://reviews.apache.org/r/37812/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 37197: Docker image store.

[Lily Chen]

> On Aug. 26, 2015, 5:50 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 126
> > 
> >
> > s/uri/uri schemes are/ ?

Most of the comments pertaining to uris is changed in review 37200 when the 
DockerImage struct is refactored.


> On Aug. 26, 2015, 5:50 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/provisioners/docker/store.cpp, line 211
> > 
> >
> > I thought we got rid of `os::basename` and suggest using 
> > `Path::basename` instead?!
> > `::basename` (which os::basename was relying on) is not thread-safe on 
> > some systems, please avoid it.

os::basename is no longer necessary when the DockerImage scheme changes in 
37200.


- Lily


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


On Aug. 26, 2015, 5:56 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 26, 2015, 5:56 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37814: Added documentation for libprocess environment variables

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37814]

All tests passed.

- Mesos ReviewBot


On Aug. 26, 2015, 10:50 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37814/
> ---
> 
> (Updated Aug. 26, 2015, 10:50 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and haosdent huang.
> 
> 
> Bugs: MESOS-2466
> https://issues.apache.org/jira/browse/MESOS-2466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for libprocess environment variables
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
> 
> Diff: https://reviews.apache.org/r/37814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 37787: Reworked Jenkins build script. Added test images for ubuntu-14.04-clang-3.6, ubuntu-12.04-gcc-4.8, centos-6.6-gcc-4.8.

[Artem Harutyunyan]

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

(Updated Aug. 26, 2015, 4:32 p.m.)


Review request for mesos, Benjamin Hindman, Timothy Chen, and Vinod Kone.


Changes
---

Following Tim's suggestion made sure we're consistent in how we add yum repos 
in centos-6.6


Bugs: MESOS-3313
https://issues.apache.org/jira/browse/MESOS-3313


Repository: mesos


Description
---

Reworked Jenkins build script. Added test images for ubuntu-14.04-clang-3.6, 
ubuntu-12.04-gcc-4.8, centos-6.6-gcc-4.8.


Diffs (updated)
-

  support/docker/centos-6.6-gcc-4.8/Dockerfile PRE-CREATION 
  support/docker/centos-6.6-gcc-4.8/RPM-GPG-KEY-cern PRE-CREATION 
  support/docker/centos-6.6-gcc-4.8/epel-apache-maven.repo PRE-CREATION 
  support/docker/centos-6.6-gcc-4.8/slc6-devtoolset.repo PRE-CREATION 
  support/docker/centos-6.6-gcc-4.8/wandisco-svn.repo PRE-CREATION 
  support/docker/centos-7.1-gcc-4.8/Dockerfile PRE-CREATION 
  support/docker/ubuntu-12.04-gcc-4.8/Dockerfile PRE-CREATION 
  support/docker/ubuntu-14.04-clang-3.6/Dockerfile PRE-CREATION 
  support/docker/ubuntu-14.04-gcc-4.8/Dockerfile PRE-CREATION 
  support/jenkins_build_docker.sh PRE-CREATION 

Diff: https://reviews.apache.org/r/37787/diff/


Testing
---

`jenkins-build-docker.sh` is a reworked version of the original 
`jenkins-build.sh` that is ran by Jenkins buildbot for building and testing 
Mesos distributions. 

Features:
 * Runs libevent, SSL and ROOT tests.
 * Easily add OS/compiler Docker images for testing Mesos.
 * Exclude tests on per-image basis.
 * Easily reproduce the test image locally.
 * Three new test images (ubuntu-14.04-clang-3.6, ubuntu-12.04-gcc-4.8, 
centos-6.6-gcc-4.8).

How to run

The following environment variables have to be set for the script to run:
 * OS - OS name/version. Currently images are available for ubuntu-14.04, 
ubuntu-12.04, centos-7.1, centos-6.6.
 * CONFIGURATION - ./configure flags (e.g. '--enable-libevent').
 * COMPILER - Compiler name/version. Currently available images include gcc-4.8 
(default value) on all platforms, clang-3.6 on ubuntu-14.04.

Examples:
`OS=ubuntu-14.04 CONFIGURATION='--enable-ssl --enable-libevent' 
COMPILER=clang-3.6 ./jenkins_build_docker.sh`
`OS=centos-7.1 CONFIGURATION='--enable-ssl --enable-libevent' 
./jenkins_build_docker.sh`

NOTE: Mesos Python module has a known issue on centos-6.6 ( 
https://issues.apache.org/jira/browse/MESOS-3314 ), so **for now centos-6.6 
should not be enabled in Jenkins**.


Thanks,

Artem Harutyunyan

Re: Review Request 37784: Remove the redundant check in HierarchicalDRFAlocator.

[Joseph Wu]

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


We probably want to keep the check.  It's good defensive coding.

- Joseph Wu


On Aug. 25, 2015, 7:54 p.m., Yong Qiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37784/
> ---
> 
> (Updated Aug. 25, 2015, 7:54 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3301
> https://issues.apache.org/jira/browse/MESOS-3301
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove the redundant check in HierarchicalDRFAlocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
> 
> Diff: https://reviews.apache.org/r/37784/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Re: Review Request 37814: Added documentation for libprocess environment variables

[Artem Harutyunyan]

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


In case you want to see how this will actually render on a website, we've put 
together a Docker image and instructions on generating website HTML 
https://github.com/mesosphere/mesos-website-container.

- Artem Harutyunyan


On Aug. 26, 2015, 3:50 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37814/
> ---
> 
> (Updated Aug. 26, 2015, 3:50 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and haosdent huang.
> 
> 
> Bugs: MESOS-2466
> https://issues.apache.org/jira/browse/MESOS-2466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for libprocess environment variables
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
> 
> Diff: https://reviews.apache.org/r/37814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 37813: Added additional diagnostic output when a fetcher cache test fails.

[Joseph Wu]

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

Ship it!


Looks good.


src/tests/fetcher_cache_tests.cpp (line 235)


Possibly want a comment explaining that this is meant for directories with 
logs.  If it's mistakenly pointed to, say, a directory with a binary, the 
output could get messy.



src/tests/fetcher_cache_tests.cpp (line 458)


Is it common knowledge that the sandbox holds the logs?  (If not, a short 
comment would suffice.)


- Joseph Wu


On Aug. 26, 2015, 2:08 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37813/
> ---
> 
> (Updated Aug. 26, 2015, 2:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3235
> https://issues.apache.org/jira/browse/MESOS-3235
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Dumps all involved task/executor sandbox contents in test tear down
> only if a failure occurred.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp b709b1eedeb880bc815e0742dc604d93828e593f 
> 
> Diff: https://reviews.apache.org/r/37813/diff/
> 
> 
> Testing
> ---
> 
> make check on OSX, where the bug showed up.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Review Request 37814: Added documentation for libprocess environment variables

[Greg Mann]

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

Review request for mesos, Alexander Rojas and haosdent huang.


Bugs: MESOS-2466
https://issues.apache.org/jira/browse/MESOS-2466


Repository: mesos


Description
---

Added documentation for libprocess environment variables


Diffs
-

  docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 

Diff: https://reviews.apache.org/r/37814/diff/


Testing
---


Thanks,

Greg Mann

Re: Review Request 37364: Maintenance Primitives: Adds an endpoint for retrieving the maintenance status for machines.

[Joseph Wu]

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

(Updated Aug. 26, 2015, 3:44 p.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
Joris Van Remoortere, and Vinod Kone.


Changes
---

Updated to match comments and changes from previous reviews.


Bugs: MESOS-2067
https://issues.apache.org/jira/browse/MESOS-2067


Repository: mesos


Description
---

Endpoint: /maintenance.status

Returns a JSON object like:
```
{
  "draining": [
{ "hostname": "foo", "ip": "0.0.0.1" },
{ "hostname": "bar", "ip": "0.0.0.2" },
  ],
  "deactivated": [
{ "hostname": "baz", "ip": "0.0.0.3" },
  ]
}
```


Diffs (updated)
-

  src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44 
  src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
  src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
  src/tests/master_maintenance_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/37364/diff/


Testing
---

`make check`

New Tests:
  MasterMaintenanceTest.FailToUnscheduleDeactivatedMachines
Extra test case for the /maintenance.schedule endpoint, which requires all 
three endpoints to work.
  MasterMaintenanceTest.MachineStatus
Schedules, starts, and stops maintenance.  Checks machine statuses after 
each step.


Thanks,

Joseph Wu

Re: Review Request 37813: Added additional diagnostic output when a fetcher cache test fails.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37813]

All tests passed.

- Mesos ReviewBot


On Aug. 26, 2015, 9:08 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37813/
> ---
> 
> (Updated Aug. 26, 2015, 9:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3235
> https://issues.apache.org/jira/browse/MESOS-3235
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Dumps all involved task/executor sandbox contents in test tear down
> only if a failure occurred.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp b709b1eedeb880bc815e0742dc604d93828e593f 
> 
> Diff: https://reviews.apache.org/r/37813/diff/
> 
> 
> Testing
> ---
> 
> make check on OSX, where the bug showed up.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 37362: Maintenance Primitives: Adds an endpoint for transitioning agents back into Normal mode.

[Joseph Wu]

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

(Updated Aug. 26, 2015, 3:30 p.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
Joris Van Remoortere, and Vinod Kone.


Changes
---

Update to match comments and changes in earlier reviews.


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

Registry operation = maintenance::StopMaintenance
  Sets the list of machines back to Normal mode.  Removes those machines from 
the maintenance schedule.


Diffs (updated)
-

  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/37362/diff/


Testing
---

`make check`

New Tests:
  RegistrarTest.StopMaintenance
Schedules some machines.  Deactivates some.  Reactivates some.
  MasterMaintenanceTest.ReactivateMachines
Tests some invalid lists.
Schedules some machines.  Deactivates some.  Tests some valid and invalid 
lists.
Checks that the schedule is modified.


Thanks,

Joseph Wu

Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for transitioning agents into the Deactivated maintenance mode.

[Joseph Wu]

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

(Updated Aug. 26, 2015, 2:59 p.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
Joris Van Remoortere, and Vinod Kone.


Changes
---

Update to match comments and changes in previous reviews.


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

Registry operation = maintenance::StartMaintenance
  Sets the list of machines as Deactivated.


Diffs (updated)
-

  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/37358/diff/


Testing
---

`make check`

New Tests:
  RegistrarTest.StartMaintenance
Schedules some machines.  Deactivates some.
  MasterMaintenanceTest.StartMaintenance
Tests some invalid lists.
Schedules some machines.  Tests some valid and invalid lists.


Thanks,

Joseph Wu

Re: Review Request 37812: Used linux filesystem isolator by default if possible.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37726, 37734, 37735, 37738, 37812]

All tests passed.

- Mesos ReviewBot


On Aug. 26, 2015, 9:04 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37812/
> ---
> 
> (Updated Aug. 26, 2015, 9:04 p.m.)
> 
> 
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used linux filesystem isolator by default if possible.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 377de50da90edd64dab812fda3730fa4d7d63b13 
> 
> Diff: https://reviews.apache.org/r/37812/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.

[Joseph Wu]

---
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 37187: Use std::unordered_{set, map} instead of boost::unordered_{set, map}.

[Joris Van Remoortere]

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


Please take a look at my comments in https://reviews.apache.org/r/37188/

Please note my comments for hashmap may also be applicable for the other 
containers modified.


3rdparty/libprocess/3rdparty/stout/include/stout/cache.hpp (line 46)


You can trim the extra whitespace `> >` to `>>` now.



3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp (line 38)


Did you want to allow the allocator to be specified? `class Allocator = 
std::allocator>`

Should we wait to add this until it is needed?



3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp (lines 50 - 59)


So now can we just use:
```
std::unordered_map(map.begin(), map.end(), 
std::ceil(map.size() / max_load_factor())) {}
```

Here and below.

Please see my note in https://reviews.apache.org/r/35874/



3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp (lines 107 - 109)


```
auto result = std::unordered_map::emplace(key, 
value);

// If the element already existed, replace it.
if (!result.second) {
  *result.first = value;
}
```



3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp (lines 116 - 119)


@Mpark Will turning this into a ternary aid RVO?


- Joris Van Remoortere


On Aug. 25, 2015, 4:02 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37187/
> ---
> 
> (Updated Aug. 25, 2015, 4:02 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Michael Park.
> 
> 
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed hashmap, hashset to use std::unordered_{set,map} instead of 
> boost::unordered_{set,map}.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/cache.hpp 
> 25684a405bfa9c4ab65641568341652a8efaf925 
>   3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp 
> ecab60a21765c58b0732de747509aa6382d31c06 
>   3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp 
> 2dd1905a4626a7d7e9f61863c8290ae1cdb9b925 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp 
> a8573ed67e20b5206afd69bab4f5dc094a7e882f 
>   3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp 
> e8ebe0b2f5e49657ee191a2535e0abdaf8e665ce 
>   3rdparty/libprocess/3rdparty/stout/tests/hashset_tests.cpp 
> 3802a29b82da57217dd75c6b1611fd21c91cfc03 
>   3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 
> b625ffaeb3672f58fbd9558a868f87404e659c53 
> 
> Diff: https://reviews.apache.org/r/37187/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.

[Joseph Wu]

> On Aug. 26, 2015, 12:03 p.m., Joris Van Remoortere wrote:
> > src/master/http.cpp, line 1448
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 37735: Refactored filesystem isolator tests to allow multiple rootfses.

[Jie Yu]

> On Aug. 26, 2015, 6:10 p.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/filesystem_isolator_tests.cpp, lines 152-158
> > 
> >
> > Rename CREATE_VOLUME to CREATE_VOLUME_FROM_HOST instead?
> > 
> > CREATE_VOLUME only has two usage instances outside this test so it 
> > should be trivial.
> > 
> > A TODO is fine.

Added a TODO,


- Jie


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


On Aug. 24, 2015, 11 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37735/
> ---
> 
> (Updated Aug. 24, 2015, 11 p.m.)
> 
> 
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored filesystem isolator tests to allow multiple rootfses.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 7003b03f1da2fee53592bc23799f59eabcd913a2 
>   src/tests/containerizer/provisioner.hpp 
> c4ba46794fe5d7875fda11155367f521c34ea339 
> 
> Diff: https://reviews.apache.org/r/37735/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.

[Joseph Wu]

---
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 33752: MESOS-2537: Fix AC_ARG_ENABLED option processing

[James Peach]

> On Aug. 26, 2015, 9:01 p.m., Timothy St. Clair wrote:
> >
> 
> Timothy St. Clair wrote:
> Have you tested all combinations here?

No, only some of the possible combinations. The addition of buld flags output 
makes is easier to tell when it it doing the right thing. This change doesn't 
really change any logic except for ```--enable-debug``` and 
```--enable-optimize```, since the absence of defaults for those is confusing.


> On Aug. 26, 2015, 9:01 p.m., Timothy St. Clair wrote:
> > configure.ac, line 145
> > 
> >
> > doesn't this change default toggle to an evaluation of the output 
> > variable?  
> > 
> > Shouldn't this just be a toggle on existance?  
> > 
> > I'm ok on uses of "$withval" but do you have an arguement for the 
> > enableval?

The ```--enable-foo``` syntax also supports ```--disable-foo```. So if the 
build option is not specified, the default should apply, otherwise the operator 
can specify the build option must be present or absent. For example, our 
internal RPM build was specifying ```--disable-perftools```, but this had the 
unintended effect of enabling perftools.


- James


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


On Aug. 26, 2015, 8:11 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33752/
> ---
> 
> (Updated Aug. 26, 2015, 8:11 p.m.)
> 
> 
> Review request for mesos, Cody Maloney and Timothy St. Clair.
> 
> 
> Bugs: MESOS-2537
> https://issues.apache.org/jira/browse/MESOS-2537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In a number of places, the Mesos configure script passes "$foo=yes"
> to the 2nd argument of AC_ARG_ENABLED. However, the 2nd argument
> is invoked when the option is provided in any form, not just when
> the --enable-foo form is used. One result of this is that
> --disable-optimize doesn't work.
> 
> The correct handling of the 2nd argument is to save the value of
> "$enableval". This change sets the value of all the enable variables
> using $enableval, and sets the default value based on the option
> name.
> 
> There are a number of enable options that were internally named
> "$with_foo" and "$without_foo". Rename these to "$enable_foo" for
> clarity and to remove the need for both a with_ and a without_
> version.
> 
> Finally, emit the compilation flags at the end of the configure
> phase so it is easier to see the results of your configuration
> options.
> 
> 
> Diffs
> -
> 
>   configure.ac 87461d73ed04c4cf176c3475ded9f98dadcda608 
> 
> Diff: https://reviews.apache.org/r/33752/diff/
> 
> 
> Testing
> ---
> 
> Configure and build on CentOS 7 and Mac OS X 10.10.3. Verify that the status 
> summary reflects the expected compiler flags. Verify that --enable-foo and 
> --disable-foo do different things.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.

[Joseph Wu]

---
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 37188: Added std::hash template specializations.

[Joris Van Remoortere]

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


@Mpark: For the standard hashers, do we want to follow this pattern?
```
template <>
  struct hash {

typedef size_t result_type;

typedef T argument_type;

result_type operator()(const argument_type &that) const {
  // body
}

  };
```

Are we replacing all uses of the hash based boost containers? If so, should we 
clean up the remaining instances of `hash_value` implementations? Grepping 
through I still see some.


3rdparty/libprocess/include/process/pid.hpp (line 153)


there is an extra space hiding here :-)



3rdparty/libprocess/src/tests/http_tests.cpp (line 806)


I think we tend to put a new line after an expression that doesn't fit on 1 
line.
Same for the one below.


- Joris Van Remoortere


On Aug. 25, 2015, 1:15 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> ---
> 
> (Updated Aug. 25, 2015, 1:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Michael Park.
> 
> 
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added std::hash template specializations.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/address.hpp 
> df78f8e525c40b87e734e16979d3315f89e12594 
>   3rdparty/libprocess/include/process/pid.hpp 
> 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0 
>   3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> c22f9d35b5dc959ce9816d3358ebff92b853bf52 
> 
> Diff: https://reviews.apache.org/r/37188/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 37726: Added support for preparing images specified in volumes.

[Jie Yu]

> On Aug. 25, 2015, 7:55 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 202
> > 
> >
> > Why not just make a copy on the stack?

Chatted offline. We cannot make a copy on the stack because otherwise, we'll 
end up updating the executorInfo on the stack, which will become invalid when 
`_prepare` returns.


- Jie


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


On Aug. 24, 2015, 7:49 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37726/
> ---
> 
> (Updated Aug. 24, 2015, 7:49 p.m.)
> 
> 
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3310
> https://issues.apache.org/jira/browse/MESOS-3310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for preparing images specified in volumes.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.hpp 
> ee5b33d14208770b474e0cb4ab4422381ceb8a3c 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> f36424e94c380870cfde49d55af397fa3dc4a612 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 7003b03f1da2fee53592bc23799f59eabcd913a2 
> 
> Diff: https://reviews.apache.org/r/37726/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 37813: Added additional diagnostic output when a fetcher cache test fails.

[Bernd Mathiske]

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

(Updated Aug. 26, 2015, 2:08 p.m.)


Review request for mesos, Benjamin Hindman, Joseph Wu, and Till Toenshoff.


Changes
---

testing description added


Bugs: MESOS-3235
https://issues.apache.org/jira/browse/MESOS-3235


Repository: mesos


Description
---

Dumps all involved task/executor sandbox contents in test tear down
only if a failure occurred.


Diffs
-

  src/tests/fetcher_cache_tests.cpp b709b1eedeb880bc815e0742dc604d93828e593f 

Diff: https://reviews.apache.org/r/37813/diff/


Testing (updated)
---

make check on OSX, where the bug showed up.


Thanks,

Bernd Mathiske

Review Request 37813: Added additional diagnostic output when a fetcher cache test fails.

[Bernd Mathiske]

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

Review request for mesos, Benjamin Hindman, Joseph Wu, and Till Toenshoff.


Bugs: MESOS-3235
https://issues.apache.org/jira/browse/MESOS-3235


Repository: mesos


Description
---

Dumps all involved task/executor sandbox contents in test tear down
only if a failure occurred.


Diffs
-

  src/tests/fetcher_cache_tests.cpp b709b1eedeb880bc815e0742dc604d93828e593f 

Diff: https://reviews.apache.org/r/37813/diff/


Testing
---


Thanks,

Bernd Mathiske

Re: Review Request 35234: libprocess: consistent handling of --enable options

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [33752, 35084, 35234]

All tests passed.

- Mesos ReviewBot


On Aug. 26, 2015, 8:09 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35234/
> ---
> 
> (Updated Aug. 26, 2015, 8:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Cody Maloney, and Timothy St. 
> Clair.
> 
> 
> Bugs: MESOS-2537
> https://issues.apache.org/jira/browse/MESOS-2537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Let both --enable-$OPTION and --disable-$OPTION work consistently.
> Add bundled package options consistent with Mesos, so that options
> passed down from Mesos work correctly.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> eb34251d24b1e5d1540151b59cf1062ca85aeb03 
>   3rdparty/libprocess/configure.ac 7c2bcffe5c7be1f7d90e6df470d20a00245bfbff 
> 
> Diff: https://reviews.apache.org/r/35234/diff/
> 
> 
> Testing
> ---
> 
> Make and make check on CentOS 7 and OS X. There's definitely combinations 
> that have not been tested!
> 
> Note that this removes some login around using gmock. AFAICT the unbundled 
> gmock doesn't work in the general case. I have a bunch of crashes where the 
> build would pick up gtest headers from the system and gmock from libprocess 
> 3rdparty. My conclusion is that the only safe path is to use the bundled 
> gmock. There's no real path through the build to use decoupled gmock and 
> gtest, it seems to be assumed that gmock will provide gtest.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 37812: Used linux filesystem isolator by default if possible.

[Jie Yu]

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

Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.


Repository: mesos


Description
---

Used linux filesystem isolator by default if possible.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
377de50da90edd64dab812fda3730fa4d7d63b13 

Diff: https://reviews.apache.org/r/37812/diff/


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 33752: MESOS-2537: Fix AC_ARG_ENABLED option processing

[Timothy St. Clair]

> On Aug. 26, 2015, 9:01 p.m., Timothy St. Clair wrote:
> >

Have you tested all combinations here?


- Timothy


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


On Aug. 26, 2015, 8:11 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33752/
> ---
> 
> (Updated Aug. 26, 2015, 8:11 p.m.)
> 
> 
> Review request for mesos, Cody Maloney and Timothy St. Clair.
> 
> 
> Bugs: MESOS-2537
> https://issues.apache.org/jira/browse/MESOS-2537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In a number of places, the Mesos configure script passes "$foo=yes"
> to the 2nd argument of AC_ARG_ENABLED. However, the 2nd argument
> is invoked when the option is provided in any form, not just when
> the --enable-foo form is used. One result of this is that
> --disable-optimize doesn't work.
> 
> The correct handling of the 2nd argument is to save the value of
> "$enableval". This change sets the value of all the enable variables
> using $enableval, and sets the default value based on the option
> name.
> 
> There are a number of enable options that were internally named
> "$with_foo" and "$without_foo". Rename these to "$enable_foo" for
> clarity and to remove the need for both a with_ and a without_
> version.
> 
> Finally, emit the compilation flags at the end of the configure
> phase so it is easier to see the results of your configuration
> options.
> 
> 
> Diffs
> -
> 
>   configure.ac 87461d73ed04c4cf176c3475ded9f98dadcda608 
> 
> Diff: https://reviews.apache.org/r/33752/diff/
> 
> 
> Testing
> ---
> 
> Configure and build on CentOS 7 and Mac OS X 10.10.3. Verify that the status 
> summary reflects the expected compiler flags. Verify that --enable-foo and 
> --disable-foo do different things.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 33752: MESOS-2537: Fix AC_ARG_ENABLED option processing

[Timothy St. Clair]

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



configure.ac (line 145)


doesn't this change default toggle to an evaluation of the output variable? 
 

Shouldn't this just be a toggle on existance?  

I'm ok on uses of "$withval" but do you have an arguement for the enableval?


- Timothy St. Clair


On Aug. 26, 2015, 8:11 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33752/
> ---
> 
> (Updated Aug. 26, 2015, 8:11 p.m.)
> 
> 
> Review request for mesos, Cody Maloney and Timothy St. Clair.
> 
> 
> Bugs: MESOS-2537
> https://issues.apache.org/jira/browse/MESOS-2537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In a number of places, the Mesos configure script passes "$foo=yes"
> to the 2nd argument of AC_ARG_ENABLED. However, the 2nd argument
> is invoked when the option is provided in any form, not just when
> the --enable-foo form is used. One result of this is that
> --disable-optimize doesn't work.
> 
> The correct handling of the 2nd argument is to save the value of
> "$enableval". This change sets the value of all the enable variables
> using $enableval, and sets the default value based on the option
> name.
> 
> There are a number of enable options that were internally named
> "$with_foo" and "$without_foo". Rename these to "$enable_foo" for
> clarity and to remove the need for both a with_ and a without_
> version.
> 
> Finally, emit the compilation flags at the end of the configure
> phase so it is easier to see the results of your configuration
> options.
> 
> 
> Diffs
> -
> 
>   configure.ac 87461d73ed04c4cf176c3475ded9f98dadcda608 
> 
> Diff: https://reviews.apache.org/r/33752/diff/
> 
> 
> Testing
> ---
> 
> Configure and build on CentOS 7 and Mac OS X 10.10.3. Verify that the status 
> summary reflects the expected compiler flags. Verify that --enable-foo and 
> --disable-foo do different things.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 35084: Provide consistent behavior for bundled packages.

[James Peach]

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

(Updated Aug. 26, 2015, 8:18 p.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, and Timothy St. Clair.


Summary (updated)
-

Provide consistent behavior for bundled packages.


Bugs: MESOS-2537
https://issues.apache.org/jira/browse/MESOS-2537


Repository: mesos


Description
---

Add the MESOS_USE_BUNDLED_PACKAGE() macro to make it easy to provide
consistent behavior for bundled packages selected by either
--enable-bundled-$PACKAGE or --with-$PACKAGE.

The default policy is set by --enable-bundled and overridden when
the user specifies an --enable-bundled-$PACKAGE or --with-$PACKAGE
option. If --with-$PACKAGE is specified as "bundled", the bundled
version is selected.


Diffs
-

  configure.ac 87461d73ed04c4cf176c3475ded9f98dadcda608 

Diff: https://reviews.apache.org/r/35084/diff/


Testing (updated)
---

Configure and build on CentOS 7 and Mac OS X 10.10.3. Verify various (not 
exhaustive!) combinations of enabling and disabling bundled packages.

For example, on CentOS, this alost works:
  $ onfigure.developer  --disable-bundled --with-zookeeper=bundled 
--with-gmock=bundled

To work completely, this change needs to be propagated to libprocess, which I 
can do once reviewers agree that it's the right behavior.


Thanks,

James Peach

Re: Review Request 33752: MESOS-2537: Fix AC_ARG_ENABLED option processing

[James Peach]

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

(Updated Aug. 26, 2015, 8:11 p.m.)


Review request for mesos, Cody Maloney and Timothy St. Clair.


Changes
---

Rebased.


Bugs: MESOS-2537
https://issues.apache.org/jira/browse/MESOS-2537


Repository: mesos


Description
---

In a number of places, the Mesos configure script passes "$foo=yes"
to the 2nd argument of AC_ARG_ENABLED. However, the 2nd argument
is invoked when the option is provided in any form, not just when
the --enable-foo form is used. One result of this is that
--disable-optimize doesn't work.

The correct handling of the 2nd argument is to save the value of
"$enableval". This change sets the value of all the enable variables
using $enableval, and sets the default value based on the option
name.

There are a number of enable options that were internally named
"$with_foo" and "$without_foo". Rename these to "$enable_foo" for
clarity and to remove the need for both a with_ and a without_
version.

Finally, emit the compilation flags at the end of the configure
phase so it is easier to see the results of your configuration
options.


Diffs (updated)
-

  configure.ac 87461d73ed04c4cf176c3475ded9f98dadcda608 

Diff: https://reviews.apache.org/r/33752/diff/


Testing
---

Configure and build on CentOS 7 and Mac OS X 10.10.3. Verify that the status 
summary reflects the expected compiler flags. Verify that --enable-foo and 
--disable-foo do different things.


Thanks,

James Peach

Re: Review Request 35084: Provided consistent behavior for bundled packages.

[James Peach]

> On June 9, 2015, 12:28 a.m., Cody Maloney wrote:
> > configure.ac, line 171
> > 
> >
> > Since these all follow about the same structure could we add an 
> > autoconf macro which takes the package name followed by the help 
> > description and  does the rest automatically?

If you send me a JIRA issue, I would prefer to do that as a separate change. 
This is already larger that I'm really comfortable with.


> On June 9, 2015, 12:28 a.m., Cody Maloney wrote:
> > configure.ac, line 58
> > 
> >
> > nit: Could you document auto here? Right now it's an implicit magic 
> > value like 'bundled' in a lot of ways.
> > 
> > I also wonder if it is possible to have this macro set the AC_ARG_WITH 
> > / AC_ARG_ENABLE, but that definitely isn't necessary / blocker here.

That's documented in rule 4 - "Otherwise use the policy set by 
--enable-bundled".


- James


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


On July 15, 2015, 4:59 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35084/
> ---
> 
> (Updated July 15, 2015, 4:59 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Cody Maloney, and Timothy St. 
> Clair.
> 
> 
> Bugs: MESOS-2537
> https://issues.apache.org/jira/browse/MESOS-2537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add the MESOS_USE_BUNDLED_PACKAGE() macro to make it easy to provide
> consistent behavior for bundled packages selected by either
> --enable-bundled-$PACKAGE or --with-$PACKAGE.
> 
> The default policy is set by --enable-bundled and overridden when
> the user specifies an --enable-bundled-$PACKAGE or --with-$PACKAGE
> option. If --with-$PACKAGE is specified as "bundled", the bundled
> version is selected.
> 
> 
> Diffs
> -
> 
>   configure.ac cad7f0e92eacc86d37b3f578382946db8b466531 
> 
> Diff: https://reviews.apache.org/r/35084/diff/
> 
> 
> Testing
> ---
> 
> Configure and build on CentOS 7 and Mac OS X 10.10.3. Verify various (not 
> exhaustive!) combinations of enabling and disableing bundled packages.
> 
> For example, on CentOS, this alost works:
>   $ onfigure.developer  --disable-bundled --with-zookeeper=bundled 
> --with-gmock=bundled
> 
> To work completely, this change needs to be propagated to libprocess, which I 
> can do once reviewers agree that it's the right behavior.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 35084: Provided consistent behavior for bundled packages.

[James Peach]

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

(Updated Aug. 26, 2015, 8:10 p.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, and Timothy St. Clair.


Changes
---

Rebased.


Bugs: MESOS-2537
https://issues.apache.org/jira/browse/MESOS-2537


Repository: mesos


Description
---

Add the MESOS_USE_BUNDLED_PACKAGE() macro to make it easy to provide
consistent behavior for bundled packages selected by either
--enable-bundled-$PACKAGE or --with-$PACKAGE.

The default policy is set by --enable-bundled and overridden when
the user specifies an --enable-bundled-$PACKAGE or --with-$PACKAGE
option. If --with-$PACKAGE is specified as "bundled", the bundled
version is selected.


Diffs (updated)
-

  configure.ac 87461d73ed04c4cf176c3475ded9f98dadcda608 

Diff: https://reviews.apache.org/r/35084/diff/


Testing
---

Configure and build on CentOS 7 and Mac OS X 10.10.3. Verify various (not 
exhaustive!) combinations of enabling and disableing bundled packages.

For example, on CentOS, this alost works:
  $ onfigure.developer  --disable-bundled --with-zookeeper=bundled 
--with-gmock=bundled

To work completely, this change needs to be propagated to libprocess, which I 
can do once reviewers agree that it's the right behavior.


Thanks,

James Peach

Re: Review Request 35234: libprocess: consistent handling of --enable options

[James Peach]

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

(Updated Aug. 26, 2015, 8:09 p.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, and Timothy St. Clair.


Changes
---

Rebased.


Bugs: MESOS-2537
https://issues.apache.org/jira/browse/MESOS-2537


Repository: mesos


Description
---

Let both --enable-$OPTION and --disable-$OPTION work consistently.
Add bundled package options consistent with Mesos, so that options
passed down from Mesos work correctly.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/Makefile.am 
eb34251d24b1e5d1540151b59cf1062ca85aeb03 
  3rdparty/libprocess/configure.ac 7c2bcffe5c7be1f7d90e6df470d20a00245bfbff 

Diff: https://reviews.apache.org/r/35234/diff/


Testing
---

Make and make check on CentOS 7 and OS X. There's definitely combinations that 
have not been tested!

Note that this removes some login around using gmock. AFAICT the unbundled 
gmock doesn't work in the general case. I have a bunch of crashes where the 
build would pick up gtest headers from the system and gmock from libprocess 
3rdparty. My conclusion is that the only safe path is to use the bundled gmock. 
There's no real path through the build to use decoupled gmock and gtest, it 
seems to be assumed that gmock will provide gtest.


Thanks,

James Peach

Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.

[Joris Van Remoortere]

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



src/tests/maintenance.hpp (line 37)


should this come before the namespace declarations?



src/tests/maintenance.hpp (lines 55 - 56)


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)


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)


`std::move(array)` ?



src/tests/maintenance.hpp (lines 83 - 84)


I think you're fixing this in an upcoming diff?



src/tests/maintenance.hpp (line 85)


how about `seconds`?
I'm guessing this function will be refactored in your next diff.



src/tests/maintenance.hpp (line 109)


`foreach` (as above)



src/tests/master_maintenance_tests.cpp (line 28)


reorder



src/tests/master_maintenance_tests.cpp (line 75)


new line.



src/tests/master_maintenance_tests.cpp (line 89)


s/masterBlob/masterSchedule_ ?



src/tests/master_maintenance_tests.cpp (line 93)


`ASSERT_SOME(masterSchedule);`



src/tests/master_maintenance_tests.cpp (line 95)


```
ASSERT_EQ(
"machine1",
masterSchedule.get().windows(0).machine_infos(0).hostname());
```



src/tests/master_maintenance_tests.cpp (lines 108 - 112)


```
schedule = createMaintenanceSchedule({
createMaintenanceWindow({machine1}),
createMaintenanceWindow({machine1})});
```



src/tests/master_maintenance_tests.cpp (line 113)


new line before this assignment.



src/tests/master_maintenance_tests.cpp (line 123)


new line



src/tests/master_maintenance_tests.cpp (line 133)


new line



src/tests/registrar_tests.cpp (line 323)


`NOTE:`



src/tests/registrar_tests.cpp (line 326)


s/into blocks/into scoped blocks ?



src/tests/registrar_tests.cpp (line 333)


machine1, etc.



src/tests/registrar_tests.cpp (lines 343 - 353)


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

Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.

[Joris Van Remoortere]

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


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)


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)


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)


new line.



src/master/http.cpp (lines 1421 - 1423)


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)


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)


s/Draining/`Draining`



src/master/http.cpp (line 1448)


I don't know if we require a new line here. I wouldn't mind one. thoughts?



src/master/maintenance.hpp (lines 26 - 27)


is this the right order?



src/master/maintenance.hpp (line 37)


Do you want to use Doxygen documentation style since this is a new File? :-D



src/master/maintenance.hpp (line 38)


backticks (`) around the modes?



src/master/maintenance.hpp (line 39)


s/non-Deactivated machines/machines that are not `Deactivated`



src/master/maintenance.hpp (line 60)


I believe we like to indent lists like this



src/master/maintenance.hpp (line 61)


Can we choose a different word than numbers? Data? TimeSpecs?



src/master/maintenance.cpp (lines 55 - 69)


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)


s/MaintenanceStatus/`MaintenanceStatus`



src/master/maintenance.cpp (line 84)


s/Draining/`Draining`



src/master/maintenance.cpp (lines 93 - 96)


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)


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)


Is it missing, or just empty?



src/master/maintenance.cpp (line 124)


Change numbers to something more meaningful?



src/master/maintenance.cpp (line 126)


indent 2 spaces for expression continuation.
new line before if statement.



src/master/maintenance.cpp (line 131)


Collect machines from the updated schedule into a set.



src/master/maintenance.cpp (line 132)


Same thing here. Can you use a `foreach` on the mutable version?



src/master/maintenance.cpp (line 136)


Is this the right place to perform this mut

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

[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 37738: Added a filesystem isolator test to test image in volume while the container root filesystem is also specified.

[Jiang Yan Xu]

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

Ship it!



src/tests/containerizer/filesystem_isolator_tests.cpp (line 575)


A compound statement to test the rootfs pivot as well?

"[ ! -d '" + os::getcwd() + "' ] && [ -d rootfs/bin ]"



src/tests/containerizer/filesystem_isolator_tests.cpp (lines 578 - 579)


Same as my comment about in the last review about  literals 
"test_image_rootfs" and "test_image_volume" needing to match the ones used to 
create the containerizer. Local variables are more explicit.


- Jiang Yan Xu


On Aug. 24, 2015, 4:22 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37738/
> ---
> 
> (Updated Aug. 24, 2015, 4:22 p.m.)
> 
> 
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a filesystem isolator test to test image in volume while the container 
> root filesystem is also specified.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 7003b03f1da2fee53592bc23799f59eabcd913a2 
> 
> Diff: https://reviews.apache.org/r/37738/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 37189: Added std::hash template specializations.

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On Aug. 26, 2015, 3:45 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37189/
> ---
> 
> (Updated Aug. 26, 2015, 3:45 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Michael Park.
> 
> 
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added std::hash template specializations.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp 92a0b4674d6058e27044f990c07dee922567fda6 
>   src/linux/cgroups.hpp 91ccfd046256d29504e93003de83c04f048c9b78 
>   src/master/metrics.hpp c51887ee1e072a063d629a735a3c6a2212e616ef 
>   src/messages/log.hpp 94e38c189703ff22c412b86429e052e942e75e75 
>   src/module/manager.hpp cab67a8b1ee7164bb3cb2a8b24e1a6e05b40fd19 
>   src/module/manager.cpp 862b71fa9d1e303e98de3767d69c9805e7b26dcc 
>   src/tests/containerizer.hpp 24b014f44d9eec56840e18cf39fbf9100f2c0711 
>   src/tests/fetcher_tests.cpp 81e70368f7a6164e9649964881aa5a688ef222e8 
>   src/tests/module.hpp 850c2002882be52e25f191f17785698deb5513c1 
> 
> Diff: https://reviews.apache.org/r/37189/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.

[Joseph Wu]

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

(Updated Aug. 26, 2015, 11:35 a.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
Joris Van Remoortere, and Vinod Kone.


Changes
---

Comment changes + a rename of a field in the Registry::MaintenanceStatus object.


Bugs: MESOS-3066
https://issues.apache.org/jira/browse/MESOS-3066


Repository: mesos


Description
---

* MachineInfo - Describes a single box that holds one or more agents.
* MachineInfos - A list of boxes.
* maintenance::Window - A set of machines and a planned downtime period.
* maintenance::Schedule - A set of maintenance windows.
* maintenance::Mode - An enum for the three states of maintenance: Normal, 
Draining, Deactivated.
* Registry::MaintenanceStatus - Holds the maintenance mode of a machine.


Diffs (updated)
-

  include/mesos/maintenance/maintenance.hpp PRE-CREATION 
  include/mesos/maintenance/maintenance.proto PRE-CREATION 
  include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
  src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
  src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 

Diff: https://reviews.apache.org/r/36571/diff/


Testing
---

`make check`


Thanks,

Joseph Wu

Re: Review Request 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.

[Joseph Wu]

> On Aug. 26, 2015, 9:57 a.m., Joris Van Remoortere wrote:
> > A higher level wording question: Do we want to be consistent with `Mode` vs 
> > `Status`, or did you explicitly use different words? For example, we could 
> > change `MaintenanceStatus` to `MaintenanceModes`. I'm not necessarily 
> > saying we should, just asking if this was done for a specific reason.

I named it `status` because we might be storing more than just the mode in the 
future.  (Not for the MVP, but there were considerations of including data like 
the accept/decline reason from frameworks and such.)


- Joseph


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


On Aug. 24, 2015, 11:33 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36571/
> ---
> 
> (Updated Aug. 24, 2015, 11:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3066
> https://issues.apache.org/jira/browse/MESOS-3066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * MachineInfo - Describes a single box that holds one or more agents.
> * MachineInfos - A list of boxes.
> * maintenance::Window - A set of machines and a planned downtime period.
> * maintenance::Schedule - A set of maintenance windows.
> * maintenance::Mode - An enum for the three states of maintenance: Normal, 
> Draining, Deactivated.
> * Registry::MaintenanceStatus - Holds the maintenance mode of a machine.
> 
> 
> Diffs
> -
> 
>   include/mesos/maintenance/maintenance.hpp PRE-CREATION 
>   include/mesos/maintenance/maintenance.proto PRE-CREATION 
>   include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 
> 
> Diff: https://reviews.apache.org/r/36571/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37197: Docker image store.

[Timothy Chen]

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



src/slave/containerizer/provisioners/docker/store.cpp (lines 273 - 274)


Should we put this in the shared utils as well?
Seems like a very common pattern


- Timothy Chen


On Aug. 26, 2015, 5:56 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 26, 2015, 5:56 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.

[Joseph Wu]

> On Aug. 26, 2015, 10:44 a.m., Joris Van Remoortere wrote:
> > src/master/registry.proto, line 45
> > 
> >
> > Should we call this `info` or rename the type to `MachineId`?

I think I can rename this to `machine_info` (to match how we name it elsewhere) 
with minimal impact on your reviews.


- Joseph


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


On Aug. 24, 2015, 11:33 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36571/
> ---
> 
> (Updated Aug. 24, 2015, 11:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3066
> https://issues.apache.org/jira/browse/MESOS-3066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * MachineInfo - Describes a single box that holds one or more agents.
> * MachineInfos - A list of boxes.
> * maintenance::Window - A set of machines and a planned downtime period.
> * maintenance::Schedule - A set of maintenance windows.
> * maintenance::Mode - An enum for the three states of maintenance: Normal, 
> Draining, Deactivated.
> * Registry::MaintenanceStatus - Holds the maintenance mode of a machine.
> 
> 
> Diffs
> -
> 
>   include/mesos/maintenance/maintenance.hpp PRE-CREATION 
>   include/mesos/maintenance/maintenance.proto PRE-CREATION 
>   include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 
> 
> Diff: https://reviews.apache.org/r/36571/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.

[Joseph Wu]

> On Aug. 26, 2015, 9:57 a.m., Joris Van Remoortere wrote:
> > include/mesos/maintenance/maintenance.proto, line 33
> > 
> >
> > Why the choice of repeated `MachineInfo` over `MachineInfos`?

Mostly because of how this is created/accessed.  If I used `MachineInfos`, we'd 
end up with `.machine_infos().machine_infos()`, instead of just 
`.machine_infos()`.  And I don't think there will be any extension of 
MachineInfos with extra fields.


> On Aug. 26, 2015, 9:57 a.m., Joris Van Remoortere wrote:
> > include/mesos/maintenance/maintenance.proto, line 29
> > 
> >
> > is `interval` referring to the old proto here?
> > Should we say `A set of machines scheduled to go into maintenance 
> > during the same unavailability.`

Yes and no.  In my head, unavailability, interval, period, range of time, etc 
are all sort of synonyms.

I'll reword anyway to be more clear.


> On Aug. 26, 2015, 9:57 a.m., Joris Van Remoortere wrote:
> > src/Makefile.am, line 428
> > 
> >
> > Is this just a whitespace change?
> > Want to make sure I'm not missing anything here.

Yes.  Apparently, that line used spaces instead of tabs, contrary to everything 
else in the file.


> On Aug. 26, 2015, 9:57 a.m., Joris Van Remoortere wrote:
> > src/master/registry.proto, line 25
> > 
> >
> > s/master/top level/ to avoid any confusion? Or did you really mean 
> > `master` as in the entity?

Top level sounds better.  I think `master` was used initially because the 
registry persists on nodes related to the master...


- Joseph


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


On Aug. 24, 2015, 11:33 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36571/
> ---
> 
> (Updated Aug. 24, 2015, 11:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3066
> https://issues.apache.org/jira/browse/MESOS-3066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * MachineInfo - Describes a single box that holds one or more agents.
> * MachineInfos - A list of boxes.
> * maintenance::Window - A set of machines and a planned downtime period.
> * maintenance::Schedule - A set of maintenance windows.
> * maintenance::Mode - An enum for the three states of maintenance: Normal, 
> Draining, Deactivated.
> * Registry::MaintenanceStatus - Holds the maintenance mode of a machine.
> 
> 
> Diffs
> -
> 
>   include/mesos/maintenance/maintenance.hpp PRE-CREATION 
>   include/mesos/maintenance/maintenance.proto PRE-CREATION 
>   include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 
> 
> Diff: https://reviews.apache.org/r/36571/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37735: Refactored filesystem isolator tests to allow multiple rootfses.

[Jiang Yan Xu]

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

Ship it!



src/tests/containerizer/filesystem_isolator_tests.cpp (line 76)


Unfortunately I realized this after 
[this](https://github.com/apache/mesos/blob/master/include/mesos/mesos.proto#L1138)
 is checked in, but appc is not (or, no longer is, I don't know) stylized as 
"AppC" so here we can just use `TestAppcProvisioner` to avoid the somewhat 
weird looking CamelCasing.

Here and elsewhere.

Also s/provision/provisions/



src/tests/containerizer/filesystem_isolator_tests.cpp (line 86)


s/"rootfs"/"rootfses"/ because this is the root dir for multiple rootfses.



src/tests/containerizer/filesystem_isolator_tests.cpp (lines 152 - 158)


Rename CREATE_VOLUME to CREATE_VOLUME_FROM_HOST instead?

CREATE_VOLUME only has two usage instances outside this test so it should 
be trivial.

A TODO is fine.



src/tests/containerizer/filesystem_isolator_tests.cpp (line 528)


"test_image" is used twice in this test without a common variable. It's not 
immediately clear that the two instances should match.

Of course the reader is expected to know this, but the more explicit the 
better, right?


- Jiang Yan Xu


On Aug. 24, 2015, 4 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37735/
> ---
> 
> (Updated Aug. 24, 2015, 4 p.m.)
> 
> 
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored filesystem isolator tests to allow multiple rootfses.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 7003b03f1da2fee53592bc23799f59eabcd913a2 
>   src/tests/containerizer/provisioner.hpp 
> c4ba46794fe5d7875fda11155367f521c34ea339 
> 
> Diff: https://reviews.apache.org/r/37735/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 37200: Refactored DockerImage struct to store a list of layer ids instead of linked list of DockerLayers.

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker.hpp (line 61)


Would a factory method be more apporoprate here(create)? What if the string 
passed in is arbitary?



src/slave/containerizer/provisioners/docker.hpp 


Needs "explicit" on the constructor?



src/slave/containerizer/provisioners/docker.hpp (line 62)


Ctor too long to be eligible for inlined in header file.



src/slave/containerizer/provisioners/docker.hpp (lines 66 - 68)


Can avoid else block as Option state is default constructed to None.



src/slave/containerizer/provisioners/docker.hpp (line 69)


No need for this variable. Could be folded into  the "if" condition.



src/slave/containerizer/provisioners/docker.cpp (line 189)


Are we always over writing existing provisioned containers?



src/slave/containerizer/provisioners/docker.cpp (line 199)


Use explicit capture.



src/slave/containerizer/provisioners/docker.cpp (line 221)


Use explicit capture.



src/slave/containerizer/provisioners/docker.cpp (line 251)


What happens on failure?


- Jojy Varghese


On Aug. 25, 2015, 8:59 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37200/
> ---
> 
> (Updated Aug. 25, 2015, 8:59 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored DockerImage struct to store a list of layer ids instead of linked 
> list of DockerLayers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37200: Refactored DockerImage struct to store a list of layer ids instead of linked list of DockerLayers.

[Till Toenshoff]

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



src/slave/containerizer/provisioners/docker/backend.cpp (line 96)


Insert blank line here please.



src/slave/containerizer/provisioners/docker/store.hpp (line 26)


Missing
```
#include 
```



src/slave/containerizer/provisioners/docker/store.hpp (line 43)


Please add a short abstract comment describing the purpose of this 
interface.



src/slave/containerizer/provisioners/docker/store.hpp (line 53)


s/ to//



src/slave/containerizer/provisioners/docker/store.hpp (line 76)


Consider adding a protected constructor to underline the fact that this is 
a pure virtual interface.



src/slave/containerizer/provisioners/docker/store.hpp (line 78)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/store.hpp (lines 80 - 82)


See below on doxygen formatting for such attributing comments.



src/slave/containerizer/provisioners/docker/store.hpp (line 85)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/store.hpp (lines 108 - 110)


I am not sure that using doxygen comments for these attributes is the right 
way to go. I personally find it wasteful and not helping in readability - but 
will be easy to get convinced otherwise :) - also I did not check the resulting 
doxygen HTML on the differences.



src/slave/containerizer/provisioners/docker/store.hpp (line 171)


s/hahsmap/hashmap/



src/slave/containerizer/provisioners/docker/store.cpp (line 100)


Insert blank line here please.



src/slave/containerizer/provisioners/docker/store.cpp (line 115)


Insert blank line here please.


- Till Toenshoff


On Aug. 25, 2015, 8:59 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37200/
> ---
> 
> (Updated Aug. 25, 2015, 8:59 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored DockerImage struct to store a list of layer ids instead of linked 
> list of DockerLayers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37497: Added Docker provisioner paths which handles path manipulation.

[Till Toenshoff]

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



src/slave/containerizer/provisioners/docker/paths.hpp (line 48)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/paths.hpp (line 51)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/paths.hpp (line 56)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/paths.hpp (line 58)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/paths.hpp (line 63)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/paths.hpp (line 69)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/paths.hpp (line 73)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/paths.hpp (line 79)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/paths.hpp (line 84)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/paths.hpp (line 88)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/paths.cpp (line 21)


This should be the first include.



src/slave/containerizer/provisioners/docker/paths.cpp (line 35)


Just one blank line between these and the others as we stay entirely within 
the same scope (global) here.



src/slave/containerizer/provisioners/docker/paths.cpp (line 48)


See above.



src/slave/containerizer/provisioners/docker/paths.cpp (line 61)


See above.



src/slave/containerizer/provisioners/docker/paths.cpp (line 70)


See above.



src/slave/containerizer/provisioners/docker/paths.cpp (line 85)


See above.



src/slave/containerizer/provisioners/docker/paths.cpp (line 93)


See above.



src/slave/containerizer/provisioners/docker/paths.cpp (line 100)


See above.



src/slave/containerizer/provisioners/docker/reference_store.cpp (line 161)


Does this break a vital function of Mesos?
--> If so, make it a LOG(ERROR)

Does this still allow normal function but will disable certain optimizing 
features?
--> If so, make it a LOG(WARNING)

Is this valuable information for a user to understand the status of the 
system?
--> If so, make it a LOG(INFO)

Anything else is usually a VLOG(1) as it is understandable and useful 
mostly for developers.


- Till Toenshoff


On Aug. 25, 2015, 9:03 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37497/
> ---
> 
> (Updated Aug. 25, 2015, 9:03 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Docker provisioner paths which handles path manipulation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37497/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> ./bin/mesos-tests.sh --gtest_filter="*DockerProvisioner*" --gtest_repeat=20 
> --gtest_shuffle=1
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37496: Move docker provisioner local store into dedicated folders.

[Till Toenshoff]

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


At this point I partially stopped re-raising previously raised issues -- please 
make sure you are globally fixing things I mentioned in earlier reviews within 
this chain.


src/Makefile.am (lines 755 - 759)


Spaces vs. hard-tabs!



src/slave/containerizer/provisioners/docker/local_store.hpp (line 34)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/local_store.hpp (lines 57 - 59)


As mentioned before, not sure we should use doxygen on these attribute 
comments.


- Till Toenshoff


On Aug. 25, 2015, 9:02 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37496/
> ---
> 
> (Updated Aug. 25, 2015, 9:02 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Move docker provisioner local store into dedicated folders.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37496/diff/
> 
> 
> Testing
> ---
> 
> sudo make check 
> ./bin/mesos-tests.sh --gtest_filter="*DockerProvisioner*" --gtest_repeat=20 
> --gtest_shuffle=1
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37247: Added Docker image reference store.

[Till Toenshoff]

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



src/Makefile.am (line 402)


Tab spacing / hard tabs.



src/Makefile.am (line 446)


Tab spacing / hard-tabs!



src/Makefile.am 


Why did you kill this line?



src/Makefile.am (line 729)


Spacing / hard-tabs!



src/Makefile.am (lines 755 - 758)


Spacing / hard-tabs!



src/messages/docker_provisioner.proto (lines 25 - 26)


s/layerId/layer id/



src/slave/containerizer/provisioners/docker/reference_store.hpp (line 26)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/reference_store.hpp (line 28)


Missing 
```
#include 
```
(for ProcessBase)



src/slave/containerizer/provisioners/docker/reference_store.hpp (line 30)


Missing
```
#include 
```



src/slave/containerizer/provisioners/docker/reference_store.hpp (line 32)


Missing
```
#include 
```



src/slave/containerizer/provisioners/docker/reference_store.cpp (line 42)


This appears a bit overblown for the amount of symbols you are actually 
using from that namespace.



src/slave/containerizer/provisioners/docker/reference_store.cpp (line 59)


Blank line here please.



src/slave/containerizer/provisioners/docker/reference_store.cpp (line 65)


Blank line here please.



src/slave/containerizer/provisioners/docker/reference_store.cpp (line 71)


Blank line here please.



src/slave/containerizer/provisioners/docker/reference_store.cpp (line 76)


Blank line here please.



src/slave/containerizer/provisioners/docker/reference_store.cpp (line 84)


Blank line here please.



src/slave/containerizer/provisioners/docker/reference_store.cpp (line 93)


Blank line here please.



src/slave/containerizer/provisioners/docker/store.cpp (line 100)


Blank line here please.



src/slave/containerizer/provisioners/docker/store.cpp (line 107)


Blank line here please.


- Till Toenshoff


On Aug. 25, 2015, 9 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37247/
> ---
> 
> (Updated Aug. 25, 2015, 9 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-3021
> https://issues.apache.org/jira/browse/MESOS-3021
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Docker image reference store.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/messages/docker_provisioner.hpp PRE-CREATION 
>   src/messages/docker_provisioner.proto PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37247/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tests will be added in a later review.
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Till Toenshoff]

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



src/Makefile.am (lines 747 - 748)


Formatting off, use hard-tabs for these files please.



src/slave/containerizer/provisioner.hpp (line 23)


Missing
```
#include 
```



src/slave/containerizer/provisioner.hpp (lines 60 - 61)


Should we adapt the comment to mention option to specify the sandbox 
location?



src/slave/containerizer/provisioner.cpp (line 23)


See my previous comment on "using namespace ...".



src/slave/containerizer/provisioner.cpp (line 46)


One space less for the indentation.



src/slave/containerizer/provisioners/docker.hpp (line 29)


Missing:
```
#include 
```



src/slave/containerizer/provisioners/docker.hpp (line 31)


Missing:
```
#include 
```



src/slave/containerizer/provisioners/docker.hpp (line 107)


You are declaring virtual functions but your destructor is not virtual => 
aren't we reaching UB-land once this class gets dynamically created via its 
factory and later on destroyed via the smartpointer?



src/slave/containerizer/provisioners/docker.hpp (line 121)


explicit?



src/slave/containerizer/provisioners/docker.cpp (line 18)


Missing
```
#include 
```



src/slave/containerizer/provisioners/docker.cpp (line 31)


This should be the top include.



src/slave/containerizer/provisioners/docker.cpp (line 49)


Remove this line please.



src/slave/containerizer/provisioners/docker.cpp (line 206)


You generally seem to prefer "initializing tightly followed by validation" 
but here you insert a blank line -- let's make this consistent.



src/slave/containerizer/provisioners/docker.cpp (lines 242 - 243)


Seems to be well below our 80 chars limit - could do without the wrapping.



src/slave/containerizer/provisioners/docker/backend.hpp (line 29)


Missing 
```
#include 
```



src/slave/containerizer/provisioners/docker/backend.hpp (line 39)


You should unify the "Docker" case -- make sure you always call it "Docker" 
or "docker" in your comments.



src/slave/containerizer/provisioners/docker/backend.hpp (line 58)


Kill this blank line please. We generally add a blank line after "closing a 
scope" and one before "openening a scope" -- it may be a bit vague as I call it 
out here, but that is how I remind myself :).



src/slave/containerizer/provisioners/docker/backend.cpp (line 21)


Missing
```
#include 
```



src/slave/containerizer/provisioners/docker/backend.cpp (line 27)


Top include!



src/slave/containerizer/provisioners/docker/backend.cpp (line 155)


See my earlier comments on the status-code/exit-code mixup.



src/slave/containerizer/provisioners/docker/backend.cpp (lines 166 - 174)


Why using rm via subprocess instead of os::rmdir?


- Till Toenshoff


On Aug. 25, 2015, 8:58 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 25, 2015, 8:58 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner and copy backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> f36424e94c380870cfde49d55af397fa3dc4a612 
>   src/slave/containerizer/provisioner.hpp 
> 541dd4e0b2f0c92a45c00cab6132a2

Re: Review Request 37495: Docker provisioner local store unit tests.

[Till Toenshoff]

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


This particular RR review is very much on style only -- would like to invest a 
bit more time in the second round for fully grasping the flow of your tests.


src/Makefile.am (line 1698)


Alphabetize please.



src/tests/containerizer/docker_provisioner_tests.cpp (line 135)


Could you please add a one/two-liner above each test that describes its 
purpose?

Imagine an advanced user or developer that tries to run these tests which 
for some reason fail for him - wouldnt it be great if she/he/it immediately 
understood what was being tested without reversing the code?


- Till Toenshoff


On Aug. 25, 2015, 9:01 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37495/
> ---
> 
> (Updated Aug. 25, 2015, 9:01 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker provisioner local store unit tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37495/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> ./bin/mesos-tests.sh --gtest_filter="*DockerProvisioner*" --gtest_repeat=20 
> --gtest_shuffle=1
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

[Till Toenshoff]

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


Great work Lily, very clean considering that this is your first bigger 
contribution so far (hope I did not miss something here :) ). I really like the 
way things are structured, elegantly scaling.

I am puzzled on how you were able to get this compiled as you are using 
"os::basename" which got removed from stout a while ago and I am assuming that 
you dont bring it back later in this chain. I may have omitted vital 
explanations in some of the issues I raised - please feel free to ask for 
clarification / reasoning where needed.

Once you addressed these issues, I would like to give the chain another pass, 
hence I will add myself as a reviewer, hoping you are OK with that.


src/Makefile.am (line 745)


Formatting looks off in reviewboard -- please check if you are using 
hard-tabs (displayed as 8 chars).



src/slave/containerizer/provisioners/docker/store.hpp (line 29)


Missing
```
#include 
```



src/slave/containerizer/provisioners/docker/store.hpp (line 33)


Missing 
```
#include 
```



src/slave/containerizer/provisioners/docker/store.hpp (line 36)


Missing
```
#include "slave/containerizer/fetcher.hpp"
```



src/slave/containerizer/provisioners/docker/store.hpp (line 42)


How about a short descriptive comment on the purpose of this interface?



src/slave/containerizer/provisioners/docker/store.hpp (line 54)


`DockerImage` is not defined by this RR but by 37198. We usually go with 
one of those two options here;
A. mark the entire chain as "must get committed entirely"
B. make sure each RR is committable without breaking build (or function)



src/slave/containerizer/provisioners/docker/store.hpp (line 61)


Consider adding a protected default constructor to underline the fact that 
this is a pure virtual interface.



src/slave/containerizer/provisioners/docker/store.hpp (line 91)


Insert blank line here please.



src/slave/containerizer/provisioners/docker/store.cpp (line 28)


This should go above the stout headers.



src/slave/containerizer/provisioners/docker/store.cpp (line 32)


This should be the first include.



src/slave/containerizer/provisioners/docker/store.cpp (line 34)


We should avoid such general "using namespace xxx" as it pollutes our local 
namespace. Prefer explicit references as in "using process::Process" as also 
described in the google styleguide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces

However, I did not raise an issue here because our codebase shows many uses 
of "using namespace process;", so maybe we actually want to trump the google 
styleguide in specific cases? I vaguely remember we had a discussion around 
this a while back but can not find the results right now -- maybe others can 
chime in here?



src/slave/containerizer/provisioners/docker/store.cpp (line 46)


Kill this blank line please.



src/slave/containerizer/provisioners/docker/store.cpp (line 61)


Insert blank line please.



src/slave/containerizer/provisioners/docker/store.cpp (line 103)


Insert blank line please.



src/slave/containerizer/provisioners/docker/store.cpp (line 126)


s/uri/uri schemes are/ ?



src/slave/containerizer/provisioners/docker/store.cpp (line 133)


How about `imagePath` or even `path`? instead of `imageUri` as you are 
effectively trimming it from a URI to a local path?



src/slave/containerizer/provisioners/docker/store.cpp (line 135)


I am not a big fan of such magic numbers (referring to that '7'). How about 
following the pattern the fetcher is using here 
(src/slave/containerizer/fetcher.cpp)?

```
[...]
namespace slave {

static const string FILE_URI_PREFIX = "file://";
[...]

if (strings::startsWith(imageUri, FILE_URI_PREFIX)) {
imageUri = imageUri.substr(FILE_URI_PREFIX

Re: Review Request 36321: Maintenance primitives: Add Unavailability and InverseOffer protobufs.

[Joseph Wu]

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

(Updated Aug. 26, 2015, 10:46 a.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
Joris Van Remoortere, and Vinod Kone.


Changes
---

More commenting revisions.


Bugs: MESOS-2061 and MESOS-2066
https://issues.apache.org/jira/browse/MESOS-2061
https://issues.apache.org/jira/browse/MESOS-2066


Repository: mesos


Description
---

MESOS-2061: Add Unavailability and InverseOffer protobufs declarations.
MESOS-2066: Add the Unavailability field to Offers.

Also copied to v1 API.

No integration with other components (that part is tracked in separate JIRAs, 
see MESOS-1474).


Diffs (updated)
-

  include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
  include/mesos/v1/mesos.proto 382b978dca769757171c5558b7f259870592c321 

Diff: https://reviews.apache.org/r/36321/diff/


Testing
---

`make check`


Thanks,

Joseph Wu

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

[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 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.

[Joris Van Remoortere]

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



src/master/registry.proto (line 45)


Should we call this `info` or rename the type to `MachineId`?


- Joris Van Remoortere


On Aug. 24, 2015, 6:33 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36571/
> ---
> 
> (Updated Aug. 24, 2015, 6:33 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3066
> https://issues.apache.org/jira/browse/MESOS-3066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * MachineInfo - Describes a single box that holds one or more agents.
> * MachineInfos - A list of boxes.
> * maintenance::Window - A set of machines and a planned downtime period.
> * maintenance::Schedule - A set of maintenance windows.
> * maintenance::Mode - An enum for the three states of maintenance: Normal, 
> Draining, Deactivated.
> * Registry::MaintenanceStatus - Holds the maintenance mode of a machine.
> 
> 
> Diffs
> -
> 
>   include/mesos/maintenance/maintenance.hpp PRE-CREATION 
>   include/mesos/maintenance/maintenance.proto PRE-CREATION 
>   include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 
> 
> Diff: https://reviews.apache.org/r/36571/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37505: Fix broken health check in docker executor.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37703, 37505]

All tests passed.

- Mesos ReviewBot


On Aug. 23, 2015, 9:44 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37505/
> ---
> 
> (Updated Aug. 23, 2015, 9:44 a.m.)
> 
> 
> Review request for mesos, Adam B and Timothy Chen.
> 
> 
> Bugs: MESOS-3136
> https://issues.apache.org/jira/browse/MESOS-3136
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix broken health check in docker executor.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
>   src/docker/docker.hpp 38e5299ad38b9e20501387f2193b0fa448e49e3e 
>   src/docker/docker.cpp 1367de8a7bbbda6348a30e4ef4c616378e450250 
>   src/docker/executor.cpp 256d53d59d5cda63bbeb8c987ce0019e24b9fb77 
>   src/health-check/main.cpp 97b25716335ec5719c1100bd73d06b7fc98036c9 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/slave/containerizer/docker.cpp a17e4f21e7f5a1dfd47699ec84c7a48fd82294ad 
>   src/tests/health_check_tests.cpp 157a56aa06677d8b7a2cef53b29ed05cb4b5d8ea 
> 
> Diff: https://reviews.apache.org/r/37505/diff/
> 
> 
> Testing
> ---
> 
> # Add two new test cases, HealthCheckTest.ROOT_DOCKER_DockerHealthyTask and 
> HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
> # Docker health check command is run through "docker exec"
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthyTask" --verbose
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange" 
> --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.

[Joris Van Remoortere]

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


A higher level wording question: Do we want to be consistent with `Mode` vs 
`Status`, or did you explicitly use different words? For example, we could 
change `MaintenanceStatus` to `MaintenanceModes`. I'm not necessarily saying we 
should, just asking if this was done for a specific reason.


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


is `interval` referring to the old proto here?
Should we say `A set of machines scheduled to go into maintenance during 
the same unavailability.`



include/mesos/maintenance/maintenance.proto (line 33)


Why the choice of repeated `MachineInfo` over `MachineInfos`?



include/mesos/maintenance/maintenance.proto (line 42)


Looks like you're trying to stick to `agent` in this file. s/slaves/agents
Can we find a better word for `blackout`? Maybe something like `upgrade`?



include/mesos/maintenance/maintenance.proto (line 58)


Can we reword this a little? I understand what you are trying to say, but 
it might not be clear to a newcomer.

The `agent` is not releasing the resources, rather the agent is in 
co-operation with the frameworks to try and drain / evacuate / release 
resources in such a way that we still maximize utilization but without 
knowingly violating SLAs.

The ideal scenario would be that the agent is fully utilized right up until 
the maintenance window, but no critical tasks end up getting killed, right?



include/mesos/mesos.proto (line 111)


Can you expand on this comment and explain how the IP vs. hostname matching 
will work? I know you're explaining this elsewhere, but here in the proto is a 
common place for users to come look to understand how this message works, and 
what the expectations are.



src/Makefile.am (line 428)


Is this just a whitespace change?
Want to make sure I'm not missing anything here.



src/master/registry.proto (line 25)


s/master/top level/ to avoid any confusion? Or did you really mean `master` 
as in the entity?



src/master/registry.proto (line 42)


Can we use backticks (`)
s/exists/exist



src/master/registry.proto (line 58)


backticks (`)?


- Joris Van Remoortere


On Aug. 24, 2015, 6:33 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36571/
> ---
> 
> (Updated Aug. 24, 2015, 6:33 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3066
> https://issues.apache.org/jira/browse/MESOS-3066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * MachineInfo - Describes a single box that holds one or more agents.
> * MachineInfos - A list of boxes.
> * maintenance::Window - A set of machines and a planned downtime period.
> * maintenance::Schedule - A set of maintenance windows.
> * maintenance::Mode - An enum for the three states of maintenance: Normal, 
> Draining, Deactivated.
> * Registry::MaintenanceStatus - Holds the maintenance mode of a machine.
> 
> 
> Diffs
> -
> 
>   include/mesos/maintenance/maintenance.hpp PRE-CREATION 
>   include/mesos/maintenance/maintenance.proto PRE-CREATION 
>   include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 
> 
> Diff: https://reviews.apache.org/r/36571/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.

[Joseph Wu]

> On Aug. 25, 2015, 6:46 p.m., Joris Van Remoortere wrote:
> > src/tests/maintenance.hpp, lines 78-79
> > 
> >
> > Why do we have to fall down to second percision here?
> > Can we take an Option 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 37505: Fix broken health check in docker executor.

[Timothy Chen]

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



src/docker/docker.hpp (line 48)


Why we need a default version?



src/health-check/main.cpp (line 322)


So I think that we can not need to modify the healthcheck at all if we 
provide the health check command a full command path to run each time, which 
the docker executor constructs the full path based on executable and container 
name.

Let me know if you think there is a reason we still need to.


- Timothy Chen


On Aug. 23, 2015, 9:44 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37505/
> ---
> 
> (Updated Aug. 23, 2015, 9:44 a.m.)
> 
> 
> Review request for mesos, Adam B and Timothy Chen.
> 
> 
> Bugs: MESOS-3136
> https://issues.apache.org/jira/browse/MESOS-3136
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix broken health check in docker executor.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 33e1b28f1ccbe227657a14395f81df20e0a9e193 
>   src/docker/docker.hpp 38e5299ad38b9e20501387f2193b0fa448e49e3e 
>   src/docker/docker.cpp 1367de8a7bbbda6348a30e4ef4c616378e450250 
>   src/docker/executor.cpp 256d53d59d5cda63bbeb8c987ce0019e24b9fb77 
>   src/health-check/main.cpp 97b25716335ec5719c1100bd73d06b7fc98036c9 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/slave/containerizer/docker.cpp a17e4f21e7f5a1dfd47699ec84c7a48fd82294ad 
>   src/tests/health_check_tests.cpp 157a56aa06677d8b7a2cef53b29ed05cb4b5d8ea 
> 
> Diff: https://reviews.apache.org/r/37505/diff/
> 
> 
> Testing
> ---
> 
> # Add two new test cases, HealthCheckTest.ROOT_DOCKER_DockerHealthyTask and 
> HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
> # Docker health check command is run through "docker exec"
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthyTask" --verbose
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange" 
> --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37189: Added std::hash template specializations.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37187, 37188, 37189]

All tests passed.

- Mesos ReviewBot


On Aug. 26, 2015, 3:45 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37189/
> ---
> 
> (Updated Aug. 26, 2015, 3:45 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Michael Park.
> 
> 
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added std::hash template specializations.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp 92a0b4674d6058e27044f990c07dee922567fda6 
>   src/linux/cgroups.hpp 91ccfd046256d29504e93003de83c04f048c9b78 
>   src/master/metrics.hpp c51887ee1e072a063d629a735a3c6a2212e616ef 
>   src/messages/log.hpp 94e38c189703ff22c412b86429e052e942e75e75 
>   src/module/manager.hpp cab67a8b1ee7164bb3cb2a8b24e1a6e05b40fd19 
>   src/module/manager.cpp 862b71fa9d1e303e98de3767d69c9805e7b26dcc 
>   src/tests/containerizer.hpp 24b014f44d9eec56840e18cf39fbf9100f2c0711 
>   src/tests/fetcher_tests.cpp 81e70368f7a6164e9649964881aa5a688ef222e8 
>   src/tests/module.hpp 850c2002882be52e25f191f17785698deb5513c1 
> 
> Diff: https://reviews.apache.org/r/37189/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>