Re: Review Request 49414: Added Executor PID in /containers endpoint. Also Added Test Cases.

[Jie Yu]

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




src/slave/containerizer/mesos/launcher.hpp (line 78)


Add a new line above.



src/slave/containerizer/mesos/launcher.hpp (line 81)


indentation for parameters should be 4



src/slave/containerizer/mesos/launcher.hpp (line 115)


Ditto.



src/slave/containerizer/mesos/linux_launcher.hpp (line 55)


Ditto.



src/slave/containerizer/mesos/linux_launcher.cpp (line 367)


2 lines apart between top level functions.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 458)


Please move this right above `TEST_F(MesosLauncherStatusTest ...`

And please move all the stuff you just added to the end of this file.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 566)


2 lines apart please.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 580)


Please use UUID::random here.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 582)


Ditto on using UUID::random



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 585 - 593)


INdentation here should be 4.



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 591 - 593)


NO need for those default variables.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 599)


Please use AWAIT_READY(validStatus) here



src/tests/containerizer/mesos_containerizer_tests.cpp (line 601)


you can use `validStatus->executor_id`



src/tests/containerizer/mesos_containerizer_tests.cpp (line 603)


No need for back slash in the end



src/tests/containerizer/mesos_containerizer_tests.cpp (line 604)


Indentation here should be 2. See style guide



src/tests/containerizer/mesos_containerizer_tests.cpp (line 610)


2 lines apart, please.


- Jie Yu


