Review Request 46261: Added new MetricsTests with authentication to Mesos tests.

[Greg Mann]

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

Review request for mesos, Adam B and Alexander Rojas.


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


Repository: mesos


Description
---

The tests `MetricsTest.AgentAuthenticationEnabled` and
`MetricsTest.MasterAuthenticationEnabled` are added in
this patch.


Diffs
-

  src/tests/metrics_tests.cpp eacff678d06da7ba8afee6ab68261968561dffc3 

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


Testing
---

`sudo make check` on OSX.


Thanks,

Greg Mann

Review Request 46260: Added a MetricsTest with authentication to libprocess.

[Greg Mann]

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

Review request for mesos, Adam B and Alexander Rojas.


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


Repository: mesos


Description
---

The test `MetricsTest.SnapshotAuthenticationEnabled`
is added to the libprocess tests in this patch.


Diffs
-

  3rdparty/libprocess/src/tests/metrics_tests.cpp 
b84dc8d858f58bc9f52b218b7153510417cf34c2 

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


Testing
---

`sudo make check` on OSX.


Thanks,

Greg Mann

Review Request 46259: Added authentication to `/metrics/snapshot` endpoint.

[Greg Mann]

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

Review request for mesos, Adam B and Alexander Rojas.


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


Repository: mesos


Description
---

This patch adds authentication to the `/metrics/snapshot`
endpoint on the master and agent.


Diffs
-

  3rdparty/libprocess/include/process/metrics/metrics.hpp 
7abfadf6d030c52b7e03e773b67c74db6ad5b5df 
  3rdparty/libprocess/src/metrics/metrics.cpp 
7677dffed98153bc6276aad492a6815e67740ea3 
  3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 

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


Testing
---

`sudo make check` on OSX


Thanks,

Greg Mann

Review Request 46258: Added authentication to `/logging/toggle` endpoint.

[Greg Mann]

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

Review request for mesos, Adam B and Alexander Rojas.


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


Repository: mesos


Description
---

This patch adds authentication to the `/logging/toggle`
HTTP endpoint on the master and agent.


Diffs
-

  3rdparty/libprocess/include/process/logging.hpp 
0c0774647d8048a9d8d395141a5864eadc64f8ef 
  3rdparty/libprocess/src/logging.cpp bdbf716ee46a459f1004f0f4b72c23aae135f347 
  3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 

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


Testing
---

`sudo make check` on OSX.


Thanks,

Greg Mann

Review Request 46255: Added a realm parameter to 'process::initialize' (Mesos).

[Greg Mann]

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

Review request for mesos, Adam B and Alexander Rojas.


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


Repository: mesos


Description
---

In order to enable authentication on libprocess-level
HTTP endpoints, this patch adds code to the master and
agent's main.cpp file which makes use of the new
`authenticationRealm` argument to `process::initialize`
which allows the authentication realm of such endpoints
to be set when libprocess is initialized. The argument is
added to libprocess initialization in the tests as well.


Diffs
-

  src/master/main.cpp ea7f0fc87c8912309a4679105dde5d8d5bb9ead6 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/tests/main.cpp 142585096493a334ac9ac0df511ae0fc10798040 
  src/tests/mesos.hpp 20370a277d55efeea8daae7ea5e2f6575b5a2d62 

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


Testing
---

`sudo make check` on OSX.


Thanks,

Greg Mann

Re: Review Request 46254: Added a realm parameter to `process::initialize` (libprocess).

[Greg Mann]

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

(Updated April 15, 2016, 6:51 a.m.)


Review request for mesos, Adam B and Alexander Rojas.


Summary (updated)
-

Added a realm parameter to `process::initialize` (libprocess).


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


Repository: mesos


Description (updated)
---

In order to enable authentication on libprocess-level
HTTP endpoints, this patch adds a parameter to
process::initialize which allows the authentication
realm of such endpoints to be set when libprocess is
initialized.


Diffs (updated)
-

  3rdparty/libprocess/include/process/gtest.hpp 
3e0887257e1484813b3547170a5a1b9bb89de0d2 
  3rdparty/libprocess/include/process/process.hpp 
77e96957ca556cf9a7e2bf427d6762572fe48c51 
  3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
  3rdparty/libprocess/src/tests/main.cpp 
78858a2b84a439d8f8a60ec8bcb6ac3a308087a6 

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


Testing
---

`sudo make check` on OSX.


Thanks,

Greg Mann

Re: Review Request 45373: Ignored the DOCKER_VOLUME volume source.

[Guangya Liu]

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

(Updated 四月 15, 2016, 6:37 a.m.)


Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
Jie Yu.


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


Repository: mesos


Description
---

Ignored the DOCKER_VOLUME volume source.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
9fc7c48f99155750fd3c18c7c102507e2726362b 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 44131: Libprocess: [2/2] Implemented assorted `os::` functions on Windows.

[Daniel Pravat]

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

(Updated April 15, 2016, 6:32 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex 
Clemmer, Joris Van Remoortere, and Michael Park.


Repository: mesos


Description
---

Libprocess: [2/2] Implemented assorted `os::` functions on Windows.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp d2c458ed93307f75358bb642aaf2ed8e17b2fe97 

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


Testing
---

OSX: make check


Thanks,

Daniel Pravat

Re: Review Request 46242: Removed a check in Reserve operation validation.

[Greg Mann]

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

(Updated April 15, 2016, 6:25 a.m.)


Review request for mesos, Adam B and Joris Van Remoortere.


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


Repository: mesos


Description
---

When a dynamic reservation is made by an operator
or framework with no principal, this patch removes
the check which enforced that the principal in
ReservationInfo must be None() in that case.


Diffs (updated)
-

  src/master/validation.cpp 13423436a4e6361fde6fa75133eebf5c02c8381f 
  src/tests/master_validation_tests.cpp 
8a5bf9477596f13b2fb3a1348337ad2fe53a034d 
  src/tests/reservation_endpoints_tests.cpp 
f014290ed9f279df4c774aeb7ce7bd38fd1cc854 

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


Testing
---

`sudo make check`


Thanks,

Greg Mann

Review Request 46254: Added a default authentication realm to libprocess tests.

[Greg Mann]

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

Review request for mesos, Adam B and Ben Mahler.


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


Repository: mesos


Description
---

Both the initialization code in the libprocess tests
main.cpp as well as some of the test code needs access
to a common authentication realm; this patch adds such
a realm.


Diffs
-

  3rdparty/libprocess/include/process/gtest.hpp 
3e0887257e1484813b3547170a5a1b9bb89de0d2 
  3rdparty/libprocess/src/tests/main.cpp 
78858a2b84a439d8f8a60ec8bcb6ac3a308087a6 

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


Testing
---

`sudo make check` on OSX.


Thanks,

Greg Mann

Re: Review Request 46008: Stout: Initialize Windows socket stack in Stout tests.

[Alex Clemmer]

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

(Updated April 15, 2016, 5:58 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Repository: mesos


Description
---

Stout: Initialize Windows socket stack in Stout tests.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/socket.hpp 
b5aeab75c2365a3431c397743b95ce7fbd6d7a1a 
  3rdparty/libprocess/3rdparty/stout/tests/main.cpp 
c449de87bede4d3bf1df368eaf1d22f857c2298f 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46246: Log executor commands w/o verbose logs enabled.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46246]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 15, 2016, 2:30 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46246/
> ---
> 
> (Updated April 15, 2016, 2:30 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joseph Wu.
> 
> 
> Bugs: MESOS-5197
> https://issues.apache.org/jira/browse/MESOS-5197
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the logging in docker.cpp from VLOG to
> LOG(INFO). The reason is that, the command line that has
> been executed by executor is useful in debug purposes.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 19cf424dfd5748ae66a7023840aa2b0652e8f2c0 
> 
> Diff: https://reviews.apache.org/r/46246/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 45270: Added spec protobut for external mount.

[Guangya Liu]

> On 四月 15, 2016, 12:27 a.m., Jie Yu wrote:
> > Second thoughts on this. We should not use a single protobuf file for 
> > checkpointing. Otherwise, we'll have to write the entire file to the 
> > filesystem anytime we do a mount or umount. Let's just create some 
> > directory structure under /var/run/mesos/isolators/docker/volume. Please 
> > take a look at network/cni/paths.hpp|cpp.

OK, will create a separate patch for this.

The hierarchy of the checkpoint will be:

   /var/run/mesos/isolators/docker/volume
|- /
|  |-- mountinfo 
|-- /
| ...


- Guangya


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


On 四月 14, 2016, 4:41 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45270/
> ---
> 
> (Updated 四月 14, 2016, 4:41 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added spec protobut for external mount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.proto 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45270/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 46214: Valided role when framework subscribe.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46214]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 15, 2016, 1:51 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46214/
> ---
> 
> (Updated April 15, 2016, 1:51 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5184
> https://issues.apache.org/jira/browse/MESOS-5184
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Valided role when framework subscribe.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
>   src/tests/master_tests.cpp 1ae72600e26c5f31476f689cbb600d41d7cc9b74 
>   src/tests/scheduler_http_api_tests.cpp 
> d469adf7230ce5bb5e71a241a06e389295905e03 
> 
> Diff: https://reviews.apache.org/r/46214/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Review Request 46249: Support hook/module process operation before main process initialize.

[Andy Pang]

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

Review request for mesos, BenjaminVW BenjaminVW, Jie Yu, and Vinod Kone.


Repository: mesos


Description
---

If we in hook/module manager use libprocess library, it will auto use process 
initialize fucntion, as the process::initialize will only call once,then in the 
main fucnton process initialize  process::initialize("master") will not be 
called. So the HTTP request process "delegate" will be wrong, http request will 
not be response.


Diffs
-

  src/master/main.cpp ea7f0fc 

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


Testing
---

make check


Thanks,

Andy Pang

Re: Review Request 45270: Added spec protobut for external mount.

[Guangya Liu]

> On 四月 14, 2016, 10:27 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/spec.proto, line 19
> > 
> >
> > Why this namespace? This is technically not 'spec', but the _state_ 
> > scheme used by the isolator. We should call the protobuf file 'state.proto'.
> > 
> > I would suggest we still put it under `package mesos.internal.slave`, 
> > and call it `DockerVolumeMount`.
> > 
> > s/ExternalMountList/DockerVolumeState/

what about putting this under `package mesos.internal.slave.volume`?


> On 四月 14, 2016, 10:27 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/spec.proto, lines 
> > 21-35
> > 
> >
> > Why do you need to checkpoint 'container_path', 'mount_point' and 
> > 'driver_options'?

My original thought was adding more info here for debug, yes, we can remove 
them now.


- Guangya


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


On 四月 14, 2016, 4:41 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45270/
> ---
> 
> (Updated 四月 14, 2016, 4:41 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added spec protobut for external mount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.proto 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45270/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 46245: Renamed Docker and CNI BASE_NAME to meaningful name.

[Guangya Liu]

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

(Updated 四月 15, 2016, 2:35 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description
---

Renamed Docker and CNI BASE_NAME to meaningful name.


Diffs (updated)
-

  src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 46245: Renamed Docker and CNI BASE_NAME to meaningful name.

[Guangya Liu]

> On 四月 15, 2016, 2:19 a.m., Jie Yu wrote:
> > src/CMakeLists.txt, line 62
> > 
> >
> > You should update it, right?
> > 
> > Can you make sure you test the patch using cmake? Thanks!

was testing now, just noticed this. Thanks.


- Guangya


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


On 四月 15, 2016, 2:09 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46245/
> ---
> 
> (Updated 四月 15, 2016, 2:09 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed Docker and CNI BASE_NAME to meaningful name.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8 
> 
> Diff: https://reviews.apache.org/r/46245/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 46246: Log executor commands w/o verbose logs enabled.

[Yong Tang]

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

Review request for mesos, Artem Harutyunyan and Joseph Wu.


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


Repository: mesos


Description
---

This patch changes the logging in docker.cpp from VLOG to
LOG(INFO). The reason is that, the command line that has
been executed by executor is useful in debug purposes.


Diffs
-

  src/docker/docker.cpp 19cf424dfd5748ae66a7023840aa2b0652e8f2c0 

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


Testing
---

make check (Ubuntu 14.04)


Thanks,

Yong Tang

Re: Review Request 46242: Removed a check in Reserve operation validation.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46242]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 15, 2016, 1:23 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46242/
> ---
> 
> (Updated April 15, 2016, 1:23 a.m.)
> 
> 
> Review request for mesos, Adam B and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5212
> https://issues.apache.org/jira/browse/MESOS-5212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a dynamic reservation is made by an operator
> or framework with no principal, this patch removes
> the check which enforced that the principal in
> ReservationInfo must be None() in that case.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 13423436a4e6361fde6fa75133eebf5c02c8381f 
>   src/tests/master_validation_tests.cpp 
> 8a5bf9477596f13b2fb3a1348337ad2fe53a034d 
>   src/tests/reservation_endpoints_tests.cpp 
> f014290ed9f279df4c774aeb7ce7bd38fd1cc854 
> 
> Diff: https://reviews.apache.org/r/46242/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46245: Renamed Docker and CNI BASE_NAME to meaningful name.

[Jie Yu]

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




src/CMakeLists.txt (line 62)


You should update it, right?

Can you make sure you test the patch using cmake? Thanks!


- Jie Yu


On April 15, 2016, 2:09 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46245/
> ---
> 
> (Updated April 15, 2016, 2:09 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed Docker and CNI BASE_NAME to meaningful name.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8 
> 
> Diff: https://reviews.apache.org/r/46245/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 46245: Renamed Docker and CNI BASE_NAME to meaningful name.

[Guangya Liu]

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

Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description
---

Renamed Docker and CNI BASE_NAME to meaningful name.


Diffs
-

  src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 46149: Speed up DynamicReservationFramework.

[Klaus Ma]

> On April 14, 2016, 7:56 p.m., Benjamin Bannier wrote:
> > This is much better, but still long. I see that now most of the time is 
> > spent in `resourceOffers` -- can you think of a way to speed these up as 
> > well?
> 
> Klaus Ma wrote:
> @Bannier, how did you check the time in `resourceOffers`?
> 
> Benjamin Bannier wrote:
> I looked at the verbose log -- and obviously cannot convert microseconds 
> to seconds (duh). The time seems to be spent elsewhere.

There're ~3s in allocator: we'll launch 5 tasks on 3 agents, one task per agent 
once on reserved resources; so scheduler need `RESERVE`, `LAUNCH` and 
`UNRESERVE` to interact with master. I think we can reduce allocator interval.


- Klaus


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


On April 14, 2016, 8:59 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46149/
> ---
> 
> (Updated April 14, 2016, 8:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Bugs: MESOS-5166
> https://issues.apache.org/jira/browse/MESOS-5166
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> **Phenomenon**: The runtime of "ExamplesTest.DynamicReservationFramework" is 
> ~13s
> **Root Cause**: In dynamic reservation example, the framework will accept 
> offer when reserving resources, launching tasks and unreserving tasks. The 
> un-used resources will return to allocator with 5s RefusedFilters. It impact 
> the performance of this example.
> **How to Fix**: Set filters.refused_timeout to zero when launching task, 
> reserving/unreserving resources.
> 
> 
> Diffs
> -
> 
>   src/examples/dynamic_reservation_framework.cpp 
> 8f00bcf50c25cf46c3dc32e3e77370b39fbd46bc 
> 
> Diff: https://reviews.apache.org/r/46149/diff/
> 
> 
> Testing
> ---
> 
> make && make check GTEST_FILTER="ExamplesTest.*"
> 
> [==] Running 5 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 5 tests from ExamplesTest
> [ RUN  ] ExamplesTest.TestFramework
> [   OK ] ExamplesTest.TestFramework (5252 ms)
> [ RUN  ] ExamplesTest.NoExecutorFramework
> [   OK ] ExamplesTest.NoExecutorFramework (5407 ms)
> [ RUN  ] ExamplesTest.TestHTTPFramework
> [   OK ] ExamplesTest.TestHTTPFramework (1265 ms)
> [ RUN  ] ExamplesTest.PersistentVolumeFramework
> [   OK ] ExamplesTest.PersistentVolumeFramework (3379 ms)
> [ RUN  ] ExamplesTest.DynamicReservationFramework
> [   OK ] ExamplesTest.DynamicReservationFramework (5127 ms)
> [--] 5 tests from ExamplesTest (20433 ms total)
> 
> [--] Global test environment tear-down
> [==] 5 tests from 1 test case ran. (20443 ms total)
> [  PASSED  ] 5 tests.
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 46214: Valided role when framework subscribe.

[Klaus Ma]

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

(Updated April 15, 2016, 9:51 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Address @vinod's comments :).


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


Repository: mesos


Description
---

Valided role when framework subscribe.


Diffs (updated)
-

  src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
  src/tests/master_tests.cpp 1ae72600e26c5f31476f689cbb600d41d7cc9b74 
  src/tests/scheduler_http_api_tests.cpp 
d469adf7230ce5bb5e71a241a06e389295905e03 

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


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 44131: Libprocess: [2/2] Implemented assorted `os::` functions on Windows.

[Daniel Pravat]

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

(Updated April 15, 2016, 1:25 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex 
Clemmer, Joris Van Remoortere, and Michael Park.


Repository: mesos


Description
---

Libprocess: [2/2] Implemented assorted `os::` functions on Windows.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp d2c458ed93307f75358bb642aaf2ed8e17b2fe97 

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


Testing
---

OSX: make check


Thanks,

Daniel Pravat

Re: Review Request 46242: Removed a check in Reserve operation validation.

[Greg Mann]

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

(Updated April 15, 2016, 1:23 a.m.)


Review request for mesos, Adam B and Joris Van Remoortere.


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


Repository: mesos


Description
---

When a dynamic reservation is made by an operator
or framework with no principal, this patch removes
the check which enforced that the principal in
ReservationInfo must be None() in that case.


Diffs
-

  src/master/validation.cpp 13423436a4e6361fde6fa75133eebf5c02c8381f 
  src/tests/master_validation_tests.cpp 
8a5bf9477596f13b2fb3a1348337ad2fe53a034d 
  src/tests/reservation_endpoints_tests.cpp 
f014290ed9f279df4c774aeb7ce7bd38fd1cc854 

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


Testing
---

`sudo make check`


Thanks,

Greg Mann

Review Request 46242: Removed a check in Reserve operation validation.

[Greg Mann]

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

Review request for mesos, Adam B and Joris Van Remoortere.


Repository: mesos


Description
---

When a dynamic reservation is made by an operator
or framework with no principal, this patch removes
the check which enforced that the principal in
ReservationInfo must be None() in that case.


Diffs
-

  src/master/validation.cpp 13423436a4e6361fde6fa75133eebf5c02c8381f 
  src/tests/master_validation_tests.cpp 
8a5bf9477596f13b2fb3a1348337ad2fe53a034d 
  src/tests/reservation_endpoints_tests.cpp 
f014290ed9f279df4c774aeb7ce7bd38fd1cc854 

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


Testing
---

`sudo make check`


Thanks,

Greg Mann

Re: Review Request 45326: Added agent flag dvdcli_dir for specify dvdcli path.

[Guangya Liu]

> On 四月 14, 2016, 10:55 p.m., Jie Yu wrote:
> > docs/configuration.md, line 1411
> > 
> >
> > It's a directory or a file? Instead of doing that, can we just assume 
> > dvdcli is in 'PATH' and add a check in DockerVolumeDriverClient::create?

OK, will try to use `os::which` to handle this. There is a JIRA tracing it: 
https://issues.apache.org/jira/browse/MESOS-4576 , will try to upload a patch 
for it.


- Guangya


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


On 四月 14, 2016, 6:06 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> ---
> 
> (Updated 四月 14, 2016, 6:06 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5084
> https://issues.apache.org/jira/browse/MESOS-5084
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent flag dvdcli_dir for specify dvdcli path.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 46212: Added documentation around using AuthN for HTTP frameworks.

[Vinod Kone]

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



Update the docs based on the previous review's changes.

- Vinod Kone


On April 14, 2016, 3:34 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46212/
> ---
> 
> (Updated April 14, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds config docs around authenticating HTTP
> frameworks using the newly introduced flags. Also added a
> small note to the `authenticate` flag that this does not
> work for HTTP based frameworks.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md ba00ec563c449345effb3114111812601addcfc2 
> 
> Diff: https://reviews.apache.org/r/46212/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46211: Added flags for authenticating HTTP frameworks to master.

[Vinod Kone]

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




src/master/constants.hpp (line 132)


If and when we add AuthN support for agent <-> executor, what is that realm 
going to be? 'mesos-http-framework' or 'mesos-http-executor'? I guess it has to 
the latter because we bring up both master and agent in the same OS process in 
tests?

so should this be called mesos-http-scheduler instead? it's kinda 
unfortunate that we sometimes equate framework with scheduler and sometimes 
with framework and executor.



src/master/flags.cpp (line 482)


do we need a default here? we needed a default for `--http_authenticators` 
for backwards compatibility. since there is no backwards compatibility concern 
here, i think we should be ok with no default? having a default and not loading 
is a bit weird IMO.

remove the default and mention in the description  that this flag is 
required iff `--authenticate_http_frameworks` is set.



src/master/master.cpp (line 448)


Master allowing HTTP frameworks to register without authentication



src/master/master.cpp (line 674)


this whole logic can be cleaned up a bit if we make the flag required as 
suggested.


- Vinod Kone


On April 14, 2016, 3:34 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46211/
> ---
> 
> (Updated April 14, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two new flags `authenticate_http_frameworks`
> and `http_framework_authenticators` to the master. This allows us
> to selectively turn on/off framework authentication and decouple
> them from authentication for operator endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 7c7cc25fcc897dedb28001fbb944d2e50eca4713 
>   src/master/flags.hpp 83bb9088e595b393d610cc65479cb6a35fb31842 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
> 
> Diff: https://reviews.apache.org/r/46211/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 45014: Add /containers endpoint.

[Jie Yu]

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



ping?

- Jie Yu


On April 12, 2016, 9:11 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45014/
> ---
> 
> (Updated April 12, 2016, 9:11 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4891
> https://issues.apache.org/jira/browse/MESOS-4891
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It returns both resource statistics and container status.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp a684ff504535e2b9b2064f8048f5e03a0efcb059 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/containerizer.hpp efc1ca87c00b04d70efd1e3f6acf4e763132d6b0 
>   src/tests/containerizer.cpp 4c7f5a26d38222daf013bd3ca9a133e12d7bf338 
>   src/tests/slave_tests.cpp 4a576b98d1cc58072626ac2c41c599bd3c8385c5 
> 
> Diff: https://reviews.apache.org/r/45014/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 45270: Added spec protobut for external mount.

[Jie Yu]

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



Second thoughts on this. We should not use a single protobuf file for 
checkpointing. Otherwise, we'll have to write the entire file to the filesystem 
anytime we do a mount or umount. Let's just create some directory structure 
under /var/run/mesos/isolators/docker/volume. Please take a look at 
network/cni/paths.hpp|cpp.

- Jie Yu


On April 14, 2016, 4:41 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45270/
> ---
> 
> (Updated April 14, 2016, 4:41 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added spec protobut for external mount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.proto 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45270/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 46114: Fixed per framework principal metrics for HTTP frameworks.

[Vinod Kone]

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


Fix it, then Ship it!





src/master/master.cpp (lines 5945 - 5948)


can you use a ternary operator here instead?


- Vinod Kone


On April 14, 2016, 3:34 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46114/
> ---
> 
> (Updated April 14, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use the `principal` set in `FrameworkInfo.principal` to populate
> framework metrics instead of looking them up in `authenticated`
> map. It also ensures that HTTP->PID framework downgrades preserve 
> the metrics and no additional logic is needed to deal with this. 
> (Previously HTTP frameworks never had AuthN and no principal was 
> passed in `FrameworkInfo`, so this was never a concern).
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
> 
> Diff: https://reviews.apache.org/r/46114/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::` functions on Windows.

[Daniel Pravat]

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

(Updated April 15, 2016, 12:23 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex 
Clemmer, Joris Van Remoortere, and Michael Park.


Repository: mesos


Description
---

Stout: [1/2] Implemented assorted `os::` functions on Windows.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
edaa76a5322d0bf60b7172405aa754b5aca95458 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 

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


Testing
---

OSX: make check


Thanks,

Daniel Pravat

Re: Review Request 46116: Added basic authentication scheme to the scheduler library.

[Anand Mazumdar]

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

(Updated April 15, 2016, 12:11 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comment from Vinod


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


Repository: mesos


Description
---

This change adds basic scheme AuthN support to the library.
It would be good to add support for additional schemes in the
future.


Diffs (updated)
-

  include/mesos/v1/scheduler.hpp 6603075fe187ac7d1b56381ffc6e18f14adbfb5a 
  src/scheduler/scheduler.cpp f9d54f99f3086a8a4349abd3892413fa6bd44437 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 46116: Added basic authentication scheme to the scheduler library.

[Anand Mazumdar]

> On April 13, 2016, 10:05 p.m., Vinod Kone wrote:
> >
> 
> Vinod Kone wrote:
> Also, I'm wondering if it would be better for the library to take 
> Authorization string as a parameter instead of Credential. That way it is 
> more generic because the library can just set "Authorization: 
> ". The idea is that scheduler will use a library to do any 
> type of auth it needs (e.g. kerberos, JWT) and finally gives the scheduler 
> library the AuthZ header. We could maybe make it even more generic by taking 
> a list of headers.

IIUC, the client still needs to send an initial request to the server with the 
login credentials. If the login request succeeds, the server would return a 
token back to the client. All subsequent requests from the client can then 
include the token as part of the `Authorization` header. So, we would still 
need to pass the credentials to the library. We can later change the logic in 
the library to look for the token as part of the response from server and then 
send it for subsequent requests.


- Anand


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


On April 12, 2016, 10:26 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46116/
> ---
> 
> (Updated April 12, 2016, 10:26 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4459
> https://issues.apache.org/jira/browse/MESOS-4459
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds basic scheme AuthN support to the library.
> It would be good to add support for additional schemes in the
> future.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/scheduler.hpp 6603075fe187ac7d1b56381ffc6e18f14adbfb5a 
>   src/scheduler/scheduler.cpp f9d54f99f3086a8a4349abd3892413fa6bd44437 
> 
> Diff: https://reviews.apache.org/r/46116/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46209: Explicitly set `FrameworkInfo.principal` if AuthN is enabled.

[Vinod Kone]

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


Fix it, then Ship it!





src/master/master.cpp (lines 2370 - 2372)


```
LOG(WARNING) << "Setting 'principal' in FrameworkInfo to '" << 
authenticated[from]
 << "' because the framework authenticated with that principal 
but did"
 << " not set it in FrameworkInfo";
```


- Vinod Kone


On April 14, 2016, 3:33 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46209/
> ---
> 
> (Updated April 14, 2016, 3:33 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5214
> https://issues.apache.org/jira/browse/MESOS-5214
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change explicitly sets `FrameworkInfo.principal` if
> AuthN is enabled for old driver based frameworks. The
> value gets defaulted to the authenticated principal (if missing).
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 1f480f03900a0dc2996c2ed3a9534dfa8036940f 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
> 
> Diff: https://reviews.apache.org/r/46209/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46214: Valided role when framework subscribe.

[Vinod Kone]

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




src/tests/master_tests.cpp (lines 4081 - 4083)


s/err/error/



src/tests/master_tests.cpp (line 4088)


Instead of driver.join() you should do a ASSERT_READY(error) to make sure 
that the error has happened. You can still do a driver.join() afterwards, 
though strictly not necessary.


- Vinod Kone


On April 14, 2016, 4:11 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46214/
> ---
> 
> (Updated April 14, 2016, 4:11 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5184
> https://issues.apache.org/jira/browse/MESOS-5184
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Valided role when framework subscribe.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
>   src/tests/master_tests.cpp 1ae72600e26c5f31476f689cbb600d41d7cc9b74 
>   src/tests/scheduler_http_api_tests.cpp 
> d469adf7230ce5bb5e71a241a06e389295905e03 
> 
> Diff: https://reviews.apache.org/r/46214/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 46230: Updated docs to reflect user in persistent volumes.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46227, 46228, 46229, 46230]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 14, 2016, 11:04 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46230/
> ---
> 
> (Updated April 14, 2016, 11:04 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated docs to reflect user in persistent volumes.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md e5f2836c8867d5221da9e4f89167850ef9dab8ec 
> 
> Diff: https://reviews.apache.org/r/46230/diff/
> 
> 
> Testing
> ---
> 
> Documentation change only. All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.

[Alex Clemmer]

> On April 12, 2016, 3:09 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp, lines 22-23
> > 
> >
> > This seems generally useful?
> > I think I've even seen it in other reviews?
> > Why is it burried in a test?
> 
> Alex Clemmer wrote:
> We are doing this to stay consistent with Neil Conway's pass over the 
> tests to use `std::string`. For the general codebase, maybe it's useful, and 
> maybe it's not, I'm not sure.
> 
> Joris Van Remoortere wrote:
> Sorry I must have messed up the selection highlight.
> I meant this: `const string errorMessage(int err)`.

We already have code like this in `WindowsError`, and slightly different code 
like this in `os::hstrerror`.

The thinking was, the tests below are calling `::strerror` and comparing them 
to `os::strerror`, and what we'd really like to do is just call 
`::FormatMessage` straight up, but it's a bit too verbose to embed directly in 
the test. Hence, I wrote a small helper function.

Let me know what else you would like to see here.


- Alex


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


On April 13, 2016, 10:05 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46009/
> ---
> 
> (Updated April 13, 2016, 10:05 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout:[1/2] Added simple tests for `os::` functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 400c6dc451602926f93b22713af8c66d7ca59ca6 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> c9d331df2f4496183b5734d2434413f68b9c1b4b 
>   3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp 
> d5a96ad6b8b1c09b4f016e0c8e3e5c5672b55ef3 
> 
> Diff: https://reviews.apache.org/r/46009/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46189: Slave rename - Update strings in error messages and other strings.

[zhou xing]

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

(Updated 四月 14, 2016, 11:27 p.m.)


Review request for mesos, Kevin Klues and Vinod Kone.


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


Repository: mesos


Description
---

[#MESOS-5057]
This patch renames 'slave' to 'agent' in the following strings:
1. Error/Warning messages
2. Flag help/description messages
3. Test case messages
4. Other standard output messages


Diffs
-

  CHANGELOG 1e07c8c2de8eff87c171378ef207c91a20d435d9 
  src/common/resources_utils.cpp 70e6f025d89383084ab8b2cda23ab1cd55d959b2 
  src/examples/dynamic_reservation_framework.cpp 
8f00bcf50c25cf46c3dc32e3e77370b39fbd46bc 
  src/hook/manager.cpp 692b9ea862442f3bac58da678425f03e4b00a79d 
  src/internal/devolve.cpp 0f58dc151c3de5a25d0ca73030a8222cac0f43c1 
  src/local/flags.hpp b3cd811c78bb7b57d82d755e36a89269d8173c2d 
  src/logging/flags.cpp 446eb921443481f4025132653e0bfa8ab42aa240 
  src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
  src/master/http.cpp b8a83b58b60416f61610cad16fc6f70028a5ee10 
  src/master/master.hpp 1f480f03900a0dc2996c2ed3a9534dfa8036940f 
  src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
  src/master/validation.cpp 13423436a4e6361fde6fa75133eebf5c02c8381f 
  src/python/executor/src/mesos/executor/mesos_executor_driver_impl.cpp 
843771ae5b4f7e283d85c093e63c235dc753397d 
  src/python/scheduler/src/mesos/scheduler/mesos_scheduler_driver_impl.cpp 
78dc298ec19a61cd491b2b43b463db67528c5526 
  src/scaling/scaling_sched.py d1008c79b1f1302d9ff6ae6dc9e1d9d0b7379773 
  src/sched/sched.cpp 5f6f5518f0858c680dc0dffc933c0bb03bba6991 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
4f3f210b8d0ab9a453ab56c5e23024e2ab7c4259 
  src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
  src/slave/http.cpp 922aaad6e83ca9d5ab503ddb733a332982843300 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/slave/slave.cpp 49fa4a06e26d0d4475ed50db254a320a7030f896 
  src/slave/state.cpp e7b44c78500e07f0f1655f41a6a977adae2ca0c0 
  src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
  src/tests/containerizer/docker_containerizer_tests.cpp 
7accd32fba5eed196a82b1a171cb16d37b9e0539 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
51e60c2e8c6c8b76b51de0e7761ecbb0ca9c3304 
  src/tests/flags.hpp af15360491b5433dcfbda03a72407667eb3977d1 
  src/tests/hierarchical_allocator_tests.cpp 
8ed0df45fc745d338482d1944a346cd40c18bb37 
  src/tests/master_allocator_tests.cpp 17607df7d488c8dd42c57504a5ca326697f57ffa 
  src/tests/master_tests.cpp 1ae72600e26c5f31476f689cbb600d41d7cc9b74 
  src/tests/paths_tests.cpp 81498e368cbb77e2cb8af71d8570dc690d3d0dfc 
  src/tests/slave_recovery_tests.cpp 79132344be3bcd2bda54357cd5e7e0c59a766fd8 
  src/tests/sorter_tests.cpp 0f3266f1222163c4d03eb4c4ca88f96836de601e 

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


Testing (updated)
---

Perform the following regular expression on 'mesos/src' and 'mesos/support' 
folder to find out all the message strings(in Eclipse IDE):
```
^[^#include].*".*slave.*"
```
then filter out the lines that using term 'slave' as the field name or 
attribute name

make
make check


Thanks,

zhou xing

Re: Review Request 46190: Slave rename - Update standard output messages in libprocess.

[zhou xing]

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

(Updated 四月 14, 2016, 11:26 p.m.)


Review request for mesos, Kevin Klues and Vinod Kone.


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


Repository: mesos


Description
---

[#MESOS-5057]
Rename 'slave' to 'agent' in the standard output messages of
libprocess


Diffs
-

  3rdparty/libprocess/src/test-master.cpp 
03f2f83613674b3b9edfe76b76adb9b2513c1f06 
  3rdparty/libprocess/src/test-slave.cpp 
9797792d79448dc8e9ed31ef689fa9655250c1c4 

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


Testing (updated)
---

Perform the following regular expression on 'mesos/3rdparty' folder to find out 
all the message strings(in Eclipse IDE):
```
^[^#include].*".*slave.*"
```
then filter out the lines that using term 'slave' as the field name or 
attribute name

make
make check


Thanks,

zhou xing

Re: Review Request 46190: Slave rename - Update standard output messages in libprocess.

[zhou xing]

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

(Updated 四月 14, 2016, 11:25 p.m.)


Review request for mesos, Kevin Klues and Vinod Kone.


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


Repository: mesos


Description
---

[#MESOS-5057]
Rename 'slave' to 'agent' in the standard output messages of
libprocess


Diffs
-

  3rdparty/libprocess/src/test-master.cpp 
03f2f83613674b3b9edfe76b76adb9b2513c1f06 
  3rdparty/libprocess/src/test-slave.cpp 
9797792d79448dc8e9ed31ef689fa9655250c1c4 

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


Testing (updated)
---

Perform the following regular expression on 'mesos/src' and 'mesos/support' 
folder to find out all the message strings(in Eclipse IDE):
```
^[^#include].*".*slave.*"
```
then filter out the lines that using term 'slave' as the field name or 
attribute name

make
make check


Thanks,

zhou xing

Re: Review Request 46227: Added an user to indicate owner of persistent volume.

[Anindya Sinha]

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

(Updated April 14, 2016, 11:07 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased to HEAD.


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


Repository: mesos


Description
---

When a persistent volume is created, it is created with the user
indicated in Resource::DiskInfo::Persistence. If missing, it would
fall back to make the user of the mesos-slave process as the owner
of the persistent volume.


Diffs (updated)
-

  include/mesos/mesos.proto 87af4a06db8cc3889fe4d3b314206103f5ce5f2d 
  include/mesos/v1/mesos.proto 34da0a1484dc2f71262d8b97484b8edaae35bb6c 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Review Request 46229: Add unit tests for adding a user for persistent volumes.

[Anindya Sinha]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Add unit tests for adding a user for persistent volumes.


Diffs
-

  src/tests/containerizer/filesystem_isolator_tests.cpp 
51e60c2e8c6c8b76b51de0e7761ecbb0ca9c3304 
  src/tests/mesos.hpp 20370a277d55efeea8daae7ea5e2f6575b5a2d62 
  src/tests/persistent_volume_tests.cpp 
d246f35046fff469b847c908de2b305ae629212f 
  src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 

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


Testing
---

All tests passed (including the newly added tests).


Thanks,

Anindya Sinha

Review Request 46227: Added an user to indicate owner of persistent volume.

[Anindya Sinha]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

When a persistent volume is created, it is created with the user
indicated in Resource::DiskInfo::Persistence. If missing, it would
fall back to make the user of the mesos-slave process as the owner
of the persistent volume.


Diffs
-

  include/mesos/mesos.proto 87af4a06db8cc3889fe4d3b314206103f5ce5f2d 
  include/mesos/v1/mesos.proto 34da0a1484dc2f71262d8b97484b8edaae35bb6c 
  src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8 
  src/Makefile.am 1ae0ea7f3ff6f136d58ef08e7b11185ea19f37a8 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
786f917c0ec04b6839bbd524fa7c8de3729f9bdb 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
0ad473dc3bd45e122fba55a670e1a893e61c977a 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Review Request 46230: Updated docs to reflect user in persistent volumes.

[Anindya Sinha]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Updated docs to reflect user in persistent volumes.


Diffs
-

  docs/persistent-volume.md e5f2836c8867d5221da9e4f89167850ef9dab8ec 

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


Testing
---

Documentation change only. All tests passed.


Thanks,

Anindya Sinha

Review Request 46228: Create persistent volume with a supplied user.

[Anindya Sinha]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

If user is specified in Resource::DiskInfo::Persistence, set the
ownership of the volume to that user. Tasks should have to comply to
ownership of the volume before they can use the volume.
If user is not specified in Resource::DiskInfo::Persistence, it is
created with the user mesos-slave process runs as. When a task is
launched, it updates the ownership of the persistent volume with its
user so as to be able to write into that persistent volume.


Diffs
-

  src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
9fc7c48f99155750fd3c18c7c102507e2726362b 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
01c0ad6dbb6d509e62e769365586b3d23dcb240d 
  src/slave/slave.cpp de99e9eb5cc812b2e07deb749b98b4f4db363728 
  src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 46180: Implemented create() for docker volume isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 59 - 
74)


we should add a os::which to check if dvdcli is in PATH or not.


- Jie Yu


On April 14, 2016, 1:40 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46180/
> ---
> 
> (Updated April 14, 2016, 1:40 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented create() for docker volume isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46180/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45326: Added agent flag dvdcli_dir for specify dvdcli path.

[Jie Yu]

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




docs/configuration.md (line 1411)


It's a directory or a file? Instead of doing that, can we just assume 
dvdcli is in 'PATH' and add a check in DockerVolumeDriverClient::create?


- Jie Yu


On April 14, 2016, 6:06 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> ---
> 
> (Updated April 14, 2016, 6:06 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5084
> https://issues.apache.org/jira/browse/MESOS-5084
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent flag dvdcli_dir for specify dvdcli path.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45373: Ignored the DOCKER_VOLUME volume source.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 469 - 470)


Can we just check `volume.has_source()` only? It does not support anyway.


- Jie Yu


On April 13, 2016, 9:26 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45373/
> ---
> 
> (Updated April 13, 2016, 9:26 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignored the DOCKER_VOLUME volume source.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 9fc7c48f99155750fd3c18c7c102507e2726362b 
> 
> Diff: https://reviews.apache.org/r/45373/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45360: Added volume client for mount and unmount.

[Jie Yu]

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




src/CMakeLists.txt (line 145)


s/volume_client/driver.cpp

No need for the 'volume' since it's already in the directory.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 
31)


Remove this.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 
37)


s/VolumeClient/DockerVolumeDriverClient/



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 
46)


Why virtual? Let's not make them virtual yet.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 
52)


```
// Returns the path of the mount point.
Future mount(
const string& driver,
const string& name,
const hashmap& options);
```



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp (line 
57)


```
Future unmount(
const string& driver,
const string& name);
```


- Jie Yu


On April 14, 2016, 6:05 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated April 14, 2016, 6:05 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added volume client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f 
>   src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::` functions on Windows.

[Alex Clemmer]

> On April 12, 2016, 2:57 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, lines 
> > 188-189
> > 
> >
> > Where did `processes()` go?

It goes into a later review. We can add back the `= delete` until then.


- Alex


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


On April 11, 2016, 4:20 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44123/
> ---
> 
> (Updated April 11, 2016, 4:20 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex 
> Clemmer, Joris Van Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: [1/2] Implemented assorted `os::` functions on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/44123/diff/
> 
> 
> Testing
> ---
> 
> OSX: make check
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 45270: Added spec protobut for external mount.

[Jie Yu]

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




src/CMakeLists.txt (lines 52 - 54)


the naming here looks bad. We should rename them to:
```
PROVISIONER_DOCKER
ISOLATOR_CNI_SPEC
ISOLATOR_DOCKER_VOLUME_SPEC
```



src/CMakeLists.txt (line 63)


You need to update the protobuf here as well (also adding the new one you 
just added).



src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp (line 29)


Let's not yet introduce file yet.



src/slave/containerizer/mesos/isolators/docker/volume/spec.proto (line 19)


Why this namespace? This is technically not 'spec', but the _state_ scheme 
used by the isolator. We should call the protobuf file 'state.proto'.

I would suggest we still put it under `package mesos.internal.slave`, and 
call it `DockerVolumeMount`.

s/ExternalMountList/DockerVolumeState/



src/slave/containerizer/mesos/isolators/docker/volume/spec.proto (lines 21 - 35)


Why do you need to checkpoint 'container_path', 'mount_point' and 
'driver_options'?


- Jie Yu


On April 14, 2016, 4:41 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45270/
> ---
> 
> (Updated April 14, 2016, 4:41 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added spec protobut for external mount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.proto 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45270/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45905: Added metrics to the balloon framework.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [45905, 45604]

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

Error:
2016-04-14 22:23:18 URL:https://reviews.apache.org/r/45604/diff/raw/ 
[23649/23649] -> "45604.patch" [1]
error: patch failed: src/examples/balloon_executor.cpp:24
error: src/examples/balloon_executor.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/12533/console

- Mesos ReviewBot


On April 14, 2016, 9:43 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45905/
> ---
> 
> (Updated April 14, 2016, 9:43 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5174
> https://issues.apache.org/jira/browse/MESOS-5174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds metrics to gauge the health of the framework.  This includes:
> 
> * uptime_secs = How long the framework has been running.
> * registered = If the framework is registered.
> * tasks_finished = Number of tasks finished (successfully).
> * tasks_oomed = Number of tasks that were OOM killed.
> * allowed_terminations = Number of terminal status updates which
>   are acceptable due to infrastructure reasons.
> * abnormal_terminations = Number of terminal status updates which 
>   were not `TASK_FINISHED` or `TASK_FAILED` due to OOM.
> 
> 
> Diffs
> -
> 
>   src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3 
> 
> Diff: https://reviews.apache.org/r/45905/diff/
> 
> 
> Testing
> ---
> 
> ```
> make check
> 
> sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
> 
> # Also launched two instances on a cluster.
> # This one OOM's:
> ./balloon-framework --master=zk://localhost:2181/mesos --checkpoint 
> --balloon_limit=256MB --task_memory=128MB 
> --executor_uri="https://s3.amazonaws.com/url/to/balloon-executor"; 
> --executor_command="LD_LIBRARY_PATH=/path/to/libmesos && ./balloon-executor"
> 
> # This one does not OOM:
> ./balloon-framework --master=zk://localhost:2181/mesos --checkpoint 
> --balloon_limit=256MB --task_memory=256MB 
> --executor_uri="https://s3.amazonaws.com/url/to/balloon-executor"; 
> --executor_command="LD_LIBRARY_PATH=/path/to/libmesos && ./balloon-executor"
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 46193: Stout:[2/2] Added `systems_tests.cpp`.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On April 14, 2016, 8:28 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46193/
> ---
> 
> (Updated April 14, 2016, 8:28 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4470
> https://issues.apache.org/jira/browse/MESOS-4470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout:[2/2] Added `systems_tests.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> 217283cd3ce6df699b63dc5b8fb3aab0c6debd04 
> 
> Diff: https://reviews.apache.org/r/46193/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46192: Stout:[1/2] Added `systems_tests.cpp`.

[Michael Park]

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




3rdparty/libprocess/3rdparty/stout/tests/os/systems_tests.cpp (lines 41 - 45)


I understand that these are new tests, but are they only true for Windows? 
or is it general enough that we can pull it out of the Windows section?

```
EXPECT_GT(info.get().release.size(), 0);
```

would be better, or maybe:

```
EXPECT_FALSE(info.get().release.empty());
```


- Michael Park


On April 14, 2016, 8:28 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46192/
> ---
> 
> (Updated April 14, 2016, 8:28 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4470
> https://issues.apache.org/jira/browse/MESOS-4470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout:[1/2] Added `systems_tests.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 400c6dc451602926f93b22713af8c66d7ca59ca6 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> c9d331df2f4496183b5734d2434413f68b9c1b4b 
>   3rdparty/libprocess/3rdparty/stout/tests/os/systems_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> 9bd34c7508cd813c5de18028956f6a740997c266 
> 
> Diff: https://reviews.apache.org/r/46192/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46220: Added documentation for Nvidia GPU support.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45969, 46053, 45970, 46220]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 14, 2016, 8:33 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46220/
> ---
> 
> (Updated April 14, 2016, 8:33 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5221
> https://issues.apache.org/jira/browse/MESOS-5221
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for Nvidia GPU support.
> 
> 
> Diffs
> -
> 
>   docs/gpu-support.md PRE-CREATION 
>   docs/home.md dc41fc4479e6c23650cd8ac78dcc4b9161d00721 
> 
> Diff: https://reviews.apache.org/r/46220/diff/
> 
> 
> Testing
> ---
> 
> Ran a local copy of the documentation site to make sure everything looks OK.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 46191: Stout: Implemented `uname` on Windows.

[Michael Park]

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




3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 44)


How about naming this `nodename` to match the field name?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 64)


How about naming this `machine` to match the field name?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 98 - 99)


This is the correct format? Do we not want `major.minor.patch`? If we do 
want `major.minor.patch`, then we could just do `return 
stringify(Version(os_version.dwMajorVersion, os_version.dwMinorVersion, 0));`



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 308 - 
312)


Can we pull this out to `Try internal::os_version();`?



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 310)


Please use a C++ cast.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 326 - 
330)


We can use `internal::os_version` here instead if we pull it out as 
suggested above.



3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 355 - 
360)


If we pull out `internal::os_version`, we could do less work here:

```
Try os_version = internal::os_version();
if (os_version.isError()) {
  return Error(os_version.error());
}

return internal::sysname(os_version);
```

If the idea is to keep the implementation consistent with `posix/os.hpp` 
that sounds fine as well, in which case we should just define this in `os.hpp`.


- Michael Park


On April 14, 2016, 8:28 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46191/
> ---
> 
> (Updated April 14, 2016, 8:28 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4470
> https://issues.apache.org/jira/browse/MESOS-4470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `uname` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46191/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 45604: Updated the balloon framework and executor.

[Joseph Wu]

> On April 12, 2016, 3:46 p.m., Vinod Kone wrote:
> > src/examples/balloon_framework.cpp, lines 69-90
> > 
> >
> > The interaction between these flags is a bit unclear.
> > 
> > If some doesn't set `task_memory` but sets `balloon_limit`, the 
> > container might or might not OOM. It depends on how much is offerred to the 
> > scheduler. I would rather make this a required field. Infact, I would also 
> > add a required flag for executor memory (--executor_memory).
> > 
> > Also s/balloon_limit/executor_memory_usage/ for clarity.
> 
> Joseph Wu wrote:
> I did it this way to maintain the existing behavior (which I agree 
> doesn't make very much sense).

Note: I renamed to `--task_memory_usage_limit`.


> On April 12, 2016, 3:46 p.m., Vinod Kone wrote:
> > src/examples/balloon_framework.cpp, line 330
> > 
> >
> > This comment doesn't make sense?

Oops, removed.  That was a copy-paste :)


- Joseph


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


On April 14, 2016, 2:43 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45604/
> ---
> 
> (Updated April 14, 2016, 2:43 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5174
> https://issues.apache.org/jira/browse/MESOS-5174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This gives the example `balloon-framework` enough options to run
> outside of the build environment.
> 
> This also updates:
> 
> * The style of the framework code.
> * Adds an option for restricting the number of resources per task
>   (otherwise, it will eat up an entire node).
> * Adds an option for persisting the framework and launching one task
>   after another.
> * Adds filters for declined offers.
> * Refines the shutdown logic for the executor.  In particular, ironed 
>   out bugs when the balloon executor does not exceed the memory limit.
> 
> 
> Diffs
> -
> 
>   src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a 
>   src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3 
>   src/tests/balloon_framework_test.sh 
> ae32753e8942f77f94752543c384d218d6e4d48d 
> 
> Diff: https://reviews.apache.org/r/45604/diff/
> 
> 
> Testing
> ---
> 
> ```
> make check 
> 
> sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 45905: Added metrics to the balloon framework.

[Joseph Wu]

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

(Updated April 14, 2016, 2:43 p.m.)


Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod 
Kone.


Changes
---

Accidentally broke the test (previous commit).  Fixed now.


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


Repository: mesos


Description
---

Adds metrics to gauge the health of the framework.  This includes:

* uptime_secs = How long the framework has been running.
* registered = If the framework is registered.
* tasks_finished = Number of tasks finished (successfully).
* tasks_oomed = Number of tasks that were OOM killed.
* allowed_terminations = Number of terminal status updates which
  are acceptable due to infrastructure reasons.
* abnormal_terminations = Number of terminal status updates which 
  were not `TASK_FINISHED` or `TASK_FAILED` due to OOM.


Diffs (updated)
-

  src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3 

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


Testing
---

```
make check

sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"

# Also launched two instances on a cluster.
# This one OOM's:
./balloon-framework --master=zk://localhost:2181/mesos --checkpoint 
--balloon_limit=256MB --task_memory=128MB 
--executor_uri="https://s3.amazonaws.com/url/to/balloon-executor"; 
--executor_command="LD_LIBRARY_PATH=/path/to/libmesos && ./balloon-executor"

# This one does not OOM:
./balloon-framework --master=zk://localhost:2181/mesos --checkpoint 
--balloon_limit=256MB --task_memory=256MB 
--executor_uri="https://s3.amazonaws.com/url/to/balloon-executor"; 
--executor_command="LD_LIBRARY_PATH=/path/to/libmesos && ./balloon-executor"
```


Thanks,

Joseph Wu

Re: Review Request 45604: Updated the balloon framework and executor.

[Joseph Wu]

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

(Updated April 14, 2016, 2:43 p.m.)


Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod 
Kone.


Changes
---

Reworked some flags and variable names + comments.


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


Repository: mesos


Description
---

This gives the example `balloon-framework` enough options to run
outside of the build environment.

This also updates:

* The style of the framework code.
* Adds an option for restricting the number of resources per task
  (otherwise, it will eat up an entire node).
* Adds an option for persisting the framework and launching one task
  after another.
* Adds filters for declined offers.
* Refines the shutdown logic for the executor.  In particular, ironed 
  out bugs when the balloon executor does not exceed the memory limit.


Diffs (updated)
-

  src/examples/balloon_executor.cpp 399218478a596387814d123290eeeb33102fad7a 
  src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3 
  src/tests/balloon_framework_test.sh ae32753e8942f77f94752543c384d218d6e4d48d 

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


Testing
---

```
make check 

sudo bin/mesos-tests.sh --gtest_filter="*ROOT_CGROUPS_BalloonFramework"
```


Thanks,

Joseph Wu

Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::` functions on Windows.

[Alex Clemmer]

> On April 13, 2016, 2:38 a.m., Alex Naparu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 168
> > 
> >
> > Do we no longer have a "not implemented" error?

Per our conversation on Slack, we have decided that we never did.


> On April 13, 2016, 2:38 a.m., Alex Naparu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 86
> > 
> >
> > Is it worth handling the error case here?

Ok, let's do it. In the case that the `FormatMessage` call failed, I choose to 
just output the error code instead of calling `FormatMessage` again, though. It 
can't be turtles _all_ the way down!


> On April 13, 2016, 2:38 a.m., Alex Naparu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 232
> > 
> >
> > We don't have a X_OK flag in Windows, so we might want to mask that out 
> > before calling `::access`.

Here's my perpective. If we try to call this with `X_OK` we will get a symbol 
undefined error. Also, if someone `#define`s it themselves, masking won't help. 
Also, we have a pattern for dealing with `#define`s, which is usually to 
explicitly not mask out flags that we don't `#define`.

Thoughts?


- Alex


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


On April 11, 2016, 4:20 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44123/
> ---
> 
> (Updated April 11, 2016, 4:20 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex 
> Clemmer, Joris Van Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: [1/2] Implemented assorted `os::` functions on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/44123/diff/
> 
> 
> Testing
> ---
> 
> OSX: make check
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 45360: Added volume client for mount and unmount.

[Gilbert Song]

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




src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (lines 
60 - 61)


Let's use `https://github.com/emccode/rexray`, which is under maintainance.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 
102)


ditto.


- Gilbert Song


On April 13, 2016, 11:05 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated April 13, 2016, 11:05 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added volume client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f 
>   src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 46015: Stout:[2/2] Moved process tests to their own file.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On April 11, 2016, 9:02 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46015/
> ---
> 
> (Updated April 11, 2016, 9:02 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout:[2/2] Moved process tests to their own file.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> 217283cd3ce6df699b63dc5b8fb3aab0c6debd04 
> 
> Diff: https://reviews.apache.org/r/46015/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46190: Slave rename - Update standard output messages in libprocess.

[Kevin Klues]

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



Can you comment in the testing section how you verified that you covered 
everything?

- Kevin Klues


On April 14, 2016, 8:01 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46190/
> ---
> 
> (Updated April 14, 2016, 8:01 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-5057
> https://issues.apache.org/jira/browse/MESOS-5057
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> [#MESOS-5057]
> Rename 'slave' to 'agent' in the standard output messages of
> libprocess
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/test-master.cpp 
> 03f2f83613674b3b9edfe76b76adb9b2513c1f06 
>   3rdparty/libprocess/src/test-slave.cpp 
> 9797792d79448dc8e9ed31ef689fa9655250c1c4 
> 
> Diff: https://reviews.apache.org/r/46190/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 46189: Slave rename - Update strings in error messages and other strings.

[Kevin Klues]

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



Can you comment in the testing section how you verified that you covered 
everything?

- Kevin Klues


On April 14, 2016, 7:28 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46189/
> ---
> 
> (Updated April 14, 2016, 7:28 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-5057
> https://issues.apache.org/jira/browse/MESOS-5057
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> [#MESOS-5057]
> This patch renames 'slave' to 'agent' in the following strings:
> 1. Error/Warning messages
> 2. Flag help/description messages
> 3. Test case messages
> 4. Other standard output messages
> 
> 
> Diffs
> -
> 
>   CHANGELOG 1e07c8c2de8eff87c171378ef207c91a20d435d9 
>   src/common/resources_utils.cpp 70e6f025d89383084ab8b2cda23ab1cd55d959b2 
>   src/examples/dynamic_reservation_framework.cpp 
> 8f00bcf50c25cf46c3dc32e3e77370b39fbd46bc 
>   src/hook/manager.cpp 692b9ea862442f3bac58da678425f03e4b00a79d 
>   src/internal/devolve.cpp 0f58dc151c3de5a25d0ca73030a8222cac0f43c1 
>   src/local/flags.hpp b3cd811c78bb7b57d82d755e36a89269d8173c2d 
>   src/logging/flags.cpp 446eb921443481f4025132653e0bfa8ab42aa240 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/master/http.cpp b8a83b58b60416f61610cad16fc6f70028a5ee10 
>   src/master/master.hpp 1f480f03900a0dc2996c2ed3a9534dfa8036940f 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
>   src/master/validation.cpp 13423436a4e6361fde6fa75133eebf5c02c8381f 
>   src/python/executor/src/mesos/executor/mesos_executor_driver_impl.cpp 
> 843771ae5b4f7e283d85c093e63c235dc753397d 
>   src/python/scheduler/src/mesos/scheduler/mesos_scheduler_driver_impl.cpp 
> 78dc298ec19a61cd491b2b43b463db67528c5526 
>   src/scaling/scaling_sched.py d1008c79b1f1302d9ff6ae6dc9e1d9d0b7379773 
>   src/sched/sched.cpp 5f6f5518f0858c680dc0dffc933c0bb03bba6991 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 4f3f210b8d0ab9a453ab56c5e23024e2ab7c4259 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
>   src/slave/http.cpp 922aaad6e83ca9d5ab503ddb733a332982843300 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.cpp 49fa4a06e26d0d4475ed50db254a320a7030f896 
>   src/slave/state.cpp e7b44c78500e07f0f1655f41a6a977adae2ca0c0 
>   src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 7accd32fba5eed196a82b1a171cb16d37b9e0539 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 51e60c2e8c6c8b76b51de0e7761ecbb0ca9c3304 
>   src/tests/flags.hpp af15360491b5433dcfbda03a72407667eb3977d1 
>   src/tests/hierarchical_allocator_tests.cpp 
> 8ed0df45fc745d338482d1944a346cd40c18bb37 
>   src/tests/master_allocator_tests.cpp 
> 17607df7d488c8dd42c57504a5ca326697f57ffa 
>   src/tests/master_tests.cpp 1ae72600e26c5f31476f689cbb600d41d7cc9b74 
>   src/tests/paths_tests.cpp 81498e368cbb77e2cb8af71d8570dc690d3d0dfc 
>   src/tests/slave_recovery_tests.cpp 79132344be3bcd2bda54357cd5e7e0c59a766fd8 
>   src/tests/sorter_tests.cpp 0f3266f1222163c4d03eb4c4ca88f96836de601e 
> 
> Diff: https://reviews.apache.org/r/46189/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 46014: Stout:[1/2] Moved process tests to their own file.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/libprocess/3rdparty/stout/tests/os/process_tests.cpp (lines 43 - 61)


Do we have plans to implement this in stout?



3rdparty/libprocess/3rdparty/stout/tests/os/process_tests.cpp (line 73)


Can we add a comment here about this similar to below?
```
// NOTE: `getsid` does not have a meaningful interpretation on Windows.
```



3rdparty/libprocess/3rdparty/stout/tests/os/process_tests.cpp (line 121)


Can we add a comment here about this similar to below?
```
// NOTE: `getsid` does not have a meaningful interpretation on Windows.
```


- Michael Park


On April 14, 2016, 2:06 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46014/
> ---
> 
> (Updated April 14, 2016, 2:06 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout:[1/2] Moved process tests to their own file.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 400c6dc451602926f93b22713af8c66d7ca59ca6 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> c9d331df2f4496183b5734d2434413f68b9c1b4b 
>   3rdparty/libprocess/3rdparty/stout/tests/os/process_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> 9bd34c7508cd813c5de18028956f6a740997c266 
> 
> Diff: https://reviews.apache.org/r/46014/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Michael Park]

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




3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 155)


Remove newline.



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 161)


`s/foreach(/foreach (/`



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (lines 173 - 183)


(1) It seems like this should live in `posix/os.hpp`.
(2) The declaration of this function in `windows/os.hpp` is:
```
inline Result process(pid_t pid);
```
It seems like the windows version needs all 3 states whereas the POSIX 
version only needs 2?
If that is the case, I think it makes sense to consolidate to using 
`Result` in both cases and
leave a comment on the POSIX version that we have the 3 states because of 
Windows.



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 200)


`s/foreach(/foreach (/`



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 202)


Indent 2 more spaces.



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 214)


`s/> >/>>/`



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp (line 217)


Remove newline.


- Michael Park


On April 14, 2016, 8:57 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46013/
> ---
> 
> (Updated April 14, 2016, 8:57 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4471
> https://issues.apache.org/jira/browse/MESOS-4471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Implemented `os::processes` on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> edaa76a5322d0bf60b7172405aa754b5aca95458 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> c48106e5905e3be0faeba7177ef534766089faff 
> 
> Diff: https://reviews.apache.org/r/46013/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 45217: Implemented docker volume driver isolator interface.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 14, 2016, 12:32 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45217/
> ---
> 
> (Updated April 14, 2016, 12:32 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented docker volume driver isolator interface.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45217/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45377: Updated prepare() logic for unified container.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45214, 45217, 45270, 45360, 45373, 45326, 46180, 45370, 
44454, 45377]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 14, 2016, 4:30 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45377/
> ---
> 
> (Updated April 14, 2016, 4:30 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated prepare() logic for unified container.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> PRE-CREATION 
>   src/slave/slave.cpp de99e9eb5cc812b2e07deb749b98b4f4db363728 
> 
> Diff: https://reviews.apache.org/r/45377/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45987: Fixed `rmdir.hpp` Windows build breaks.

[Alex Clemmer]

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

(Updated April 14, 2016, 8:38 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Repository: mesos


Description
---

Fixed `rmdir.hpp` Windows build breaks.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp 
dd5cc6923ecc503d58e56128cf4ae736d8145fd7 
  3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
cc92953ccc53f37c54b87e738b16ea1fb521b987 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46220: Added documentation for Nvidia GPU support.

[Kevin Klues]

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

(Updated April 14, 2016, 8:33 p.m.)


Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.


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


Repository: mesos


Description
---

Added documentation for Nvidia GPU support.


Diffs
-

  docs/gpu-support.md PRE-CREATION 
  docs/home.md dc41fc4479e6c23650cd8ac78dcc4b9161d00721 

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


Testing
---

Ran a local copy of the documentation site to make sure everything looks OK.


Thanks,

Kevin Klues

Re: Review Request 40266: Libprocess Reinit: Cleanup SocketManager alongside ProcessManager.

[Joseph Wu]

> On Dec. 2, 2015, 8:08 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/process.cpp, line 993
> > 
> >
> > Once you use a proper smart pointer for `process_route` this comment 
> > will be right (maybe `s/deleted/cleaned up/`).

Note: This change was moved into a prior commit in the chain.


> On Dec. 2, 2015, 8:08 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/process.cpp, line 493
> > 
> >
> > You should be more explicit about lifetimes here and use `unique_ptrs` 
> > of .. instead (you can always `reset` in place of `delete` if you need to 
> > destroy at a certain point).

Note: This change was moved into a prior commit in the chain.


- Joseph


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


On April 14, 2016, 1:28 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40266/
> ---
> 
> (Updated April 14, 2016, 1:28 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3910
> https://issues.apache.org/jira/browse/MESOS-3910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `SocketManager` and `ProcessManager` are highly inter-dependent, 
> which requires some untangling in `process::finalize`.
> 
> * Logic originally found in `~ProcessManager` has been split into 
>   `ProcessManager::finalize` due to what happens during cleanup.
> * The future from `__s__->accept()` must be explicitly discarded as 
>   libevent does not detect a locally closed socket.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> afeddec20495bb9621c3e26b0d425c9419654739 
> 
> Diff: https://reviews.apache.org/r/40266/diff/
> 
> 
> Testing
> ---
> 
> `make check` (libev)
> `make check` (--enable-libevent --enable-ssl)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 46220: Added documentation for Nvidia GPU support.

[Kevin Klues]

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

Review request for mesos.


Repository: mesos


Description
---

Added documentation for Nvidia GPU support.


Diffs
-

  docs/gpu-support.md PRE-CREATION 
  docs/home.md dc41fc4479e6c23650cd8ac78dcc4b9161d00721 

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


Testing
---

Ran a local copy of the documentation site to make sure everything looks OK.


Thanks,

Kevin Klues

Re: Review Request 40410: Libprocess Reinit: Move MetricsProcess instantiation into process.cpp.

[Joseph Wu]

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

(Updated April 14, 2016, 1:28 p.m.)


Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.


Changes
---

Reviving this chain...


Summary (updated)
-

Libprocess Reinit: Move MetricsProcess instantiation into process.cpp.


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


Repository: mesos


Description (updated)
---

The metrics singleton must be unified with `process::initialize` so 
that it also falls under the scope of reinitialization.  The singleton 
must also not be guarded by `Once`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/metrics/metrics.hpp 
7abfadf6d030c52b7e03e773b67c74db6ad5b5df 
  3rdparty/libprocess/src/metrics/metrics.cpp 
7677dffed98153bc6276aad492a6815e67740ea3 
  3rdparty/libprocess/src/process.cpp afeddec20495bb9621c3e26b0d425c9419654739 
  3rdparty/libprocess/src/tests/system_tests.cpp 
0f4d0424689522337806ba2227ec4330c700e17e 

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


Testing
---

Tests done in a subsequent review.


Thanks,

Joseph Wu

Re: Review Request 40512: Libprocess Reinit: Add a test-only method to reinitialize libprocess.

[Joseph Wu]

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

(Updated April 14, 2016, 1:28 p.m.)


Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.


Changes
---

Reviving this chain...


Summary (updated)
-

Libprocess Reinit: Add a test-only method to reinitialize libprocess.


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


Repository: mesos


Description (updated)
---

This builds upon earlier changes to complete `process::finalize`.  
Tests which need to reconfigure libprocess, such as some SSL-related 
tests, can use `process::reinitialize` to do so.  

This method may also be useful for providing additional isolation 
between tests, such as cleaning up all processes after each test case.


Diffs (updated)
-

  3rdparty/libprocess/src/openssl.cpp 523b9740a45bdae3fa9a1a1423108e6815fbf872 
  3rdparty/libprocess/src/process.cpp afeddec20495bb9621c3e26b0d425c9419654739 

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


Testing
---

Defined `process::reinitialize` in several tests, such as `HTTPTest`, 
`MetricsTest`, and `ReapTest` can called `process::reinitialize` before and 
after tests.  Confirmed that this did not have side effects on other tests (See 
https://reviews.apache.org/r/40453/ ).


Thanks,

Joseph Wu

Re: Review Request 40413: Libprocess Reinit: Move ReaperProcess instantiation into process.cpp.

[Joseph Wu]

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

(Updated April 14, 2016, 1:28 p.m.)


Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.


Changes
---

Reviving this chain...


Summary (updated)
-

Libprocess Reinit: Move ReaperProcess instantiation into process.cpp.


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


Repository: mesos


Description (updated)
---

The reaper singleton must be unified with `process::initialize` so 
that it also falls under the scope of reinitialization.  The singleton 
must also not be guarded by `Once`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/reap.hpp 
1a9709c618c5ddc9d2b7492cc1855a11f1fc4fb9 
  3rdparty/libprocess/src/process.cpp afeddec20495bb9621c3e26b0d425c9419654739 
  3rdparty/libprocess/src/reap.cpp 110386842d5eeccfbc4e48bfca88d79a9d087fb0 

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


Testing
---

Tests done in a subsequent review.


Thanks,

Joseph Wu

Re: Review Request 40411: Libprocess Reinit: Modify test to use PID.

[Joseph Wu]

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

(Updated April 14, 2016, 1:28 p.m.)


Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.


Changes
---

Reviving this chain...


Summary (updated)
-

Libprocess Reinit: Modify test to use PID.


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


Repository: mesos


Description (updated)
---

Updates the test's reference to the global metrics singleton.


Diffs (updated)
-

  src/tests/scheduler_driver_tests.cpp 217185780e3576faf633dd9629ae93a275fac284 

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


Testing
---

Tests done in a subsequent review.


Thanks,

Joseph Wu

Re: Review Request 40268: Libprocess Reinit: Change Socket::DEFAULT_KIND to a non-static value.

[Joseph Wu]

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

(Updated April 14, 2016, 1:28 p.m.)


Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.


Changes
---

Reviving this chain...


Summary (updated)
-

Libprocess Reinit: Change Socket::DEFAULT_KIND to a non-static value.


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


Repository: mesos


Description (updated)
---

This is needed for tests that utilize the test-only 
`process::reinitialize` function.


Diffs (updated)
-

  3rdparty/libprocess/include/process/socket.hpp 
4cb49680d1304899a4ee675ac07379e51d9c55b1 
  3rdparty/libprocess/src/socket.cpp ec0e913aca30f92d4a0543ad1e685b897617bca3 

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


Testing
---

Some test cleanup issues were uncovered by this change; fixed here: 
https://reviews.apache.org/r/40453/


Thanks,

Joseph Wu

Re: Review Request 40266: Libprocess Reinit: Cleanup SocketManager alongside ProcessManager.

[Joseph Wu]

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

(Updated April 14, 2016, 1:28 p.m.)


Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.


Changes
---

Reviving this chain.  Rebased on Neil's memory leak fixes.


Summary (updated)
-

Libprocess Reinit: Cleanup SocketManager alongside ProcessManager.


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


Repository: mesos


Description (updated)
---

The `SocketManager` and `ProcessManager` are highly inter-dependent, 
which requires some untangling in `process::finalize`.

* Logic originally found in `~ProcessManager` has been split into 
  `ProcessManager::finalize` due to what happens during cleanup.
* The future from `__s__->accept()` must be explicitly discarded as 
  libevent does not detect a locally closed socket.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp afeddec20495bb9621c3e26b0d425c9419654739 

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


Testing
---

`make check` (libev)
`make check` (--enable-libevent --enable-ssl)


Thanks,

Joseph Wu

Re: Review Request 45270: Added spec protobut for external mount.

[Greg Mann]

> On April 14, 2016, 8:17 p.m., Greg Mann wrote:
> >

Could you fix the typo in the commit message: "protobut" should be "protobuf". 
Also, could you remove the duplicate Summary that's in the Description field, 
and add a more verbose description of the patch in the Description field?


- Greg


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


On April 14, 2016, 4:41 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45270/
> ---
> 
> (Updated April 14, 2016, 4:41 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added spec protobut for external mount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.proto 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45270/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45270: Added spec protobut for external mount.

[Greg Mann]

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




src/slave/containerizer/mesos/isolators/docker/volume/spec.proto (line 30)


s/and not configurable/and is not configurable/


- Greg Mann


On April 14, 2016, 4:41 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45270/
> ---
> 
> (Updated April 14, 2016, 4:41 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added spec protobut for external mount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.proto 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45270/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45670: Updated tests for HTTP command executor.

[Anand Mazumdar]

> On April 12, 2016, 9:11 p.m., Vinod Kone wrote:
> > Are you also planning to update slave recovery tests? Those are the most 
> > crucial.
> 
> Qian Zhang wrote:
> Sure, I will update slave recovery tests soon. Just want to double 
> confirm, in `slave_recovery_tests.cpp`, I see there are two TODOs related to 
> HTTP based command executor in two test cases:
> 
> https://github.com/apache/mesos/blob/0.28.0/src/tests/slave_recovery_tests.cpp#L462
> 
> https://github.com/apache/mesos/blob/0.28.0/src/tests/slave_recovery_tests.cpp#L1082
> 
> You mean I need to update these two test cases with the HTTP command 
> executor I implemented in MESOS-3558, right?
> 
> Vinod Kone wrote:
> Definitely those two and the one we talked about in the earlier review.
> 
> It would also be good to add the following tests for HTTP command 
> executor. There are already tests for this for driver based command executor.
> 
> --> RecoverUnregisteredHTTPExecutor 
> --> RecoverTerminatedExecutor
> --> KillTask
> 
> Qian Zhang wrote:
> OK, I have posted a patch (https://reviews.apache.org/r/46204/) to update 
> those two tests and add two more (`RecoverUnregisteredHTTPExecutor` and 
> `KillTaskWithHTTPExecutor`) in slave recovery tests, and I will post a 
> separate patch for adding the test that slave restarts with flag change.
> 
> But for the test `RecoverTerminatedExecutor`, I have a question: we need 
> to shutdown the executor when agent is down, for the driver based executor, I 
> see we use `process::post(executorPid, ShutdownExecutorMessage())`, but for 
> HTTP based executor, I am not sure how to shutdown it in the test, any 
> suggestions? Thanks.

We currently don't have the ability to insert `Event`'s on the response stream. 
I filed https://issues.apache.org/jira/browse/MESOS-5220 to deal with it.

As Vinod suggested: For now, you would need to manually kill the `forkedPid` 
similar to what is being done here: 
https://github.com/apache/mesos/blob/master/src/tests/slave_recovery_tests.cpp#L2484

Can you also add a comment adding a `TODO` linking to MESOS-5220 once you do 
this change so that this is tracked?


- Anand


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


On April 13, 2016, 9:15 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45670/
> ---
> 
> (Updated April 13, 2016, 9:15 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated tests for HTTP command executor.
> 
> 
> Diffs
> -
> 
>   src/tests/command_executor_tests.cpp 
> 843233adaa68ab1f5cedb7b075439bb8b77469a8 
> 
> Diff: https://reviews.apache.org/r/45670/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 46008: Stout: Initialize Windows socket stack in Stout tests.

[Alex Clemmer]

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

(Updated April 14, 2016, 7:24 p.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Repository: mesos


Description
---

Stout: Initialize Windows socket stack in Stout tests.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/socket.hpp 
b5aeab75c2365a3431c397743b95ce7fbd6d7a1a 
  3rdparty/libprocess/3rdparty/stout/tests/main.cpp 
c449de87bede4d3bf1df368eaf1d22f857c2298f 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46214: Valided role when framework subscribe.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46214]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 14, 2016, 4:11 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46214/
> ---
> 
> (Updated April 14, 2016, 4:11 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5184
> https://issues.apache.org/jira/browse/MESOS-5184
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Valided role when framework subscribe.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
>   src/tests/master_tests.cpp 1ae72600e26c5f31476f689cbb600d41d7cc9b74 
>   src/tests/scheduler_http_api_tests.cpp 
> d469adf7230ce5bb5e71a241a06e389295905e03 
> 
> Diff: https://reviews.apache.org/r/46214/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 45996: Fixed memory leak of `gc` in `finalize()` in libprocess.

[Joseph Wu]

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


Fix it, then Ship it!





3rdparty/libprocess/src/process.cpp (lines 489 - 490)


This is now effectively managed by the `ProcessManager`, so you could bring 
it into the `ProcessManager` 's scope (which would still be global).


- Joseph Wu


On April 10, 2016, 6:41 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45996/
> ---
> 
> (Updated April 10, 2016, 6:41 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5144
> https://issues.apache.org/jira/browse/MESOS-5144
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed memory leak of `gc` in `finalize()` in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/gc.hpp 
> 799468ebe49f2a49d325f40ffd8acea727abf74c 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/45996/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 45995: Fixed memory leak of `Route` in `finalize()` in libprocess.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On April 10, 2016, 6:39 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45995/
> ---
> 
> (Updated April 10, 2016, 6:39 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5144
> https://issues.apache.org/jira/browse/MESOS-5144
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed memory leak of `Route` in `finalize()` in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/45995/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 45991: Fixed memory leak in Route::Route() in libprocess.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On April 10, 2016, 6:38 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45991/
> ---
> 
> (Updated April 10, 2016, 6:38 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5144
> https://issues.apache.org/jira/browse/MESOS-5144
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed memory leak in Route::Route() in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/45991/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 45370: Implemented prepare() for volume isolator.

[Gilbert Song]

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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 111)


Shrink the first space to above line.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 124 - 
125)


We need a `if` to check here, otherwise it can cause segfault if 
`driver_options` is none.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 127 - 
134)


How about moving `.setContainerId(stringify(containerId))` next line?



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 164 - 
165)


Save the raw cmd as a variable and pass this cmd to VLOG(1).


- Gilbert Song


On April 13, 2016, 11:29 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> ---
> 
> (Updated April 13, 2016, 11:29 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() for volume isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45370/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.

[Joris Van Remoortere]

> On April 12, 2016, 3:09 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp, lines 26-27
> > 
> >
> > Please use plain names that are self descriptive.
> 
> Alex Clemmer wrote:
> I'm not sure whether you're talking about the symbol `key` or the string 
> that is its value so I made the first more descriptive and the second more 
> boring. Christmas comes early to the Van Remoortere household!

How about:
```
string key = "key";
string value = "value";
string new_value = "new_value";
```


- Joris


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


On April 13, 2016, 10:05 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46009/
> ---
> 
> (Updated April 13, 2016, 10:05 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout:[1/2] Added simple tests for `os::` functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 400c6dc451602926f93b22713af8c66d7ca59ca6 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> c9d331df2f4496183b5734d2434413f68b9c1b4b 
>   3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp 
> d5a96ad6b8b1c09b4f016e0c8e3e5c5672b55ef3 
> 
> Diff: https://reviews.apache.org/r/46009/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.

[Joris Van Remoortere]

> On April 12, 2016, 3:09 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp, lines 22-23
> > 
> >
> > This seems generally useful?
> > I think I've even seen it in other reviews?
> > Why is it burried in a test?
> 
> Alex Clemmer wrote:
> We are doing this to stay consistent with Neil Conway's pass over the 
> tests to use `std::string`. For the general codebase, maybe it's useful, and 
> maybe it's not, I'm not sure.

Sorry I must have messed up the selection highlight.
I meant this: `const string errorMessage(int err)`.


- Joris


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


On April 13, 2016, 10:05 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46009/
> ---
> 
> (Updated April 13, 2016, 10:05 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout:[1/2] Added simple tests for `os::` functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 400c6dc451602926f93b22713af8c66d7ca59ca6 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> c9d331df2f4496183b5734d2434413f68b9c1b4b 
>   3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp 
> d5a96ad6b8b1c09b4f016e0c8e3e5c5672b55ef3 
> 
> Diff: https://reviews.apache.org/r/46009/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 45987: Fixed `rmdir.hpp` Windows build breaks.

[Joris Van Remoortere]

> On April 12, 2016, 2:41 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp, line 179
> > 
> >
> > Can you explain why this is not defined on windows?
> 
> Alex Clemmer wrote:
> Well, I don't know how to port it to Windows. It is not clear to me that 
> `rdev` or `mknod` make much sense to Windows. You might be able to make a 
> totally different test that makes this work, but after spending an hour or 
> two on it I decided that it is more important to make progress on other 
> issues instead.
> 
> Thoughts? Do you think it is important to port it right now?

I meant can you add a comment explaining exactly what you just said in this 
review ;-)
Feel free to make it concise.


- Joris


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


On April 14, 2016, 1:37 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45987/
> ---
> 
> (Updated April 14, 2016, 1:37 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed `rmdir.hpp` Windows build breaks.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/rmdir.hpp 
> dd5cc6923ecc503d58e56128cf4ae736d8145fd7 
>   3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
> cc92953ccc53f37c54b87e738b16ea1fb521b987 
> 
> Diff: https://reviews.apache.org/r/45987/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46180: Implemented create() for docker volume isolator.

[Gilbert Song]

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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 72 - 
73)


How about moving "' used by volume isolator" to the second line?


- Gilbert Song


On April 14, 2016, 6:40 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46180/
> ---
> 
> (Updated April 14, 2016, 6:40 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented create() for docker volume isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46180/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 46008: Stout: Initialize Windows socket stack in Stout tests.

[Joris Van Remoortere]

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


Fix it, then Ship it!





3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/socket.hpp (line 60)


brace on new line



3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/socket.hpp (lines 
79 - 80)


style



3rdparty/libprocess/3rdparty/stout/tests/main.cpp (line 34)


Can we use the `EXIT(EXIT_FAILURE) << error_msg;` pattern?



3rdparty/libprocess/3rdparty/stout/tests/main.cpp (line 39)


Can we call this `teardown_error` so that in the future others won't 
introduce their own bools?



3rdparty/libprocess/3rdparty/stout/tests/main.cpp (lines 46 - 49)


I think this comment makes more sense when we introduce the bool 
`teardown_error`. Otherwise one will ask the question "why don't we just `EXIT` 
if `wsa_cleanup` fails"?


- Joris Van Remoortere


On April 14, 2016, 5:21 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46008/
> ---
> 
> (Updated April 14, 2016, 5:21 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Initialize Windows socket stack in Stout tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/socket.hpp 
> b5aeab75c2365a3431c397743b95ce7fbd6d7a1a 
>   3rdparty/libprocess/3rdparty/stout/tests/main.cpp 
> c449de87bede4d3bf1df368eaf1d22f857c2298f 
> 
> Diff: https://reviews.apache.org/r/46008/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 45326: Added agent flag dvdcli_dir for specify dvdcli path.

[Gilbert Song]

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




src/slave/flags.cpp (line 803)


Just leave a concern here:

Do we always have dvdcli installed in `/usr/bin` for all os (e.g., centos)? 
any special case?


- Gilbert Song


On April 13, 2016, 11:06 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45326/
> ---
> 
> (Updated April 13, 2016, 11:06 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5084
> https://issues.apache.org/jira/browse/MESOS-5084
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent flag dvdcli_dir for specify dvdcli path.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
> 
> Diff: https://reviews.apache.org/r/45326/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45373: Ignored the DOCKER_VOLUME volume source.

[Gilbert Song]

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




src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 471 - 472)


let quote the containerId:

```
  VLOG(1) << "Ignored the 'DOCKER_VOLUME' volume source for container '"
  << containerId << "'";
```


- Gilbert Song


On April 13, 2016, 2:26 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45373/
> ---
> 
> (Updated April 13, 2016, 2:26 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignored the DOCKER_VOLUME volume source.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 9fc7c48f99155750fd3c18c7c102507e2726362b 
> 
> Diff: https://reviews.apache.org/r/45373/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45360: Added volume client for mount and unmount.

[Gilbert Song]

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




src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 
57)


How about using:

`const stirng&`



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 
58)


ditto.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (lines 
64 - 81)


It is still the issue I mentioned above:

Since we have to specify `--volumedriver` and `--volumename`, and possibly 
have a bunch of `--volumeopt` appended. I would suggest collecting all this 
arguments (e.g., {dvdcliPath, "mount", "--volumedriver=", ...}) into a 
`vector argv;`.

and in subprocess, if you grep the examples `Try`, your can see 
for most cases we have cmd and argv separated. Considering the cmd may not be 
short. Let's make it more clear:

```
  Try s = subprocess(
  dvdcliPath,
  argv,
  Subprocess::PATH("/dev/null"),
  Subprocess::PIPE(),
  Subprocess::PIPE());
```



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 
87)


Let's following the pattern in CNI isolator by using await here and passing 
the `tuple` to `launch()` instead of passing a subprocess.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 
104)


ditto.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (lines 
128 - 150)


Understand that you want to simplify the logic for mount and unmount here. 
Let us discuss this comparing the logic in CNI isolator `attach()` and 
`detach()` V.S. passing the reference of subprocess to launch.


- Gilbert Song


On April 13, 2016, 11:05 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated April 13, 2016, 11:05 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added volume client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f 
>   src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 46164: Moved mesos::internal::state to mesos::state namespace.

[Jie Yu]

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




include/mesos/state/state.hpp (line 82)


Let's void mesos:: if possible. Ditto for others.


- Jie Yu


On April 13, 2016, 11:49 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46164/
> ---
> 
> (Updated April 13, 2016, 11:49 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5171
> https://issues.apache.org/jira/browse/MESOS-5171
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved mesos::internal::state to mesos::state namespace.
> 
> 
> Diffs
> -
> 
>   include/mesos/state/state.hpp ecae61fa4fdfb30928c1ccb4e85aa6ebe5addbcd 
>   include/mesos/state/storage.hpp 5de3b174cb80924c6b0dc4cab4569674f70c689a 
>   src/java/jni/org_apache_mesos_state_AbstractState.cpp 
> 4fd43ca31c6917e81ea1b331b8507ca42a2249cf 
>   src/java/jni/org_apache_mesos_state_LevelDBState.cpp 
> fb3c5418220f66c6320581033ee7963de47eafd3 
>   src/java/jni/org_apache_mesos_state_LogState.cpp 
> 528fe5367f55c63916cd4990abcc3c137e77cfec 
>   src/java/jni/org_apache_mesos_state_ZooKeeperState.cpp 
> 23a89c3e5c5309a8a9b45c168fb61b12d89db1ec 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/main.cpp ea7f0fc87c8912309a4679105dde5d8d5bb9ead6 
>   src/state/in_memory.hpp 28ad76134f1cfe64d916679b4f6409d6a5fdd532 
>   src/state/leveldb.hpp 626ed82d84e15f65bc6187f9183cc376b705e1a3 
>   src/state/log.hpp 5dd30ca180380121639f23a931486e97b660c0d2 
>   src/state/protobuf.hpp 7ac5bca1c1a0b92549fc6d7e692e8bdb920b8a0e 
>   src/state/zookeeper.hpp d8f9df8d1f5575e7dcbfaf27e92656baf3b4f0ff 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/registrar_tests.cpp f18e8030f69d8ebf8de81ff03632106e08823df1 
>   src/tests/state_tests.cpp 0b5a6abadc24d16aa0e5e5f2f4b0c9f524c399ec 
> 
> Diff: https://reviews.apache.org/r/46164/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 46163: Exposed state/{state,storage}.hpp files.

[Jie Yu]

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




src/Makefile.am (line 174)


I think the mesos.internal.state.Entry in state.proto needs to be expose. 
However, mesos.internal.state.Operation is impl. details for log storage, and 
should not be exposed.



src/tests/state_tests.cpp (lines 49 - 52)


I think we should expose them as well. We definitely need 
state/protobuf.hpp and state/log.hpp. But it doesn't make sense to just expose 
log storage and leave the rest in private.


- Jie Yu


On April 13, 2016, 11:49 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46163/
> ---
> 
> (Updated April 13, 2016, 11:49 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5171
> https://issues.apache.org/jira/browse/MESOS-5171
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Exposed state/{state,storage}.hpp files.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   src/java/jni/org_apache_mesos_state_AbstractState.cpp 
> 4fd43ca31c6917e81ea1b331b8507ca42a2249cf 
>   src/java/jni/org_apache_mesos_state_LevelDBState.cpp 
> fb3c5418220f66c6320581033ee7963de47eafd3 
>   src/java/jni/org_apache_mesos_state_LogState.cpp 
> 528fe5367f55c63916cd4990abcc3c137e77cfec 
>   src/java/jni/org_apache_mesos_state_Variable.cpp 
> 7a69b3d878e6eaa87bfaf56c0500e11efe4ac44d 
>   src/java/jni/org_apache_mesos_state_ZooKeeperState.cpp 
> 23a89c3e5c5309a8a9b45c168fb61b12d89db1ec 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/main.cpp ea7f0fc87c8912309a4679105dde5d8d5bb9ead6 
>   src/messages/state.proto  
>   src/state/in_memory.hpp 28ad76134f1cfe64d916679b4f6409d6a5fdd532 
>   src/state/in_memory.cpp ac4f34bd2332c8b42a26454809d56c225aa44587 
>   src/state/leveldb.hpp 626ed82d84e15f65bc6187f9183cc376b705e1a3 
>   src/state/leveldb.cpp f925a73856075f08c4af2af0fcaae8e81358667e 
>   src/state/log.hpp 5dd30ca180380121639f23a931486e97b660c0d2 
>   src/state/protobuf.hpp 7ac5bca1c1a0b92549fc6d7e692e8bdb920b8a0e 
>   src/state/state.hpp ecae61fa4fdfb30928c1ccb4e85aa6ebe5addbcd 
>   src/state/storage.hpp 5de3b174cb80924c6b0dc4cab4569674f70c689a 
>   src/state/zookeeper.hpp d8f9df8d1f5575e7dcbfaf27e92656baf3b4f0ff 
>   src/state/zookeeper.cpp 5578fa5f0b86f8dfeabf1be2984bfa0443cc049c 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
>   src/tests/registrar_tests.cpp f18e8030f69d8ebf8de81ff03632106e08823df1 
>   src/tests/state_tests.cpp 0b5a6abadc24d16aa0e5e5f2f4b0c9f524c399ec 
> 
> Diff: https://reviews.apache.org/r/46163/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>