On June 30, 2016, 12:51 a.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49414/
> ---
> 
> (Updated June 30, 2016, 12:51 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5737
> https://issues.apache.org/jira/browse/MESOS-5737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Executor PID in /containers endpoint. Also Added Test Cases.
> 
> In order to greatly simplify the implementation for the Mesos CLI's container 
> plugin, we need the executor PID (Process ID) to be exposed in the 
> /containers endpoint. [Mesos CLI 
> Epic](https://issues.apache.org/jira/browse/MESOS-5676)
> This change will introduce the pid for an executor if it was launched by the 
> mesos containerizer in the /containers endpoint of an agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
>   include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a96b382f22886362a1159e1166dfe041072985ba 
>   src/slave/containerizer/mesos/launcher.hpp 
> 05320f462653c31fc2f093d6c67e2182e9c794fa 
>   src/slave/containerizer/mesos/launcher.cpp 
> ff675262af8947b89f8099828665e5e5d86491d8 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> 89bb2958a41dffe4ade9c2492b9a7412f90a432d 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 5028854fa003615f158120e030866b7ec4402b66 
>   src/tests/containerizer/launcher.hpp 
> c352634c4766d289706c7cc738677619d7d02ccd 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 6c14f6e20b4d14d5ed095670673571739101b0e4 
> 
> Diff: https://reviews.apache.org/r/49414/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>

Re: Review Request 49549: Used the launch helper binary to launch user task in command executor.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [49549, 49548, 49542, 49541, 49540, 49523, 49524, 49479, 
49473, 49472, 49425, 49424, 49415]

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

Error:
2016-07-02 05:37:57 URL:https://reviews.apache.org/r/49541/diff/raw/ 
[1789/1789] -> "49541.patch" [1]
error: patch failed: src/launcher/executor.cpp:676
error: src/launcher/executor.cpp: patch does not apply

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

- Mesos ReviewBot


On July 2, 2016, 1:02 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49549/
> ---
> 
> (Updated July 2, 2016, 1:02 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Ian Downes, and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5753
> https://issues.apache.org/jira/browse/MESOS-5753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, command executor and 'mesos-containerizer launch' share a
> lot of the logic. Command executor should in fact, just use
> `mesos-containerizer launch` to launch the user task. Potentially,
> 'mesos-containerizer launch' can be also used by custom executor to
> launch user tasks.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 63010252ffd6b1dfbe3358a1f1f4447967b824d2 
>   src/launcher/posix/executor.hpp e5f3c0a8e2d3687a5330adbf2f667db9106970c7 
>   src/launcher/posix/executor.cpp ab1dd938a7c59f52b44aafe1340c614087460f84 
>   src/slave/containerizer/mesos/launch.cpp 
> 83f4d7f28c066a605aa84862eca9fde900ec96c6 
> 
> Diff: https://reviews.apache.org/r/49549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49518: Initial snapshot for v1 master event stream.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49464, 49465, 49487, 49488, 49489, 49509, 49516, 49517, 49518]

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

- Mesos ReviewBot


On July 2, 2016, 12:20 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49518/
> ---
> 
> (Updated July 2, 2016, 12:20 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5498
> https://issues.apache.org/jira/browse/MESOS-5498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initial snapshot for v1 master event stream.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto d06258e9fd39c7eefd8ecd394e3bdfb888479b1e 
>   include/mesos/v1/master/master.proto 
> b7cb6fdf2e4f34a11d326ac3ad3ec26525f8f343 
>   src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
>   src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 
> 
> Diff: https://reviews.apache.org/r/49518/diff/
> 
> 
> Testing
> ---
> 
> Updated test and make check.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49544: Made stout::Path default constructible / assignable / copyable.

[Guangya Liu]

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



@Ben, someone will faield to apply this patch as the commit message summary 
exceeds 72 characters.

- Guangya Liu


On 七月 2, 2016, 12:46 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49544/
> ---
> 
> (Updated 七月 2, 2016, 12:46 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This makes Path closer to boost's path, and more convenient to work with 
> (e.g. as map values, struct members, etc).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 0ca13c6a1aebcb980c29dd33414bfa50c4b8bf81 
> 
> Diff: https://reviews.apache.org/r/49544/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 49543: Fix ProcessRemoteLinkTests that try to emulate 'stale' sockets.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49543]

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

- Mesos ReviewBot


On July 1, 2016, 11:53 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49543/
> ---
> 
> (Updated July 1, 2016, 11:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-5759
> https://issues.apache.org/jira/browse/MESOS-5759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The ProcessRemoteLinkTests `RemoteUseStaleLink` and 
> `RemoteStaleLinkRelink` were failing because the `test-linkee` was
> closing sockets shortly after the test `::shutdown` the link.  This 
> is the expected behavior of libprocess.
> 
> Rewrites the `test-linkee` to keep all sockets open, regardless of the
> state of the connection.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> 7ba0f4833a5163d2323e1477509fd2fbacc23690 
> 
> Diff: https://reviews.apache.org/r/49543/diff/
> 
> 
> Testing
> ---
> 
> make check (CentOS 7)
> 
> 3rdparty/libprocess/libprocess-tests --gtest_filter="ProcessRemoteLinkTest*" 
> --gtest_break_on_failure --gtest_repeat=1
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 45967: Added documentation for shareable resources.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45958, 45959, 48616, 45960, 45961, 45962, 45963, 45964, 
45966, 45967]

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

- Mesos ReviewBot


On July 1, 2016, 10:29 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45967/
> ---
> 
> (Updated July 1, 2016, 10:29 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4325
> https://issues.apache.org/jira/browse/MESOS-4325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for shareable resources.
> 
> 
> Diffs
> -
> 
>   docs/home.md 2d728333a3c1421b310d861b71c92691fd41a482 
>   docs/shared-resources.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45967/diff/
> 
> 
> Testing
> ---
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 49501: Added cmake target to build mesos-master.

[Joseph Wu]

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


Ship it!




LGTM!  Tested this out locally.

A couple minor nits noted below, which I'll fix before committing.


src/CMakeLists.txt (line 260)


This file doesn't seem to exist anymore.  I'll remove it.



src/master/CMakeLists.txt (line 24)


This is already defined in 
`3rdparty/libprocess/cmake/Process3rdparyConfigure.cmake` as `MESOS_MASTER`.  
It is somewhat odd that mesos targets are defined in 3rdparty, but we can 
refactor that later.



src/CMakeLists.txt (line 259)


This is already present at line 241.



src/CMakeLists.txt (lines 271 - 273)


These are already present in lines 242-244.



src/master/CMakeLists.txt (line 37)


Tried this out on Windows, but looks like there's a bit more work to be 
done there.  I'll exclude the master from Win32 for now.


- Joseph Wu


On July 1, 2016, 5:37 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49501/
> ---
> 
> (Updated July 1, 2016, 5:37 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-3541
> https://issues.apache.org/jira/browse/MESOS-3541
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added cmake target to build mesos-master.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt c820dcd7d86de8e4896b96be6cc213be33de0d9b 
>   cmake/MesosConfigure.cmake 09135380e558ce8834eb70e804fbd90472334160 
>   src/CMakeLists.txt 996d9e655b9d4343c07663fe840e94727cd792fe 
>   src/master/CMakeLists.txt PRE-CREATION 
>   src/master/cmake/MasterConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49501/diff/
> 
> 
> Testing
> ---
> 
> make mesos-master
> mesos-master --help
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49547: Fixed dangerous use of Subprocess::PIPE as default Docker::run IO.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On July 1, 2016, 5:55 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49547/
> ---
> 
> (Updated July 1, 2016, 5:55 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker::run used Subprocess::PIPE as defaults for stdout and stderr,
> however the caller cannot obtain the read end of the pipe in order
> to read and avoid blocking the child. Also, internally these pipes
> are not read. This means the child process can block!
> 
> For now, this changes the default to match stdout and stderr from
> the parent process. Ideally, we also prevent the caller from using
> Subprocess::PIPE since they cannot obtain the read end.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 4343213391bb9a86e87410ce3a2c5d8447a1ab8d 
> 
> Diff: https://reviews.apache.org/r/49547/diff/
> 
> 
> Testing
> ---
> 
> `make check GTEST_FILTER="*Docker*"`
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 49549: Used the launch helper binary to launch user task in command executor.

[Jie Yu]

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

Review request for mesos, Gilbert Song, Ian Downes, and Joris Van Remoortere.


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


Repository: mesos


Description
---

Currently, command executor and 'mesos-containerizer launch' share a
lot of the logic. Command executor should in fact, just use
`mesos-containerizer launch` to launch the user task. Potentially,
'mesos-containerizer launch' can be also used by custom executor to
launch user tasks.


Diffs
-

  src/launcher/executor.cpp 63010252ffd6b1dfbe3358a1f1f4447967b824d2 
  src/launcher/posix/executor.hpp e5f3c0a8e2d3687a5330adbf2f667db9106970c7 
  src/launcher/posix/executor.cpp ab1dd938a7c59f52b44aafe1340c614087460f84 
  src/slave/containerizer/mesos/launch.cpp 
83f4d7f28c066a605aa84862eca9fde900ec96c6 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 49548: Added devolve function for CommandInfo.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


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


Repository: mesos


Description
---

Added devolve function for CommandInfo.


Diffs
-

  src/internal/devolve.hpp 7c242d3d9235235c954f232af84b52d62373afeb 
  src/internal/devolve.cpp 04fac996a0463bdb3218a90bcfdb68b79d08dd7b 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 49546: Added device support to Docker::run.

[Benjamin Mahler]

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

Review request for mesos and Kevin Klues.


Repository: mesos


Description
---

Added device support to Docker::run.


Diffs
-

  src/docker/docker.hpp 4343213391bb9a86e87410ce3a2c5d8447a1ab8d 
  src/docker/docker.cpp b356848ea8f65b58102b26b38e2e753c4b665a4b 
  src/docker/executor.cpp 2089b547408a46c9b4fa91e3ab17b88f7d2d8397 
  src/slave/containerizer/docker.cpp 915030bdbe5a5b55acc38042ee0475074a602ee0 
  src/tests/containerizer/docker_containerizer_tests.cpp 
f7b42836ceb80cdae901c4926bd702b6da36a55c 
  src/tests/containerizer/docker_tests.cpp 
c0ff9574633f8f72ccae9b9298f3b60bc2f3cf81 
  src/tests/mesos.hpp 3173bf9cfc832be0aa18d745434afc45d352a269 
  src/tests/mesos.cpp a433e5b2775f1d62b18927cf71af23ef28d6a503 

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


Testing
---

Added a test.


Thanks,

Benjamin Mahler

Review Request 49545: Updated code to use Path::string() instead of Path::value.

[Benjamin Mahler]

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

Review request for mesos and Kevin Klues.


Repository: mesos


Description
---

Updated code to use Path::string() instead of Path::value.


Diffs
-

  src/credentials/credentials.hpp 253ea8353ac67fde40268e35ea16f773f11eb509 
  src/linux/systemd.cpp 3e1ba935763383ba5d5bf9fad21a948a771101a0 
  src/slave/containerizer/fetcher.cpp 7b52cb1703e5b8a6ce0e995896fab3cd520a5090 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp 
b29fc79de7f1915778e43a7f4e72c2754bb64ab3 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
249acad49122d988e44744384bcf840b941c0997 
  src/tests/common/command_utils_tests.cpp 
c91a7f38263e4c8352ec56f79070baec2b1cb06f 
  src/tests/fetcher_cache_tests.cpp ba7d3f84ee54d982d7dfe0eeb9c67c1a68f9cf40 
  src/watcher/whitelist_watcher.cpp b252f2f1b35113996f72c2ff92bcd6a3728d02f9 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 49547: Fixed dangerous use of Subprocess::PIPE as default Docker::run IO.

[Benjamin Mahler]

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

Review request for mesos, Joseph Wu and Kevin Klues.


Repository: mesos


Description
---

Docker::run used Subprocess::PIPE as defaults for stdout and stderr,
however the caller cannot obtain the read end of the pipe in order
to read and avoid blocking the child. Also, internally these pipes
are not read. This means the child process can block!

For now, this changes the default to match stdout and stderr from
the parent process. Ideally, we also prevent the caller from using
Subprocess::PIPE since they cannot obtain the read end.


Diffs
-

  src/docker/docker.hpp 4343213391bb9a86e87410ce3a2c5d8447a1ab8d 

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


Testing
---

`make check GTEST_FILTER="*Docker*"`


Thanks,

Benjamin Mahler

Review Request 49544: Made stout::Path default constructible / assignable / copyable.

[Benjamin Mahler]

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

Review request for mesos and Kevin Klues.


Repository: mesos


Description
---

This makes Path closer to boost's path, and more convenient to work with (e.g. 
as map values, struct members, etc).


Diffs
-

  3rdparty/stout/include/stout/path.hpp 
0ca13c6a1aebcb980c29dd33414bfa50c4b8bf81 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 49501: Added cmake target to build mesos-master.

[Srinivas Brahmaroutu]

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

(Updated July 2, 2016, 12:37 a.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Added cmake target to build mesos-master.


Diffs (updated)
-

  CMakeLists.txt c820dcd7d86de8e4896b96be6cc213be33de0d9b 
  cmake/MesosConfigure.cmake 09135380e558ce8834eb70e804fbd90472334160 
  src/CMakeLists.txt 996d9e655b9d4343c07663fe840e94727cd792fe 
  src/master/CMakeLists.txt PRE-CREATION 
  src/master/cmake/MasterConfigure.cmake PRE-CREATION 

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


Testing
---

make mesos-master
mesos-master --help


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49518: Initial snapshot for v1 master event stream.

[Zhitao Li]

> On July 1, 2016, 7:41 p.m., Vinod Kone wrote:
> > src/master/http.cpp, line 595
> > 
> >
> > just capture `http`?

Also needed `ok`.


- Zhitao


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


On July 2, 2016, 12:20 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49518/
> ---
> 
> (Updated July 2, 2016, 12:20 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5498
> https://issues.apache.org/jira/browse/MESOS-5498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initial snapshot for v1 master event stream.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto d06258e9fd39c7eefd8ecd394e3bdfb888479b1e 
>   include/mesos/v1/master/master.proto 
> b7cb6fdf2e4f34a11d326ac3ad3ec26525f8f343 
>   src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
>   src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 
> 
> Diff: https://reviews.apache.org/r/49518/diff/
> 
> 
> Testing
> ---
> 
> Updated test and make check.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49518: Initial snapshot for v1 master event stream.

[Zhitao Li]

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

(Updated July 2, 2016, 12:20 a.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.


Changes
---

Review comments.


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


Repository: mesos


Description
---

Initial snapshot for v1 master event stream.


Diffs (updated)
-

  include/mesos/master/master.proto d06258e9fd39c7eefd8ecd394e3bdfb888479b1e 
  include/mesos/v1/master/master.proto b7cb6fdf2e4f34a11d326ac3ad3ec26525f8f343 
  src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
  src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 

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


Testing
---

Updated test and make check.


Thanks,

Zhitao Li

Re: Review Request 49517: Implement GetState V1 master API.

[Zhitao Li]

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

(Updated July 2, 2016, 12:19 a.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.


Changes
---

Review comments.


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


Repository: mesos


Description
---

The help function will also be used for snapshot of
event stream.


Diffs (updated)
-

  src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
  src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
  src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 

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


Testing
---

make check


Thanks,

Zhitao Li

Re: Review Request 48315: Restart slave if a volume is CREATED on a non-empty path.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [48313, 48314, 48315]

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

- Mesos ReviewBot


On July 1, 2016, 9:41 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48315/
> ---
> 
> (Updated July 1, 2016, 9:41 p.m.)
> 
> 
> Review request for mesos, Neil Conway and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5448
> https://issues.apache.org/jira/browse/MESOS-5448
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Root of a MOUNT disk is not deleted on volume DELETE. When we do a
> CREATE on a persistent volume and the root directory exists (which
> can happen for MOUNT disks), we allow the operation only if the
> contents within the root directory is empty. If not, we do not update
> the checkpoint with this volume and exit, so as to cleanup when the
> slave restarts and handles the CheckpointResourcesMessage.
> 
> 
> Diffs
> -
> 
>   docs/multiple-disk.md 20b99e0e57360ad43804201b27e593d7ed48ce2c 
>   src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
>   src/tests/persistent_volume_tests.cpp 
> 5125a8da44759d1235fddac26e9eb5436c3d037b 
> 
> Diff: https://reviews.apache.org/r/48315/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 49390: Added test for container image command task with health check.

[Gilbert Song]

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

(Updated July 1, 2016, 5 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, haosdent huang, 
Artem Harutyunyan, Jie Yu, and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added test for container image command task with health check.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1c4a554ab07731963a4a38e3ae40b0323bf317bb 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 49389: Refactored HealthCheck from a binary to be a library.

[Gilbert Song]

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

(Updated July 1, 2016, 5 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, haosdent huang, 
Artem Harutyunyan, Jie Yu, and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Refactored HealthCheck from a binary to be a library.


Diffs (updated)
-

  src/CMakeLists.txt 996d9e655b9d4343c07663fe840e94727cd792fe 
  src/Makefile.am 72f8a995f0a6373c0108900ff40395f698aa678e 
  src/health-check/health_checker.hpp PRE-CREATION 
  src/health-check/health_checker.cpp PRE-CREATION 
  src/health-check/main.cpp 4cc9dde8927acd4594477476f1b1bdc43963d789 
  src/launcher/executor.cpp cb23501b1be9371e634911132d95239be78a637d 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 49534: Fixed linux filesystem test recover orphaned persistent volume.

[Gilbert Song]

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

(Updated July 1, 2016, 5 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, Timothy Chen, and Jiang 
Yan Xu.


Changes
---

Rebase.


Repository: mesos


Description
---

Fixed linux filesystem test recover orphaned persistent volume.


Diffs (updated)
-

  src/tests/containerizer/filesystem_isolator_tests.cpp 
30aab7671c963d7a93379988fbe8968ce05233d0 

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


Testing
---

make check

sudo ./bin/mesos-test.sh with 100+ iterations


Thanks,

Gilbert Song

Re: Review Request 49388: Added devolve method for TaskID and HealthCheck.

[Gilbert Song]

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

(Updated July 1, 2016, 5 p.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, Artem Harutyunyan, 
Jie Yu, and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added devolve method for TaskID and HealthCheck.


Diffs (updated)
-

  src/internal/devolve.hpp 4a6ae681d37b3405ee81c4a58388b8d501743ebf 
  src/internal/devolve.cpp cecb22e49614c6ee47489eead1d3161e033a53ef 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 49533: Added sleep binary to test rootfs.

[Gilbert Song]

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

(Updated July 1, 2016, 5 p.m.)


Review request for mesos, haosdent huang, Artem Harutyunyan, Jie Yu, and 
Timothy Chen.


Changes
---

Rebase.


Repository: mesos


Description
---

Added sleep binary to test rootfs.


Diffs (updated)
-

  src/tests/containerizer/rootfs.hpp 26d0cfe061072ded78a5e0e36212b8ce5539454e 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 49532: Fixed unit test os test nonblock.

[Gilbert Song]

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

(Updated July 1, 2016, 5 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, and 
Joseph Wu.


Changes
---

Rebase.


Repository: mesos


Description
---

Fixed unit test os test nonblock.


Diffs (updated)
-

  3rdparty/stout/tests/os_tests.cpp 786da14addf62be5f5270f156fb1e011d3f403e3 

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 49543: Fix ProcessRemoteLinkTests that try to emulate 'stale' sockets.

[Joseph Wu]

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

Review request for mesos, Benjamin Mahler and Artem Harutyunyan.


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


Repository: mesos


Description
---

The ProcessRemoteLinkTests `RemoteUseStaleLink` and 
`RemoteStaleLinkRelink` were failing because the `test-linkee` was
closing sockets shortly after the test `::shutdown` the link.  This 
is the expected behavior of libprocess.

Rewrites the `test-linkee` to keep all sockets open, regardless of the
state of the connection.


Diffs
-

  3rdparty/libprocess/src/tests/test_linkee.cpp 
7ba0f4833a5163d2323e1477509fd2fbacc23690 

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


Testing
---

make check (CentOS 7)

3rdparty/libprocess/libprocess-tests --gtest_filter="ProcessRemoteLinkTest*" 
--gtest_break_on_failure --gtest_repeat=1


Thanks,

Joseph Wu

Review Request 49542: Explicitly passed in launcher dir to command executor.

[Jie Yu]

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

Review request for mesos, Gilbert Song, Joris Van Remoortere, and Timothy Chen.


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


Repository: mesos


Description
---

Relying on dirname is not quite reliable, especially in tests where
launchable scripts are under 'build/src', but actual binaries are
under 'build/src/.libs'. This patch passed in launcher_dir explicitly
through executor flags.


Diffs
-

  src/launcher/executor.cpp bb88a4570d183d22dff65f6e220d566c0924001a 
  src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
  src/tests/health_check_tests.cpp 1c4a554ab07731963a4a38e3ae40b0323bf317bb 
  src/tests/slave_tests.cpp d800cb8c10b5730a711c8354802bdff6aaca8c01 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 49390: Added test for container image command task with health check.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 1, 2016, 11:34 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49390/
> ---
> 
> (Updated July 1, 2016, 11:34 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, haosdent huang, 
> Artem Harutyunyan, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-5727
> https://issues.apache.org/jira/browse/MESOS-5727
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for container image command task with health check.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1c4a554ab07731963a4a38e3ae40b0323bf317bb 
> 
> Diff: https://reviews.apache.org/r/49390/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49516: Refactor Master::Http::getExecutors into helper function.

[Zhitao Li]

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

(Updated July 1, 2016, 11:45 p.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.


Changes
---

Review comments.


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


Repository: mesos


Description
---

Helper function will be reused by `GetExecutors` and `GetState`.


Diffs (updated)
-

  src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
  src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 

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


Testing
---

make check


Thanks,

Zhitao Li

Re: Review Request 49509: Revised protobuf definition of GetState response.

[Zhitao Li]

> On July 1, 2016, 7:22 p.m., Vinod Kone wrote:
> > include/mesos/master/master.proto, lines 273-274
> > 
> >
> > move this to GetLeadingMaster.

Removed. If we want this in the other call, let's do it in a separate patch.


> On July 1, 2016, 7:22 p.m., Vinod Kone wrote:
> > include/mesos/v1/master/master.proto, lines 274-275
> > 
> >
> > ditto. move this to GetLeadingMaster

Ditto.


- Zhitao


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


On July 1, 2016, 11:43 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49509/
> ---
> 
> (Updated July 1, 2016, 11:43 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Revised protobuf definition of GetState response.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto d06258e9fd39c7eefd8ecd394e3bdfb888479b1e 
>   include/mesos/v1/master/master.proto 
> b7cb6fdf2e4f34a11d326ac3ad3ec26525f8f343 
> 
> Diff: https://reviews.apache.org/r/49509/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49509: Revised protobuf definition of GetState response.

[Zhitao Li]

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

(Updated July 1, 2016, 11:43 p.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.


Changes
---

Drop started and elected time. If we want them in `GetLeadingMaster`, that will 
be a separate patch.


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


Repository: mesos


Description
---

Revised protobuf definition of GetState response.


Diffs (updated)
-

  include/mesos/master/master.proto d06258e9fd39c7eefd8ecd394e3bdfb888479b1e 
  include/mesos/v1/master/master.proto b7cb6fdf2e4f34a11d326ac3ad3ec26525f8f343 

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


Testing
---

make check


Thanks,

Zhitao Li

Re: Review Request 49489: Refactor master::Http::getFrameworks to helper function.

[Zhitao Li]

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

(Updated July 1, 2016, 11:42 p.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.


Changes
---

Review comments.


Repository: mesos


Description
---

The new helper function will be reused by `GET_FRAMEWORKS` and
`GET_STATE` calls.


Diffs (updated)
-

  src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
  src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 

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


Testing
---

make check


Thanks,

Zhitao Li

Re: Review Request 49488: Refactor Master::Http::getAgents into helper function.

[Zhitao Li]

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

(Updated July 1, 2016, 11:34 p.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.


Changes
---

Review comments.


Summary (updated)
-

Refactor Master::Http::getAgents into helper function.


Repository: mesos


Description
---

The new helper function will be reused by both `GET_AGENTS`
and `GET_STATE` calls.


Diffs (updated)
-

  src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
  src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 

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


Testing
---

make check


Thanks,

Zhitao Li

Re: Review Request 49390: Added test for container image command task with health check.

[Gilbert Song]

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

(Updated July 1, 2016, 4:34 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, haosdent huang, 
Artem Harutyunyan, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

Added test for container image command task with health check.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1c4a554ab07731963a4a38e3ae40b0323bf317bb 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 49389: Refactored HealthCheck from a binary to be a library.

[Gilbert Song]

> On July 1, 2016, 3:24 p.m., Jie Yu wrote:
> > src/launcher/executor.cpp, lines 431-432
> > 
> >
> > Hum, if `_check.isError()`, `checker->healthCheck()` below will abort. 
> > you should put `checker->healthCheck()` in else block

My bad! Fool mistake!


- Gilbert


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


On July 1, 2016, 4:33 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49389/
> ---
> 
> (Updated July 1, 2016, 4:33 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, haosdent huang, 
> Artem Harutyunyan, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-5727
> https://issues.apache.org/jira/browse/MESOS-5727
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored HealthCheck from a binary to be a library.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 996d9e655b9d4343c07663fe840e94727cd792fe 
>   src/Makefile.am 72f8a995f0a6373c0108900ff40395f698aa678e 
>   src/health-check/health_checker.hpp PRE-CREATION 
>   src/health-check/health_checker.cpp PRE-CREATION 
>   src/health-check/main.cpp 4cc9dde8927acd4594477476f1b1bdc43963d789 
>   src/launcher/executor.cpp bb88a4570d183d22dff65f6e220d566c0924001a 
> 
> Diff: https://reviews.apache.org/r/49389/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49389: Refactored HealthCheck from a binary to be a library.

[Gilbert Song]

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

(Updated July 1, 2016, 4:33 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, haosdent huang, 
Artem Harutyunyan, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

Refactored HealthCheck from a binary to be a library.


Diffs (updated)
-

  src/CMakeLists.txt 996d9e655b9d4343c07663fe840e94727cd792fe 
  src/Makefile.am 72f8a995f0a6373c0108900ff40395f698aa678e 
  src/health-check/health_checker.hpp PRE-CREATION 
  src/health-check/health_checker.cpp PRE-CREATION 
  src/health-check/main.cpp 4cc9dde8927acd4594477476f1b1bdc43963d789 
  src/launcher/executor.cpp bb88a4570d183d22dff65f6e220d566c0924001a 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 49487: Refactor Master::Http::getTasks into helper function.

[Zhitao Li]

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

(Updated July 1, 2016, 11:32 p.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.


Changes
---

Review comments.


Summary (updated)
-

Refactor Master::Http::getTasks into helper function.


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


Repository: mesos


Description
---

Helper function will be also be reused for `GetState`.


Diffs (updated)
-

  src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
  src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 

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


Testing
---

make check


Thanks,

Zhitao Li

Review Request 49541: Renamed healthCheckDir to launcherDir in command executor.

[Jie Yu]

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

Review request for mesos, Gilbert Song, Joris Van Remoortere, and Timothy Chen.


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


Repository: mesos


Description
---

Renamed healthCheckDir to launcherDir in command executor.


Diffs
-

  src/launcher/executor.cpp bb88a4570d183d22dff65f6e220d566c0924001a 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 49540: Used the argv version for command that launches the command executor.

[Jie Yu]

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

Review request for mesos, Gilbert Song, Ian Downes, and Vinod Kone.


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


Repository: mesos


Description
---

Used the argv version for command that launches the command executor.


Diffs
-

  src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
  src/tests/slave_tests.cpp d800cb8c10b5730a711c8354802bdff6aaca8c01 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 49472: Fixed some style issue in stout Makefile.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On June 30, 2016, 1:52 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49472/
> ---
> 
> (Updated June 30, 2016, 1:52 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed some style issue in stout Makefile.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am f10c836c1ac008cc4055741648b5e7dd697e4c1e 
> 
> Diff: https://reviews.apache.org/r/49472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49425: Used os::raw::Argv in command executor.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On June 30, 2016, 9:18 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49425/
> ---
> 
> (Updated June 30, 2016, 9:18 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used os::raw::Argv in command executor.
> 
> 
> Diffs
> -
> 
>   src/launcher/posix/executor.cpp ab1dd938a7c59f52b44aafe1340c614087460f84 
>   src/launcher/windows/executor.cpp f6da398eeddaf68b8eaf510648c55964d9f5b7c7 
> 
> Diff: https://reviews.apache.org/r/49425/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49424: Added an abstraction os::raw::Argv in stout.

[Gilbert Song]

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


Ship it!




LGTM!

- Gilbert Song


On June 30, 2016, 9:18 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49424/
> ---
> 
> (Updated June 30, 2016, 9:18 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an abstraction os::raw::Argv in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am f10c836c1ac008cc4055741648b5e7dd697e4c1e 
>   3rdparty/stout/include/stout/os.hpp 
> 53b00932693fba7cf6514da6a519269a904de345 
>   3rdparty/stout/include/stout/os/raw/argv.hpp PRE-CREATION 
>   3rdparty/stout/tests/os_tests.cpp 786da14addf62be5f5270f156fb1e011d3f403e3 
> 
> Diff: https://reviews.apache.org/r/49424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49390: Added test for container image command task with health check.

[Jie Yu]

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




src/tests/health_check_tests.cpp (line 309)


I would rename this to ROOT_HealtyTaskWithContainerImage



src/tests/health_check_tests.cpp (lines 383 - 385)


Why we need reconciliation here?


- Jie Yu


On July 1, 2016, 9:54 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49390/
> ---
> 
> (Updated July 1, 2016, 9:54 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, haosdent huang, 
> Artem Harutyunyan, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-5727
> https://issues.apache.org/jira/browse/MESOS-5727
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for container image command task with health check.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1c4a554ab07731963a4a38e3ae40b0323bf317bb 
> 
> Diff: https://reviews.apache.org/r/49390/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49480: Added installing NVML headers with --enable-install-module-dependencies.

[Kevin Klues]

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

(Updated July 1, 2016, 10:32 p.m.)


Review request for mesos, Alexander Rojas, Kapil Arya, and Till Toenshoff.


Changes
---

Updated to just use `cp` to do install, rather than rolling our own Makefile 
and bundling it just to copy on header file.


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


Repository: mesos


Description
---

Added installing NVML headers with --enable-install-module-dependencies.


Diffs (updated)
-

  3rdparty/Makefile.am 2b243878d9688421c0847a3387611616588185ed 

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


Testing
---

../configure --enable-install-module-dependencies --prefix=$HOME/mesos-install
make install
find $HOME/mesos-install -name nvml.h
  --> $HOME/mesos-install/lib/mesos/3rdparty/include/nvidia/gdk/nvml.h


Thanks,

Kevin Klues

Re: Review Request 49520: Fixed the flaky BusyMountPoint test.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49520]

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

- Mesos ReviewBot


On July 1, 2016, 6:22 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49520/
> ---
> 
> (Updated July 1, 2016, 6:22 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5752
> https://issues.apache.org/jira/browse/MESOS-5752
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In ROOT_GarbageCollectorUndeletableFilesTest.BusyMountPoint there is
> a race between the task creating a file and the test looking for it
> which sometimes leads to assertion failure on the existence of the
> file.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp 2e23f04ea908aadcefb21e11203b95b94ec3c60b 
> 
> Diff: https://reviews.apache.org/r/49520/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 45967: Added documentation for shareable resources.

[Anindya Sinha]

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

(Updated July 1, 2016, 10:29 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added documentation for shareable resources.


Diffs (updated)
-

  docs/home.md 2d728333a3c1421b310d861b71c92691fd41a482 
  docs/shared-resources.md PRE-CREATION 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 49472: Fixed some style issue in stout Makefile.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On June 30, 2016, 8:52 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49472/
> ---
> 
> (Updated June 30, 2016, 8:52 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed some style issue in stout Makefile.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am f10c836c1ac008cc4055741648b5e7dd697e4c1e 
> 
> Diff: https://reviews.apache.org/r/49472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45961: Support sharing of resources through reference counting of resources.

[Anindya Sinha]

> On June 14, 2016, 10:50 p.m., Jiang Yan Xu wrote:
> > src/master/allocator/sorter/drf/sorter.cpp, lines 457-467
> > 
> >
> > For this we need to
> > 
> > 1. Run existing benchmarks to see how much performance degradation is 
> > there for clusters (large number of agents or frameworks) that are not 
> > using shared resources. Ideally there should be negligible performance 
> > impact for such clusters.
> > 2. Create a benchmark that uses shared resources in a realistic way.
> > 
> > In implementing this patch we do have the options to store shared 
> > resources separately in a few places (e.g., store shared resources outside 
> > of `allocations[name].resources`) that can help with performance but the 
> > trade off is that it may increase code complexity. Benchmarks can help us 
> > address these tradeoffs.

Verified that changes in allocator and sorter does not affect run time 
performance adversely for regular resources. I will add a benchmark for shared 
resources in the next update.


- Anindya


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


On June 23, 2016, 3:50 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45961/
> ---
> 
> (Updated June 23, 2016, 3:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, Michael 
> Park, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Each shared resource is accouted via its share count. This count is
> updated based on the resource operations (such as add and subtract)
> in various scenarios such as task launch and terminate at multiple
> modules such as master, allocator, sorter, etc.
> Only allow DESTROY if no task is using the volume, or has not been
> offered to any framework.
> Since shared resources are available to multiple frameworks of the
> same role simultaneously, we normalize it with a weight equivalent
> to the number of frameworks to which this shared resource has been
> allocated to in the sorter.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/common/resources_utils.cpp 8e881a09500b0966b577e514077460f724868a8d 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5b2331b3f99252fd16f2514123006dd4ba7f1c0d 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 35273b5dcf39938125a112c5e56b5a8a542157db 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 27d56f274c41bbabe6f2abbbcebd2c4eff52693e 
>   src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e 
>   src/master/master.hpp fe57878dc59637459d5c5cdae0be2aa159133fa4 
>   src/master/master.cpp 8def7156f4a05b39590321ce7743f7385a68bed0 
>   src/master/validation.hpp 43d876b00511d4ef652c2685d4950c994b5eef31 
>   src/master/validation.cpp 9120b71fc7725bdf7094aac6619d8aadcc352df5 
>   src/tests/master_validation_tests.cpp 
> 9eb82a569f7c48caa96d4d54a93b199ccac74385 
>   src/tests/sorter_tests.cpp 6fc58829892dc0223140f1b47593a3e5853cace5 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> Diff: https://reviews.apache.org/r/45961/diff/
> 
> 
> Testing
> ---
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45961: Support sharing of resources through reference counting of resources.

[Anindya Sinha]

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

(Updated July 1, 2016, 10:28 p.m.)


Review request for mesos, Benjamin Mahler, Joris Van Remoortere, Michael Park, 
and Jiang Yan Xu.


Changes
---

Following changes have been addressed for shared resources based on discussion 
in Mesos Working Group:
1) DESTROY on a persistent volume should not be allowed if there is a running 
or pending task with that persistent volume. However, if the volume to be 
DESTROYed has been offered to another framework, proceed with the DESTROY but 
rescind all such offers.
2) Give equal share to all clients in calculation of dominant resource if that 
client has a shared resource allocated (instead of any normalization).


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


Repository: mesos


Description (updated)
---

o Each shared resource is accouted via its share count. This count is
  updated based on the resource operations (such as add and subtract)
  in various scenarios such as task launch and terminate at multiple
  modules such as master, allocator, sorter, etc.
o Only allow DESTROY if there are no running or pending tasks using
  the volume. However, if the volume is in a pending offer to one or
  more frameworks, resind that offer before processing the DESTROY.


Diffs (updated)
-

  src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
  src/common/resources_utils.cpp 8e881a09500b0966b577e514077460f724868a8d 
  src/master/allocator/mesos/hierarchical.cpp 
eca949e07abb00423a2f35a56eedc5d4287d81f3 
  src/master/allocator/sorter/drf/sorter.hpp 
0aa1a71da4501a3b469d07538a043b4c1d74d688 
  src/master/allocator/sorter/drf/sorter.cpp 
967290d4d1100208900b4b724422c3218abc23cb 
  src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
  src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
  src/master/master.cpp 78a8889313179b4509a6657e6c61d9f6d3bb99c0 
  src/master/validation.hpp 43d876b00511d4ef652c2685d4950c994b5eef31 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
  src/tests/master_validation_tests.cpp 
9eb82a569f7c48caa96d4d54a93b199ccac74385 
  src/tests/sorter_tests.cpp 6fc58829892dc0223140f1b47593a3e5853cace5 
  src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45966: Offer shareable resources to frameworks only if opted in.

[Anindya Sinha]

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

(Updated July 1, 2016, 10:29 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added a new capability SHAREABLE_RESOURCES that frameworks need to opt
in if they are interested in receiving shared resources in their
offers.


Diffs (updated)
-

  include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
  include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/master/allocator/mesos/hierarchical.hpp 
b72ba16277a3210e4d309b616d185a10e2029a66 
  src/master/allocator/mesos/hierarchical.cpp 
eca949e07abb00423a2f35a56eedc5d4287d81f3 
  src/tests/hierarchical_allocator_tests.cpp 
0498cd5e54b0e4b87a767585a77699653aa52179 

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


Testing
---

Tests updated with new capability.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45964: Add unit tests for sharing of resources.

[Anindya Sinha]

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

(Updated July 1, 2016, 10:29 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Updated the sorter test for shared resources based on giving equal share for 
calculation of dominant resource across multiple clients in the sorter which 
has the same shared resource allocated.


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


Repository: mesos


Description
---

Add unit tests for sharing of resources.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
0498cd5e54b0e4b87a767585a77699653aa52179 
  src/tests/master_validation_tests.cpp 
9eb82a569f7c48caa96d4d54a93b199ccac74385 
  src/tests/mesos.hpp 3173bf9cfc832be0aa18d745434afc45d352a269 
  src/tests/persistent_volume_tests.cpp 
5125a8da44759d1235fddac26e9eb5436c3d037b 
  src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 
  src/tests/sorter_tests.cpp 6fc58829892dc0223140f1b47593a3e5853cace5 

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


Testing
---

Tests for shared resources added for allocator, resources and sorter.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 49389: Refactored HealthCheck from a binary to be a library.

[Jie Yu]

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


Fix it, then Ship it!





src/health-check/health_checker.hpp (line 64)


Let's kill this and use explicit includes.



src/launcher/executor.cpp (lines 430 - 431)


Hum, if `_check.isError()`, `checker->healthCheck()` below will abort. you 
should put `checker->healthCheck()` in else block


- Jie Yu


On July 1, 2016, 9:54 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49389/
> ---
> 
> (Updated July 1, 2016, 9:54 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, haosdent huang, 
> Artem Harutyunyan, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-5727
> https://issues.apache.org/jira/browse/MESOS-5727
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored HealthCheck from a binary to be a library.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 996d9e655b9d4343c07663fe840e94727cd792fe 
>   src/Makefile.am 72f8a995f0a6373c0108900ff40395f698aa678e 
>   src/health-check/health_checker.hpp PRE-CREATION 
>   src/health-check/health_checker.cpp PRE-CREATION 
>   src/health-check/main.cpp 4cc9dde8927acd4594477476f1b1bdc43963d789 
>   src/launcher/executor.cpp bb88a4570d183d22dff65f6e220d566c0924001a 
> 
> Diff: https://reviews.apache.org/r/49389/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49532: Fixed unit test os test nonblock.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On July 1, 2016, 9:54 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49532/
> ---
> 
> (Updated July 1, 2016, 9:54 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed unit test os test nonblock.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/os_tests.cpp 786da14addf62be5f5270f156fb1e011d3f403e3 
> 
> Diff: https://reviews.apache.org/r/49532/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49480: Added installing NVML headers with --enable-install-module-dependencies.

[Kapil Arya]

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


Ship it!




Ship It!

- Kapil Arya


On June 30, 2016, 7:54 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49480/
> ---
> 
> (Updated June 30, 2016, 7:54 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Kapil Arya, and Till Toenshoff.
> 
> 
> Bugs: MESOS-5755
> https://issues.apache.org/jira/browse/MESOS-5755
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added installing NVML headers with --enable-install-module-dependencies.
> 
> 
> Diffs
> -
> 
>   3rdparty/Makefile.am 2b24387 
>   3rdparty/nvml-352.79.tar.gz 04eee42c306344369b27ffe5f884663e634960b1 
> 
> Diff: https://reviews.apache.org/r/49480/diff/
> 
> 
> Testing
> ---
> 
> ../configure --enable-install-module-dependencies --prefix=$HOME/mesos-install
> make install
> find $HOME/mesos-install -name nvml.h
>   --> $HOME/mesos-install/lib/mesos/3rdparty/include/nvidia/gdk/nvml.h
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49533: Added sleep binary to test rootfs.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 1, 2016, 9:54 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49533/
> ---
> 
> (Updated July 1, 2016, 9:54 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Artem Harutyunyan, Jie Yu, and 
> Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added sleep binary to test rootfs.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/rootfs.hpp 26d0cfe061072ded78a5e0e36212b8ce5539454e 
> 
> Diff: https://reviews.apache.org/r/49533/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49534: Fixed linux filesystem test recover orphaned persistent volume.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 1, 2016, 9:54 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49534/
> ---
> 
> (Updated July 1, 2016, 9:54 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, Timothy Chen, and Jiang 
> Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed linux filesystem test recover orphaned persistent volume.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 30aab7671c963d7a93379988fbe8968ce05233d0 
> 
> Diff: https://reviews.apache.org/r/49534/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-test.sh with 100+ iterations
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49388: Added devolve method for TaskID and HealthCheck.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 1, 2016, 9:54 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49388/
> ---
> 
> (Updated July 1, 2016, 9:54 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, Artem Harutyunyan, 
> Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-5727
> https://issues.apache.org/jira/browse/MESOS-5727
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added devolve method for TaskID and HealthCheck.
> 
> 
> Diffs
> -
> 
>   src/internal/devolve.hpp 4a6ae681d37b3405ee81c4a58388b8d501743ebf 
>   src/internal/devolve.cpp cecb22e49614c6ee47489eead1d3161e033a53ef 
> 
> Diff: https://reviews.apache.org/r/49388/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 48313: Creation and deletion of persistent volumes across agent restart.

[Anindya Sinha]

> On June 23, 2016, 5:35 p.m., Jiang Yan Xu wrote:
> > src/slave/state.hpp, line 271
> > 
> >
> > It looks a bit odd that we are explicitly initiating `target` but not 
> > `resources`.
> > 
> > Note that it's not necessary to initiate either but it is necessary to 
> > initiate errors here.

Ok. Removed explicit initialization of `target` to `None()`.


> On June 23, 2016, 5:35 p.m., Jiang Yan Xu wrote:
> > src/slave/state.hpp, lines 277-280
> > 
> >
> > s/parseResource/recoverResources/ and this could just be a non-member 
> > helper hidden in the cpp file (which I suspect can be replaced in a 
> > followup).

Renamed `parseResource()` to `recoverResources()`.


> On June 23, 2016, 5:35 p.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp, lines 2579-2580
> > 
> >
> > This is more of a comment on the later review:
> > 
> > Here we don't know if framework intends to create a new persistent 
> > volume, i.e., if both the new checkpointed resources and the old have the 
> > same persistent volume and the disk has the directory and it's not empty -> 
> > its OK!
> > 
> > We only want to make sure the directory is empty when the new 
> > checkpointed resources' volume is not in the old checkpointed resources.
> > 
> > Plus this condition only applies to MOUNT, we can easily check/CHECK 
> > the resources to find out.

Added an appropriate comment in https://reviews.apache.org/r/48315.


- Anindya


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


On July 1, 2016, 9:39 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48313/
> ---
> 
> (Updated July 1, 2016, 9:39 p.m.)
> 
> 
> Review request for mesos, Neil Conway and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5448
> https://issues.apache.org/jira/browse/MESOS-5448
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the agent receives CheckpointedResourcesMessage, we store the
> target checkpoint on disk. On successful create and destroy of
> persistent volumes as a part of handling this messages, we commit
> the checkpoint on the disk, and clear the target checkpoint.
> 
> However, incase of any failure we do not commit the checkpoint to
> disk, and exit the agent. When the agent restarts and there is a
> target checkpoint present on disk which differs from the committed
> checkpoint, we retry to sync the target and committed checkpoint.
> On success, we reregister the agent with the master, but in case it
> fails, we do not commit the checkpoint and the agent exists.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.hpp 339e539863c678b6ed4d4670d75c7ff4c54daa79 
>   src/slave/paths.cpp 03157f93b1e703006f95ef6d0a30afae375dcdb5 
>   src/slave/slave.hpp 484ba758b4c87935aabd2f76a0e654a3c6d09167 
>   src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
>   src/slave/state.hpp 0de2a4ee4fabaad612c4526166157b001c380bdb 
>   src/slave/state.cpp 9cec0868b1187ed3ccac7f065e8a21c2f52178d9 
> 
> Diff: https://reviews.apache.org/r/48313/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 49532: Fixed unit test os test nonblock.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 1, 2016, 9:54 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49532/
> ---
> 
> (Updated July 1, 2016, 9:54 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed unit test os test nonblock.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/os_tests.cpp 786da14addf62be5f5270f156fb1e011d3f403e3 
> 
> Diff: https://reviews.apache.org/r/49532/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 49533: Added sleep binary to test rootfs.

[Gilbert Song]

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

Review request for mesos, haosdent huang, Artem Harutyunyan, Jie Yu, and 
Timothy Chen.


Repository: mesos


Description
---

Added sleep binary to test rootfs.


Diffs
-

  src/tests/containerizer/rootfs.hpp 26d0cfe061072ded78a5e0e36212b8ce5539454e 

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 49534: Fixed linux filesystem test recover orphaned persistent volume.

[Gilbert Song]

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

Review request for mesos, Artem Harutyunyan, Jie Yu, Timothy Chen, and Jiang 
Yan Xu.


Repository: mesos


Description
---

Fixed linux filesystem test recover orphaned persistent volume.


Diffs
-

  src/tests/containerizer/filesystem_isolator_tests.cpp 
30aab7671c963d7a93379988fbe8968ce05233d0 

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


Testing
---

make check

sudo ./bin/mesos-test.sh with 100+ iterations


Thanks,

Gilbert Song

Re: Review Request 49389: Refactored HealthCheck from a binary to be a library.

[Gilbert Song]

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

(Updated July 1, 2016, 2:54 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, haosdent huang, 
Artem Harutyunyan, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

Refactored HealthCheck from a binary to be a library.


Diffs (updated)
-

  src/CMakeLists.txt 996d9e655b9d4343c07663fe840e94727cd792fe 
  src/Makefile.am 72f8a995f0a6373c0108900ff40395f698aa678e 
  src/health-check/health_checker.hpp PRE-CREATION 
  src/health-check/health_checker.cpp PRE-CREATION 
  src/health-check/main.cpp 4cc9dde8927acd4594477476f1b1bdc43963d789 
  src/launcher/executor.cpp bb88a4570d183d22dff65f6e220d566c0924001a 

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


Testing (updated)
---

make check


Thanks,

Gilbert Song

Re: Review Request 49390: Added test for container image command task with health check.

[Gilbert Song]

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

(Updated July 1, 2016, 2:54 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, haosdent huang, 
Artem Harutyunyan, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

Added test for container image command task with health check.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1c4a554ab07731963a4a38e3ae40b0323bf317bb 

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


Testing (updated)
---

make check


Thanks,

Gilbert Song

Re: Review Request 49388: Added devolve method for TaskID and HealthCheck.

[Gilbert Song]

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

(Updated July 1, 2016, 2:54 p.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, Artem Harutyunyan, 
Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

Added devolve method for TaskID and HealthCheck.


Diffs (updated)
-

  src/internal/devolve.hpp 4a6ae681d37b3405ee81c4a58388b8d501743ebf 
  src/internal/devolve.cpp cecb22e49614c6ee47489eead1d3161e033a53ef 

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


Testing (updated)
---

make check


Thanks,

Gilbert Song

Review Request 49532: Fixed unit test os test nonblock.

[Gilbert Song]

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

Review request for mesos, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, and 
Joseph Wu.


Repository: mesos


Description
---

Fixed unit test os test nonblock.


Diffs
-

  3rdparty/stout/tests/os_tests.cpp 786da14addf62be5f5270f156fb1e011d3f403e3 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 49531: Added 'systemGetDriverVersion' to NVML abstraction.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On July 1, 2016, 9:41 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49531/
> ---
> 
> (Updated July 1, 2016, 9:41 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5765
> https://issues.apache.org/jira/browse/MESOS-5765
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This command returns a string representing the version of the
> underlying Nvidia drivers installed on a host.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/gpu/nvml.hpp 
> 15217407acdb6ceb6efc695580b1f441d1891daf 
>   src/slave/containerizer/mesos/isolators/gpu/nvml.cpp 
> 8197cb1b0bc35517a1876f8d8ac18e0d4e550a17 
> 
> Diff: https://reviews.apache.org/r/49531/diff/
> 
> 
> Testing
> ---
> 
> `sudo GTEST_FILTER="*NVIDIA*" bin/mesos-tests.sh`
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49348: Added implementation to Appc Runtime Isolator.

[Srinivas Brahmaroutu]

> On July 1, 2016, 7:30 a.m., Guangya Liu wrote:
> > I think that this patch should be created after the function patches 
> > finished.

@gyliu I have merged runtime implementation to here and moving the testing to 
another patch.


- Srinivas


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


On July 1, 2016, 9:44 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49348/
> ---
> 
> (Updated July 1, 2016, 9:44 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation to Appc Runtime Isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49348/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49348: Added implementation to Appc Runtime Isolator.

[Srinivas Brahmaroutu]

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

(Updated July 1, 2016, 9:44 p.m.)


Review request for mesos.


Summary (updated)
-

Added implementation to Appc Runtime Isolator.


Repository: mesos


Description (updated)
---

Added implementation to Appc Runtime Isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/appc/runtime.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Review Request 49531: Added 'systemGetDriverVersion' to NVML abstraction.

[Kevin Klues]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This command returns a string representing the version of the
underlying Nvidia drivers installed on a host.


Diffs
-

  src/slave/containerizer/mesos/isolators/gpu/nvml.hpp 
15217407acdb6ceb6efc695580b1f441d1891daf 
  src/slave/containerizer/mesos/isolators/gpu/nvml.cpp 
8197cb1b0bc35517a1876f8d8ac18e0d4e550a17 

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


Testing
---

`sudo GTEST_FILTER="*NVIDIA*" bin/mesos-tests.sh`


Thanks,

Kevin Klues

Re: Review Request 48315: Restart slave if a volume is CREATED on a non-empty path.

[Anindya Sinha]

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

(Updated July 1, 2016, 9:41 p.m.)


Review request for mesos, Neil Conway and Jiang Yan Xu.


Changes
---

Added a unit test to check for failed slave recovery if the target checkpoint 
contains a volume to be CREATEd which has non-empty contents.


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


Repository: mesos


Description
---

Root of a MOUNT disk is not deleted on volume DELETE. When we do a
CREATE on a persistent volume and the root directory exists (which
can happen for MOUNT disks), we allow the operation only if the
contents within the root directory is empty. If not, we do not update
the checkpoint with this volume and exit, so as to cleanup when the
slave restarts and handles the CheckpointResourcesMessage.


Diffs (updated)
-

  docs/multiple-disk.md 20b99e0e57360ad43804201b27e593d7ed48ce2c 
  src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
  src/tests/persistent_volume_tests.cpp 
5125a8da44759d1235fddac26e9eb5436c3d037b 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 48315: Restart slave if a volume is CREATED on a non-empty path.

[Anindya Sinha]

> On June 23, 2016, 10:30 a.m., Neil Conway wrote:
> > Can you write a unit test that verifies the main functional change in this 
> > review? That is, if the user attempts to create a persistent volume on a 
> > path that already contains files, the creation fails and the slave exits 
> > with an error. (We could maybe test the behavior upon restartig the 
> > slave...)

Added a unit test to check for failed slave recovery if the target checkpoint 
contains a volume to be CREATEd which has non-empty contents.


- Anindya


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


On June 20, 2016, 11:41 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48315/
> ---
> 
> (Updated June 20, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Neil Conway and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5448
> https://issues.apache.org/jira/browse/MESOS-5448
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Root of a MOUNT disk is not deleted on volume DELETE. When we do a
> CREATE on a persistent volume and the root directory exists (which
> can happen for MOUNT disks), we allow the operation only if the
> contents within the root directory is empty. If not, we do not update
> the checkpoint with this volume and exit, so as to cleanup when the
> slave restarts and handles the CheckpointResourcesMessage.
> 
> 
> Diffs
> -
> 
>   docs/multiple-disk.md 20b99e0e57360ad43804201b27e593d7ed48ce2c 
>   src/slave/slave.cpp 4bf01f2b020f5e975fb57cffcd19865d7431eac2 
>   src/tests/persistent_volume_tests.cpp 
> 5125a8da44759d1235fddac26e9eb5436c3d037b 
> 
> Diff: https://reviews.apache.org/r/48315/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 48313: Creation and deletion of persistent volumes across agent restart.

[Anindya Sinha]

> On June 20, 2016, 5:16 p.m., Jiang Yan Xu wrote:
> > In terms of testing, if we don't crash the agent within 
> > `syncCheckpointedResources()` but rather return a failure when its fails 
> > during recovery, we can capture this in `Slave::__recover` and verify the 
> > failed future right?
> 
> Jiang Yan Xu wrote:
> Echoing Neil on /r/48315/, can we add a test?

Added in https://reviews.apache.org/r/48315/.


> On June 20, 2016, 5:16 p.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp, lines 2520-2521
> > 
> >
> > If this fails, we don't know how it has failed and if the agent crashes 
> > next and restarts, it's possible for the agent to recover some non-empty 
> > resources from the target and erroneously starts deleting directories. 
> > Possible?
> 
> Anindya Sinha wrote:
> is it possible? `os::rm()` for files calls `unlink()` which clears the 
> inode entry for that file (assuming no other links to that file), although 
> the data may still exist on disk (until another write operation overwrites 
> the actual bits). And isn't clearing the inode entry an atomic operation? So 
> I do not think there is a case where the data is partial.
> 
> Jiang Yan Xu wrote:
> I was referring to the use of 
> `state::checkpoint(paths::getResourcesTargetPath(metaDir), Resources())` to 
> clean up target checkpointed resources. If we use `os::rm()` it should be no 
> problem.

Ok. Using os::rm().


> On June 20, 2016, 5:16 p.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp, lines 2552-2553
> > 
> >
> > Can we directly return in case of an error instead of using variables 
> > like this?
> 
> Anindya Sinha wrote:
> There are 3 error conditions here so to consolidate return from one place 
> which I believe is better than return with the same failures from 3 places... 
> hence, the variables. I can certainly consolidate `isFailed` and 
> `additionalReason` into `Option additionalReason = None()` to capture 
> any errors (and not need `isFailed` in this case).
> 
> I just realized this RR has 1 condition only, but when the changes for RR 
> 48315 is added, there are 3 conditions. Let me know if you want me to remove 
> `additionalReason` from this RR and add it in RR 48315. Again, all the RRs 
> 48313 through to 48315 go together.
> 
> Jiang Yan Xu wrote:
> The thing is that the 3 places don't return the same failure, right? They 
> are errors from three different actions. Combining with /r/48315/, the three 
> errors are (forgive my line length):
> 
> ```
> return Error("Failed to create the persistent volume" + 
> volume.disk().persistence().id() + "at '" + path + "': " + mkdir.error());
> 
> return Error("Failed to determine the disk status of persistent volume" + 
> volume.disk().persistence().id() + "at '" + path + "': " + empty.error());
> 
> return Error("New persistent volume" + volume.disk().persistence().id() + 
> "at '" + path + "' already exists on disk and isn't empty"
> ```
> 
> The only thing they share is `"persistent volume " + 
> volume.disk().persistence().id() + "at '" + path + "'` so I would use a 
> variable for that:
> 
> `string volumeDescription = "persistent volume " + 
> volume.disk().persistence().id() + "at '" + path + "'`;
> 
> As a reader I think this is clearer than tracing 
> `additionalReason.isSome()` or `isFailed` in multiple branches to determine 
> where they lead to.
> 
> Speaking of the relationship between /r/48313/ and /r/48315/, mutliple 
> places in this review point to the later review and you have to use a TODO. 
> Would it be cleaner to pull out /r/48314/ as the first review in the chain 
> and combine the other two? Up to you but if it's more effort to seperate 
> reviews cleanly then leaving them as one is OK too (and it makes it easier to 
> review).

TBH, I do not see how using `additionalReason` to return `Error()` from a 
single place is confusing at all. The multiple branches are setting the actual 
error which is returned from a single place which I think is a very common 
pattern.
Obviously, your recommendation of returning `Error()` directly from the 3 
conditions is fine also.
To avoid unnecessary to and fro on this, I updated the code though.

Regarding the sequence of RRs, let us keep the 3 RRs like we have been doing.


> On June 20, 2016, 5:16 p.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp, lines 2583-2591
> > 
> >
> > This comment is supposed to be revised together with /r/48315/?
> 
> Anindya Sinha wrote:
> Since RRs 48313 through to 48315 go together, do  you want to change this 
> comment in this RR?
> 
> Jiang Yan Xu wrote:
> I should say that the comment about "directory should be empty" seems to 
> not only 

Re: Review Request 48313: Creation and deletion of persistent volumes across agent restart.

[Anindya Sinha]

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

(Updated July 1, 2016, 9:39 p.m.)


Review request for mesos, Neil Conway and Jiang Yan Xu.


Changes
---

Minor changes based on code review comments.


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


Repository: mesos


Description
---

When the agent receives CheckpointedResourcesMessage, we store the
target checkpoint on disk. On successful create and destroy of
persistent volumes as a part of handling this messages, we commit
the checkpoint on the disk, and clear the target checkpoint.

However, incase of any failure we do not commit the checkpoint to
disk, and exit the agent. When the agent restarts and there is a
target checkpoint present on disk which differs from the committed
checkpoint, we retry to sync the target and committed checkpoint.
On success, we reregister the agent with the master, but in case it
fails, we do not commit the checkpoint and the agent exists.


Diffs (updated)
-

  src/slave/paths.hpp 339e539863c678b6ed4d4670d75c7ff4c54daa79 
  src/slave/paths.cpp 03157f93b1e703006f95ef6d0a30afae375dcdb5 
  src/slave/slave.hpp 484ba758b4c87935aabd2f76a0e654a3c6d09167 
  src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
  src/slave/state.hpp 0de2a4ee4fabaad612c4526166157b001c380bdb 
  src/slave/state.cpp 9cec0868b1187ed3ccac7f065e8a21c2f52178d9 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 49415: Removed the argv parameter in command executor helper.

[Gilbert Song]

> On July 1, 2016, 2:33 p.m., Gilbert Song wrote:
> > Do you miss `windows/executor.hpp`?

Fix it, Ship it! :)


- Gilbert


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


On June 29, 2016, 5:54 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49415/
> ---
> 
> (Updated June 29, 2016, 5:54 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's confusing to pass in 'argv' because it's already contained in
> 'command'. This patch removed this parameter.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp bb88a4570d183d22dff65f6e220d566c0924001a 
>   src/launcher/posix/executor.hpp e5f3c0a8e2d3687a5330adbf2f667db9106970c7 
>   src/launcher/posix/executor.cpp ab1dd938a7c59f52b44aafe1340c614087460f84 
>   src/launcher/windows/executor.cpp f6da398eeddaf68b8eaf510648c55964d9f5b7c7 
> 
> Diff: https://reviews.apache.org/r/49415/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49415: Removed the argv parameter in command executor helper.

[Gilbert Song]

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


Ship it!




Do you miss `windows/executor.hpp`?

- Gilbert Song


On June 29, 2016, 5:54 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49415/
> ---
> 
> (Updated June 29, 2016, 5:54 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's confusing to pass in 'argv' because it's already contained in
> 'command'. This patch removed this parameter.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp bb88a4570d183d22dff65f6e220d566c0924001a 
>   src/launcher/posix/executor.hpp e5f3c0a8e2d3687a5330adbf2f667db9106970c7 
>   src/launcher/posix/executor.cpp ab1dd938a7c59f52b44aafe1340c614087460f84 
>   src/launcher/windows/executor.cpp f6da398eeddaf68b8eaf510648c55964d9f5b7c7 
> 
> Diff: https://reviews.apache.org/r/49415/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49523: Moved MESOS_CONTAINERIZER to a separate constants file.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On July 1, 2016, 6:02 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49523/
> ---
> 
> (Updated July 1, 2016, 6:02 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Ian Downes, and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5753
> https://issues.apache.org/jira/browse/MESOS-5753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved MESOS_CONTAINERIZER to a separate constants file.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am bdad9c2ae07585b53aac97341550f3ea0b852ae7 
>   src/slave/containerizer/mesos/constants.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 8e347735fad2301a2bcbc7d141efbf0f2b708435 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 63cf92217054fab43c843379c86e25ce7f07c7d9 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 1c50cf545392488cf1e2d51d8e03887bebef5e75 
> 
> Diff: https://reviews.apache.org/r/49523/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49524: Fixed a const ref issue in command executor.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On July 1, 2016, 6:02 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49524/
> ---
> 
> (Updated July 1, 2016, 6:02 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Ian Downes, and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5753
> https://issues.apache.org/jira/browse/MESOS-5753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a const ref issue in command executor.
> 
> 
> Diffs
> -
> 
>   src/launcher/posix/executor.hpp e5f3c0a8e2d3687a5330adbf2f667db9106970c7 
>   src/launcher/posix/executor.cpp ab1dd938a7c59f52b44aafe1340c614087460f84 
> 
> Diff: https://reviews.apache.org/r/49524/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49530: Added 'nvidia-uvm-tools' as optional control device for Nvidia GPUs.

[Benjamin Mahler]

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


Ship it!





src/slave/containerizer/mesos/isolators/gpu/isolator.hpp (line 133)


Need a little comment here that the key is a path, or perhaps just store 
Path from stout?

```
const map controlDeviceEntries;
```


- Benjamin Mahler


On July 1, 2016, 8:17 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49530/
> ---
> 
> (Updated July 1, 2016, 8:17 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5764
> https://issues.apache.org/jira/browse/MESOS-5764
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As part of this, we also generalized the method for enumerating the
> GPU control devices which should be accessible inside a container.
> Now it should be easier to add new control devices as they appear in
> the future.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 
> a6734105dcb3efadfceb7cdd357b749813a5bf40 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
> 2bf7fe2d9bff1afb1cb8ba5c401246e4579d8889 
> 
> Diff: https://reviews.apache.org/r/49530/diff/
> 
> 
> Testing
> ---
> 
> sudo GTEST_FILTER="*NVIDIA*" bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49524: Fixed a const ref issue in command executor.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On July 1, 2016, 11:02 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49524/
> ---
> 
> (Updated July 1, 2016, 11:02 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Ian Downes, and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5753
> https://issues.apache.org/jira/browse/MESOS-5753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a const ref issue in command executor.
> 
> 
> Diffs
> -
> 
>   src/launcher/posix/executor.hpp e5f3c0a8e2d3687a5330adbf2f667db9106970c7 
>   src/launcher/posix/executor.cpp ab1dd938a7c59f52b44aafe1340c614087460f84 
> 
> Diff: https://reviews.apache.org/r/49524/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49469: Added new utility function, `frameworkHasCapability()`.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 1, 2016, 10:59 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49469/
> ---
> 
> (Updated July 1, 2016, 10:59 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace various hand-coded loops checking for capabilities with calls to
> this new function instead.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 3dd03da3fea10a01aa4508f53c5d1ebcc3d6a2a4 
>   src/common/protobuf_utils.cpp 040bdf82134289f0caf63e11c2ce8e7853a392b3 
>   src/docker/executor.cpp 88b7fc4c36ed3974ac6b103a29e1d975619f0c69 
>   src/launcher/executor.cpp bb88a4570d183d22dff65f6e220d566c0924001a 
>   src/master/allocator/mesos/hierarchical.cpp 
> 38381237fa6ceb3f21fd0d4b07d7c3787f0129df 
> 
> Diff: https://reviews.apache.org/r/49469/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49523: Moved MESOS_CONTAINERIZER to a separate constants file.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49415, 49424, 49425, 49472, 49473, 49479, 49524, 49523]

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

- Mesos ReviewBot


On July 1, 2016, 6:02 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49523/
> ---
> 
> (Updated July 1, 2016, 6:02 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Ian Downes, and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5753
> https://issues.apache.org/jira/browse/MESOS-5753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved MESOS_CONTAINERIZER to a separate constants file.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am bdad9c2ae07585b53aac97341550f3ea0b852ae7 
>   src/slave/containerizer/mesos/constants.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 8e347735fad2301a2bcbc7d141efbf0f2b708435 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 63cf92217054fab43c843379c86e25ce7f07c7d9 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 1c50cf545392488cf1e2d51d8e03887bebef5e75 
> 
> Diff: https://reviews.apache.org/r/49523/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49446: Implemented LIST_FILES Call in v1 master API.

[Abhishek Dasgupta]

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

(Updated July 1, 2016, 8:20 p.m.)


Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and Vinod 
Kone.


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


Repository: mesos


Description
---

Implemented LIST_FILES Call in v1 master API.


Diffs (updated)
-

  include/mesos/master/master.proto 2e5d6eeb3a960e4f41c382d65321f18bb05ed6be 
  include/mesos/v1/master/master.proto 93157d57dcc53b54fed2ebbc4772c689ddba2119 
  src/master/http.cpp e5acdb8e0bbcd7a2b7e8a8bc7f4bbeaae2c4fea1 
  src/master/master.hpp e2ab2110fe5a287ab16ac9ef4222fed633e02ebe 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 49446: Implemented LIST_FILES Call in v1 master API.

[Abhishek Dasgupta]

> On June 30, 2016, 10:21 p.m., Anand Mazumdar wrote:
> > src/master/http.cpp, line 2813
> > 
> >
> > Can we have an explicit defensive check here to ensure `result` does 
> > not have error set?
> > 
> > Also, applies to the prior review for `/files/browse`.

Didn't quite get it. We are already checking result.isError() .


- Abhishek


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


On June 30, 2016, 2:06 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49446/
> ---
> 
> (Updated June 30, 2016, 2:06 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5487
> https://issues.apache.org/jira/browse/MESOS-5487
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented LIST_FILES Call in v1 master API.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 2e5d6eeb3a960e4f41c382d65321f18bb05ed6be 
>   include/mesos/v1/master/master.proto 
> 93157d57dcc53b54fed2ebbc4772c689ddba2119 
>   src/master/http.cpp e5acdb8e0bbcd7a2b7e8a8bc7f4bbeaae2c4fea1 
>   src/master/master.hpp e2ab2110fe5a287ab16ac9ef4222fed633e02ebe 
> 
> Diff: https://reviews.apache.org/r/49446/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Review Request 49530: Added 'nvidia-uvm-tools' as optional control device for Nvidia GPUs.

[Kevin Klues]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

As part of this, we also generalized the method for enumerating the
GPU control devices which should be accessible inside a container.
Now it should be easier to add new control devices as they appear in
the future.


Diffs
-

  src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 
a6734105dcb3efadfceb7cdd357b749813a5bf40 
  src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
2bf7fe2d9bff1afb1cb8ba5c401246e4579d8889 

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


Testing
---

sudo GTEST_FILTER="*NVIDIA*" bin/mesos-tests.sh


Thanks,

Kevin Klues

Re: Review Request 49447: Implemented LIST_FILES Call in v1 agent API.

[Abhishek Dasgupta]

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

(Updated July 1, 2016, 8:13 p.m.)


Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and Vinod 
Kone.


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


Repository: mesos


Description
---

Implemented LIST_FILES Call in v1 agent API.


Diffs (updated)
-

  include/mesos/agent/agent.proto 538d12f71df1943f91bafb99650625aa910affaa 
  include/mesos/v1/agent/agent.proto 48f15173fe62b9ce7f648f6b54d74ec62f797c55 
  src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
  src/slave/slave.hpp 2afd7d152dcd2f5390014cd7bd4e926b62c292d1 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 49445: Updated FilesProcess to support List_Files Call in Operator API v1.

[Abhishek Dasgupta]

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

(Updated July 1, 2016, 8:07 p.m.)


Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and Vinod 
Kone.


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


Repository: mesos


Description
---

Updated FilesProcess to support List_Files Call in Operator API v1.


Diffs (updated)
-

  src/files/files.hpp b767d5bc5bee16e3bd98199773a6bc7d30c1c32d 
  src/files/files.cpp a5a1b86e14f63e5e3834a2900270252fbe16f586 
  src/tests/files_tests.cpp 31337e280c6224a8c949c8868a53e5a785b4573f 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 49445: Updated FilesProcess to support List_Files Call in Operator API v1.

[Abhishek Dasgupta]

> On June 30, 2016, 9:45 p.m., Anand Mazumdar wrote:
> > src/files/files.cpp, lines 104-106
> > 
> >
> > See my earlier comments in the header file about this function and then 
> > modify this.

If changed to browse(), obviously this compliler error is coming :

../src/files/files.cpp:223:71: error: no matching function for call to 
‘bind(, mesos::internal::FilesProcess*, 
const std::_Placeholder<1>&, None)’
   lambda::bind(::browse, this, lambda::_1, None()));


- Abhishek


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


On June 30, 2016, 2:06 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49445/
> ---
> 
> (Updated June 30, 2016, 2:06 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5487
> https://issues.apache.org/jira/browse/MESOS-5487
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated FilesProcess to support List_Files Call in Operator API v1.
> 
> 
> Diffs
> -
> 
>   src/files/files.hpp b767d5bc5bee16e3bd98199773a6bc7d30c1c32d 
>   src/files/files.cpp a5a1b86e14f63e5e3834a2900270252fbe16f586 
>   src/tests/files_tests.cpp 31337e280c6224a8c949c8868a53e5a785b4573f 
> 
> Diff: https://reviews.apache.org/r/49445/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49445: Updated FilesProcess to support List_Files Call in Operator API v1.

[Abhishek Dasgupta]

> On June 30, 2016, 9:45 p.m., Anand Mazumdar wrote:
> > src/files/files.hpp, lines 103-105
> > 
> >
> > hmm, wondering why is the return type a `std::map`. Why can't it be 
> > just a `std::list` of `FileInfo` with an explicit comment that it is sorted 
> > on path?

Errr... If not std::map, we can't gurantee it is sorted based on it's key i.e 
path. Can you be little elaborate if intended any other change in this case?


- Abhishek


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


On June 30, 2016, 2:06 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49445/
> ---
> 
> (Updated June 30, 2016, 2:06 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5487
> https://issues.apache.org/jira/browse/MESOS-5487
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated FilesProcess to support List_Files Call in Operator API v1.
> 
> 
> Diffs
> -
> 
>   src/files/files.hpp b767d5bc5bee16e3bd98199773a6bc7d30c1c32d 
>   src/files/files.cpp a5a1b86e14f63e5e3834a2900270252fbe16f586 
>   src/tests/files_tests.cpp 31337e280c6224a8c949c8868a53e5a785b4573f 
> 
> Diff: https://reviews.apache.org/r/49445/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49444: Added a helper function to construct JSON:Object from FileInfo protobuf.

[Abhishek Dasgupta]

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

(Updated July 1, 2016, 7:53 p.m.)


Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and Vinod 
Kone.


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


Repository: mesos


Description
---

Added a helper function to construct JSON:Object from FileInfo protobuf.


Diffs (updated)
-

  src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
  src/common/http.cpp daf5672287bb6633f468c88632a561f5a01590df 

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


Testing
---


Thanks,

Abhishek Dasgupta

Review Request 49529: Remove jsonFileInfo implementation from files.hpp.

[Abhishek Dasgupta]

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

Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and Vinod 
Kone.


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


Repository: mesos


Description
---

Remove jsonFileInfo implementation from files.hpp.


Diffs
-

  src/files/files.hpp b767d5bc5bee16e3bd98199773a6bc7d30c1c32d 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 49443: Include a function to construct FileInfo protobuf message.

[Abhishek Dasgupta]

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

(Updated July 1, 2016, 7:49 p.m.)


Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and Vinod 
Kone.


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


Repository: mesos


Description
---

Include a function to construct FileInfo protobuf message.


Diffs (updated)
-

  src/common/protobuf_utils.hpp 3dd03da3fea10a01aa4508f53c5d1ebcc3d6a2a4 
  src/common/protobuf_utils.cpp 040bdf82134289f0caf63e11c2ce8e7853a392b3 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 49518: Initial snapshot for v1 master event stream.

[Vinod Kone]

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




include/mesos/master/master.proto (line 446)


// Snapshot of the entire cluster state. This is the first event
// that is sent after a client is subscribed. Further updates to
// the cluster state are sent as separate events on the stream.



include/mesos/v1/master/master.proto (lines 447 - 449)


see above.



src/master/http.cpp (line 595)


just capture `http`?



src/master/http.cpp (lines 597 - 598)


what do you mean by "before subscribe"?

// Send snapshot of the current state before any other events.


- Vinod Kone


On July 1, 2016, 4:12 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49518/
> ---
> 
> (Updated July 1, 2016, 4:12 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5498
> https://issues.apache.org/jira/browse/MESOS-5498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initial snapshot for v1 master event stream.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto e34414ba9f4b350254396ca1d6391f7cc5ef451f 
>   include/mesos/v1/master/master.proto 
> f6155dfacfbd98efaf72b337b73cc950c0f3b7c3 
>   src/master/http.cpp 528f01f2e00ddbd15da1cc4fca27b5347c9fc798 
>   src/tests/api_tests.cpp a7f075172f870bcd3ac7fb9c7a69b70ac5cb73c5 
> 
> Diff: https://reviews.apache.org/r/49518/diff/
> 
> 
> Testing
> ---
> 
> Updated test and make check.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49517: Implement GetState V1 master API.

[Vinod Kone]

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




src/master/http.cpp (line 1428)


fix this in the previous review.



src/master/http.cpp (lines 1536 - 1537)


ditto. see earlier review for comments.



src/master/http.cpp (lines 1565 - 1569)


this should be moved to GetLeadingMaster



src/master/http.cpp (line 1571)


s/instead/instead of/



src/tests/api_tests.cpp (lines 430 - 433)


mvoe this to #465



src/tests/api_tests.cpp (lines 533 - 534)


move this to #549.



src/tests/api_tests.cpp (line 537)


copy paste error? update the comment.



src/tests/api_tests.cpp (line 546)


a task goes to completed after the ACK for terminal state is processed by 
the master. you need to setup an expectation on StatusUpdateAcknowledgement 
message to guarantee that. See `GetTasks` test for an example.


- Vinod Kone


On July 1, 2016, 4:09 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49517/
> ---
> 
> (Updated July 1, 2016, 4:09 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The help function will also be used for snapshot of
> event stream.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 528f01f2e00ddbd15da1cc4fca27b5347c9fc798 
>   src/master/master.hpp 7388a3e7f3a0c9f0a2a286a8c8107210971b654a 
>   src/tests/api_tests.cpp a7f075172f870bcd3ac7fb9c7a69b70ac5cb73c5 
> 
> Diff: https://reviews.apache.org/r/49517/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49516: Refactor Master::Http::getExecutors into helper function.

[Vinod Kone]

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




src/master/http.cpp (line 1428)


s/getExecutors/_getExecutors/


- Vinod Kone


On July 1, 2016, 4:08 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49516/
> ---
> 
> (Updated July 1, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Helper function will be reused by `GetExecutors` and `GetState`.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 528f01f2e00ddbd15da1cc4fca27b5347c9fc798 
>   src/master/master.hpp 7388a3e7f3a0c9f0a2a286a8c8107210971b654a 
> 
> Diff: https://reviews.apache.org/r/49516/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49509: Revised protobuf definition of GetState response.

[Vinod Kone]

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




include/mesos/master/master.proto (lines 273 - 274)


move this to GetLeadingMaster.



include/mesos/v1/master/master.proto (lines 274 - 275)


ditto. move this to GetLeadingMaster


- Vinod Kone


On July 1, 2016, 4:07 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49509/
> ---
> 
> (Updated July 1, 2016, 4:07 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Revised protobuf definition of GetState response.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto e34414ba9f4b350254396ca1d6391f7cc5ef451f 
>   include/mesos/v1/master/master.proto 
> f6155dfacfbd98efaf72b337b73cc950c0f3b7c3 
> 
> Diff: https://reviews.apache.org/r/49509/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49489: Refactor master::Http::getFrameworks to helper function.

[Vinod Kone]

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




src/master/http.cpp (lines 1336 - 1339)


ditto. see earlier review.

s/getFrameworksResp/getFrameworks/


- Vinod Kone


On July 1, 2016, 1:37 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49489/
> ---
> 
> (Updated July 1, 2016, 1:37 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new helper function will be reused by `GET_FRAMEWORKS` and
> `GET_STATE` calls.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 528f01f2e00ddbd15da1cc4fca27b5347c9fc798 
>   src/master/master.hpp 7388a3e7f3a0c9f0a2a286a8c8107210971b654a 
> 
> Diff: https://reviews.apache.org/r/49489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49488: Refactor master::Http::getAgents into helper function.

[Vinod Kone]

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




src/master/http.cpp (lines 1964 - 1967)


ditto. see comments in earlier review.



src/master/http.cpp (line 1966)


s/getAgentsResp/getAgents/



src/master/http.cpp (line 1983)


space after "="


- Vinod Kone


On July 1, 2016, 2:47 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49488/
> ---
> 
> (Updated July 1, 2016, 2:47 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new helper function will be reused by both `GET_AGENTS`
> and `GET_STATE` calls.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 528f01f2e00ddbd15da1cc4fca27b5347c9fc798 
>   src/master/master.hpp 7388a3e7f3a0c9f0a2a286a8c8107210971b654a 
> 
> Diff: https://reviews.apache.org/r/49488/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49487: Refactor master::Http::getTasks into helper function.

[Vinod Kone]

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




src/master/http.cpp (lines 3335 - 3336)


no need to defer, since this is a static lambda. also, just capture 
contentType specifically instead of "=".

```
return _getTasks(principal)
  .then([contentType](const mesos::master::Response::GetTasks& getTasks)
-> Future {
  
  });

```



src/master/http.cpp (line 3345)


extra blank line.



src/master/http.cpp (line 3397)


new line before this.


- Vinod Kone


On July 1, 2016, 1:32 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49487/
> ---
> 
> (Updated July 1, 2016, 1:32 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Helper function will be also be reused for `GetState`.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 528f01f2e00ddbd15da1cc4fca27b5347c9fc798 
>   src/master/master.hpp 7388a3e7f3a0c9f0a2a286a8c8107210971b654a 
> 
> Diff: https://reviews.apache.org/r/49487/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49518: Initial snapshot for v1 master event stream.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [49518, 49517, 49516, 49509, 49489, 49488, 49487, 49465, 49464]

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

Error:
2016-07-01 19:12:37 URL:https://reviews.apache.org/r/49489/diff/raw/ 
[3520/3520] -> "49489.patch" [1]
error: patch failed: src/master/master.hpp:1483
error: src/master/master.hpp: patch does not apply

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

- Mesos ReviewBot


On July 1, 2016, 4:12 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49518/
> ---
> 
> (Updated July 1, 2016, 4:12 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5498
> https://issues.apache.org/jira/browse/MESOS-5498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initial snapshot for v1 master event stream.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto e34414ba9f4b350254396ca1d6391f7cc5ef451f 
>   include/mesos/v1/master/master.proto 
> f6155dfacfbd98efaf72b337b73cc950c0f3b7c3 
>   src/master/http.cpp 528f01f2e00ddbd15da1cc4fca27b5347c9fc798 
>   src/tests/api_tests.cpp a7f075172f870bcd3ac7fb9c7a69b70ac5cb73c5 
> 
> Diff: https://reviews.apache.org/r/49518/diff/
> 
> 
> Testing
> ---
> 
> Updated test and make check.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49500: Moved TODO to correct place in master/http.cpp.

[Vinod Kone]

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




src/master/http.cpp (lines 3102 - 3139)


can you rebase. the logic of `tasks()` and `getTasks()` has been changed on 
trunk.


- Vinod Kone


On July 1, 2016, 9:36 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49500/
> ---
> 
> (Updated July 1, 2016, 9:36 a.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved TODO to correct place in master/http.cpp.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 670b4e0e4424400d138fdf4ca1493520c90b6448 
> 
> Diff: https://reviews.apache.org/r/49500/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>