Re: Review Request 44753: Fixed runtime isolator tests out of disk issue.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44753]

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

- Mesos ReviewBot


On March 12, 2016, 4:15 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44753/
> ---
> 
> (Updated March 12, 2016, 4:15 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed runtime isolator tests out of disk issue.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 9fd5ea9e465f21624d3cdc0125a702528fe7548b 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> d8a000413fb30d7daaa77827287a2395d81b8b04 
> 
> Diff: https://reviews.apache.org/r/44753/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-test.sh
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 42590: Renamed reserved() to reservations().

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [42590]

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

Error:
2016-03-12 04:33:44 URL:https://reviews.apache.org/r/42590/diff/raw/ 
[4988/4988] -> "42590.patch" [1]
error: patch failed: include/mesos/resources.hpp:222
error: include/mesos/resources.hpp: patch does not apply
error: patch failed: include/mesos/v1/resources.hpp:222
error: include/mesos/v1/resources.hpp: patch does not apply
error: patch failed: src/common/resources.cpp:859
error: src/common/resources.cpp: patch does not apply
error: patch failed: src/master/http.cpp:168
error: src/master/http.cpp: patch does not apply
error: patch failed: src/tests/resources_tests.cpp:1460
error: src/tests/resources_tests.cpp: patch does not apply
error: patch failed: src/v1/resources.cpp:862
error: src/v1/resources.cpp: patch does not apply

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

- Mesos ReviewBot


On March 12, 2016, 2:47 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42590/
> ---
> 
> (Updated March 12, 2016, 2:47 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, and Klaus Ma.
> 
> 
> Bugs: MESOS-4447
> https://issues.apache.org/jira/browse/MESOS-4447
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed reserved() to reservations().
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/master/http.cpp 893e0651b89de4df1813f8f4f365eab80a7b3bc2 
>   src/tests/resources_tests.cpp 5357275833383bbff055689c8c1d0fa59791f2d0 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/42590/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> ./bin/mesos-tests.sh  --gtest_filter="ResourcesTest.*"
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 44753: Fixed runtime isolator tests out of disk issue.

[Gilbert Song]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Fixed runtime isolator tests out of disk issue.


Diffs
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
9fd5ea9e465f21624d3cdc0125a702528fe7548b 
  src/tests/containerizer/runtime_isolator_tests.cpp 
d8a000413fb30d7daaa77827287a2395d81b8b04 

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


Testing
---

make check

sudo ./bin/mesos-test.sh


Thanks,

Gilbert Song

Re: Review Request 44749: Separate XFS utilities.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44342, 44749]

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

- Mesos ReviewBot


On March 12, 2016, 1:30 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44749/
> ---
> 
> (Updated March 12, 2016, 1:30 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Move the low level XFS utility helpers into a separate API so that we
> can write tests for them.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 70c32f0672e4525bc3a8e67a2aba9c874854bdc7 
>   src/linux/xfs.hpp PRE-CREATION 
>   src/linux/xfs.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44749/diff/
> 
> 
> Testing
> ---
> 
> make check on Fedora 23.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44748: Stout: Added implementation of `read` that works on Windows.

[Daniel Pravat]

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


Ship it!




Ship It!

- Daniel Pravat


On March 12, 2016, 1:28 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44748/
> ---
> 
> (Updated March 12, 2016, 1:28 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3641
> https://issues.apache.org/jira/browse/MESOS-3641
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Added implementation of `read` that works on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/read.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp 
> d494cbf8a2a24c88b0569634cfcbf29de0784797 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/read.hpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44748/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 42590: Renamed reserved() to reservations().

[Klaus Ma]

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




include/mesos/resources.hpp (line 228)


I think we need to update document about the `Option role`


- Klaus Ma


On March 12, 2016, 10:47 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42590/
> ---
> 
> (Updated March 12, 2016, 10:47 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, and Klaus Ma.
> 
> 
> Bugs: MESOS-4447
> https://issues.apache.org/jira/browse/MESOS-4447
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed reserved() to reservations().
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/master/http.cpp 893e0651b89de4df1813f8f4f365eab80a7b3bc2 
>   src/tests/resources_tests.cpp 5357275833383bbff055689c8c1d0fa59791f2d0 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/42590/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> ./bin/mesos-tests.sh  --gtest_filter="ResourcesTest.*"
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 42590: Renamed reserved() to reservations().

[Ben Mahler]

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



I left a comment but I'll take care of that for you before committing.


include/mesos/resources.hpp (lines 224 - 229)


Would be great to update the documentation here to reflect the changes.


- Ben Mahler


On March 12, 2016, 2:47 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42590/
> ---
> 
> (Updated March 12, 2016, 2:47 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, and Klaus Ma.
> 
> 
> Bugs: MESOS-4447
> https://issues.apache.org/jira/browse/MESOS-4447
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed reserved() to reservations().
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/master/http.cpp 893e0651b89de4df1813f8f4f365eab80a7b3bc2 
>   src/tests/resources_tests.cpp 5357275833383bbff055689c8c1d0fa59791f2d0 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/42590/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> ./bin/mesos-tests.sh  --gtest_filter="ResourcesTest.*"
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 42590: Renamed reserved() to reservations().

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On March 12, 2016, 2:47 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42590/
> ---
> 
> (Updated March 12, 2016, 2:47 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, and Klaus Ma.
> 
> 
> Bugs: MESOS-4447
> https://issues.apache.org/jira/browse/MESOS-4447
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed reserved() to reservations().
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/master/http.cpp 893e0651b89de4df1813f8f4f365eab80a7b3bc2 
>   src/tests/resources_tests.cpp 5357275833383bbff055689c8c1d0fa59791f2d0 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/42590/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> ./bin/mesos-tests.sh  --gtest_filter="ResourcesTest.*"
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 44090: Windows: Added a cast for `get/setsockopt` parameters.

[Daniel Pravat]

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

(Updated March 12, 2016, 2:54 a.m.)


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


Summary (updated)
-

Windows: Added a cast for `get/setsockopt` parameters.


Repository: mesos


Description (updated)
---

Windows: Added a cast for `get/setsockopt` parameters.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent.cpp c4a8da8a70b97dd575b1256179c4f43742131a1e 
  3rdparty/libprocess/src/poll_socket.cpp 
6e6634b4b352e3723096521843546cf56ec6dd8b 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 42590: Renamed reserved() to reservations().

[Guangya Liu]

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

(Updated 三月 12, 2016, 2:47 a.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 
Joseph Wu, and Klaus Ma.


Summary (updated)
-

Renamed reserved() to reservations().


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


Repository: mesos


Description (updated)
---

Renamed reserved() to reservations().


Diffs (updated)
-

  include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
  include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
  src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
  src/master/http.cpp 893e0651b89de4df1813f8f4f365eab80a7b3bc2 
  src/tests/resources_tests.cpp 5357275833383bbff055689c8c1d0fa59791f2d0 
  src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 

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


Testing
---

make
make check
./bin/mesos-tests.sh  --gtest_filter="ResourcesTest.*"


Thanks,

Guangya Liu

Re: Review Request 41772: Added helper function to flatten resources.

[Guangya Liu]

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

(Updated 三月 12, 2016, 2:46 a.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 
Joseph Wu, and Klaus Ma.


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


Repository: mesos


Description
---

Added helper function to flatten resources.


Diffs
-

  include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
  include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
  src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
  src/tests/resources_tests.cpp 5357275833383bbff055689c8c1d0fa59791f2d0 
  src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 

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


Testing
---

make
make check

GLOG_v=2 ./bin/mesos-tests.sh  --gtest_filter="ResourcesOperationTest.*" 
--verbose


Thanks,

Guangya Liu

Re: Review Request 44743: Improved grammar in `--help` output for master and agent.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44742, 44743]

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

- Mesos ReviewBot


On March 12, 2016, 12:48 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44743/
> ---
> 
> (Updated March 12, 2016, 12:48 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved grammar in `--help` output for master and agent.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md f6e84023b90e560594429826ed7163310d62b265 
>   src/master/main.cpp 7c1656bcc266f6c94cb4befad37fa813a218b2fa 
>   src/slave/flags.cpp eb470154f30634b3db439be1c122ff93d3147afe 
>   src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 
> 
> Diff: https://reviews.apache.org/r/44743/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44721: Avoided external linkage for sched constants.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On March 11, 2016, 6:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44721/
> ---
> 
> (Updated March 11, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoided external linkage for sched constants.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e9f7c3a447aad79ab19d53879888a413a587408f 
>   src/Makefile.am b24f0f58fa188c16770fe6a3c23ec06262cb0955 
>   src/sched/constants.hpp 523c6d94571ae22b1aa8fe786f5c9f965039ea7a 
>   src/sched/constants.cpp c00b8ba31d5f24f99e99a9ec7034b641929246c0 
>   src/sched/flags.hpp 57dbab8ad7f8d4829d15627caf0e69ef91702081 
> 
> Diff: https://reviews.apache.org/r/44721/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44720: Replaced `const string` slave constants with `constexpr char[]`.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On March 11, 2016, 6:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44720/
> ---
> 
> (Updated March 11, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per style guide.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/flags.cpp eb470154f30634b3db439be1c122ff93d3147afe 
> 
> Diff: https://reviews.apache.org/r/44720/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44719: Avoided external linkage for slave constants.

[Ben Mahler]

> On March 12, 2016, 2:28 a.m., Ben Mahler wrote:
> >

I'll make sure the commit description provides the similar motivation as was 
provided in https://reviews.apache.org/r/44191/ (although referring to the 
latest project style guidelines rather than performance).


- Ben


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


On March 11, 2016, 6:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44719/
> ---
> 
> (Updated March 11, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoided external linkage for slave constants.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
> 
> Diff: https://reviews.apache.org/r/44719/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44719: Avoided external linkage for slave constants.

[Ben Mahler]

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


Ship it!





src/slave/constants.hpp (lines 110 - 112)


Can this be in-line or is there a circular dependency issue that warrants 
the .cpp file?


- Ben Mahler


On March 11, 2016, 6:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44719/
> ---
> 
> (Updated March 11, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoided external linkage for slave constants.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
> 
> Diff: https://reviews.apache.org/r/44719/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44718: Replaced `const string` master constants with `constexpr char[]`.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On March 11, 2016, 6:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44718/
> ---
> 
> (Updated March 11, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per style guide.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 2c3299bc1aaa0888f7e47a71965c56ada8ecc21f 
>   src/master/flags.cpp c1dd127109f1ba96a8f9b95f3eb99dfeb43f7d28 
>   src/tests/flags.hpp 542f462453e41cad3b4cb8174ac59f058e625b19 
> 
> Diff: https://reviews.apache.org/r/44718/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44191: Avoided external linkage for master constants.

[Ben Mahler]

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


Fix it, then Ship it!




We may want to update the description to say that the constexpr change is to 
bring it in-line with the latest style guidelines for the project. Initially 
when I read performance I thought we were trying to optimize something without 
a basis. :)


src/master/constants.hpp (lines 120 - 121)


Any reason this Duration constant wasn't changed to constexpr?


- Ben Mahler


On March 11, 2016, 6:31 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44191/
> ---
> 
> (Updated March 11, 2016, 6:31 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4790
> https://issues.apache.org/jira/browse/MESOS-4790
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We were only using external linkage to workaround bugs in ancient
> (4.1.x) of GCC. We can also mark most of these constants `constexpr`,
> which should be both slightly more efficient (in theory) and
> safer (see MESOS-1023).
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e9f7c3a447aad79ab19d53879888a413a587408f 
>   src/Makefile.am b24f0f58fa188c16770fe6a3c23ec06262cb0955 
>   src/master/constants.hpp 2c3299bc1aaa0888f7e47a71965c56ada8ecc21f 
>   src/master/constants.cpp e316f9772d880b694faeee6d001dc56bc088c118 
> 
> Diff: https://reviews.apache.org/r/44191/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Note that I left a few string constants as `const std::string`; you might 
> arguably want them to be `constexpr char[]` (per Google Style Guide).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44255: Add metrics for {RESERVE, UNRESERVE, CREATE, DESTROY} offer operation.

[Greg Mann]

On March 12, 2016, 2:11 a.m., fan du wrote:
> > In a separate patch, could you add documentation for these metrics to 
> > docs/monitoring.md?

Whoops, sorry! I see that you already have the documentation :-)


- Greg


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


On March 10, 2016, 2:42 a.m., fan du wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44255/
> ---
> 
> (Updated March 10, 2016, 2:42 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Guangya Liu, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4492
> https://issues.apache.org/jira/browse/MESOS-4492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add metrics for {RESERVE, UNRESERVE, CREATE, DESTROY} offer operation.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 323d01d99456a71bd384faf186264e3fc4bf2207 
>   src/master/http.cpp 950206baf7f3a1cccdc49d810126473966d8d021 
>   src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 
>   src/master/metrics.hpp 9d201fcce1c46a890c86a889ab31029f9a061561 
>   src/master/metrics.cpp 30c091198a8fdd6d6a957a351dc37d3dae7788e4 
>   src/tests/metrics_tests.cpp 419d275e0b32817388120222bd433ee6f4835efd 
> 
> Diff: https://reviews.apache.org/r/44255/diff/
> 
> 
> Testing
> ---
> 
> ChangLog:
> 
> v3:
>   - Move the couting out of common code path to http endpoint and master 
> accept call separately to reflect its logic.
> 
> v2:
>   - Documenting those metrics
>   - Add test code for MetricsTest as suggested by Guangya
>   - post-review.py does not update original 
> RR(https://reviews.apache.org/r/44058/), but only create a new one even if I 
> rebased.
> 
> 
> Tests:
> 1. make check GTEST_FILTER="MetricsTest.Master" on Centos-7 
> (3.10.0-123.el7.x86_640)
> 
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from MetricsTest
> [ RUN  ] MetricsTest.Master
> [   OK ] MetricsTest.Master (211 ms)
> [--] 1 test from MetricsTest (211 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (244 ms total)
> [  PASSED  ] 1 test
> 
> 2. Verify its functionality with 'reserve' http endpoint as an test case
> 
> # curl http://ipdc02-kvm-guest2:5050/metrics/snapshot | python -mjson.tool | 
> grep reserve
> "master/messages_reserve_resource": 0.0,
> "master/messages_unreserve_resource": 0.0,
> 
> 
> # curl -i -d slaveId=6250553a-2f39-4a92-9073-4618d130f433-S1  -d resources='[ 
> { "name": "cpus", "type": "SCALAR","scalar": { "value": 1 
> },"reservation":{"principal": "XiaoHaHa"}}  ]' -X POST  
> ipdc02-kvm-guest2:5050/master/reserve
> HTTP/1.1 200 OK
> Date: Fri, 26 Feb 2016 19:59:01 GMT
> Content-Length: 0
> 
> # curl http://ipdc02-kvm-guest2:5050/metrics/snapshot  | python -mjson.tool | 
> grep reserve
> "master/messages_reserve_resource": 1.0,
> "master/messages_unreserve_resource": 0.0,
> 
> 
> Thanks,
> 
> fan du
> 
>

Re: Review Request 44255: Add metrics for {RESERVE, UNRESERVE, CREATE, DESTROY} offer operation.

[Greg Mann]

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



Thanks for the edits! A few comments below.


src/master/master.cpp (line 3027)


You could move your incrementing statements here to be more consistent with 
the existing code.



src/master/master.cpp (line 3214)


Remove the space after ->



src/master/http.cpp (line 787)


This increment statement occurs after some invalidation logic, and directly 
before the authorization call; is this precisely where we want to be 
incrementing? We end up not counting invalid operations, but counting 
unauthorized operations.

Looking through the other metrics code in master.cpp, it seems we aren't 
entirely consistent with respect to when we increment the `messages_XXX` 
metrics. We increment them both before and after operation validation, for 
example.

In master.cpp, there are some existing `messages_XXX` metrics in the 
`accept()` function which get incremented together; those occur before 
validation and before authorization. To be consistent, you should probably do 
the same here. Currently, the metric gets incremented after `validate()` is 
called.

We should create a separate JIRA for making all of the messages metrics 
consistent with respect to when they get incremented.


In a separate patch, could you add documentation for these metrics to 
docs/monitoring.md?

- Greg Mann


On March 10, 2016, 2:42 a.m., fan du wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44255/
> ---
> 
> (Updated March 10, 2016, 2:42 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Guangya Liu, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4492
> https://issues.apache.org/jira/browse/MESOS-4492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add metrics for {RESERVE, UNRESERVE, CREATE, DESTROY} offer operation.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 323d01d99456a71bd384faf186264e3fc4bf2207 
>   src/master/http.cpp 950206baf7f3a1cccdc49d810126473966d8d021 
>   src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 
>   src/master/metrics.hpp 9d201fcce1c46a890c86a889ab31029f9a061561 
>   src/master/metrics.cpp 30c091198a8fdd6d6a957a351dc37d3dae7788e4 
>   src/tests/metrics_tests.cpp 419d275e0b32817388120222bd433ee6f4835efd 
> 
> Diff: https://reviews.apache.org/r/44255/diff/
> 
> 
> Testing
> ---
> 
> ChangLog:
> 
> v3:
>   - Move the couting out of common code path to http endpoint and master 
> accept call separately to reflect its logic.
> 
> v2:
>   - Documenting those metrics
>   - Add test code for MetricsTest as suggested by Guangya
>   - post-review.py does not update original 
> RR(https://reviews.apache.org/r/44058/), but only create a new one even if I 
> rebased.
> 
> 
> Tests:
> 1. make check GTEST_FILTER="MetricsTest.Master" on Centos-7 
> (3.10.0-123.el7.x86_640)
> 
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from MetricsTest
> [ RUN  ] MetricsTest.Master
> [   OK ] MetricsTest.Master (211 ms)
> [--] 1 test from MetricsTest (211 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (244 ms total)
> [  PASSED  ] 1 test
> 
> 2. Verify its functionality with 'reserve' http endpoint as an test case
> 
> # curl http://ipdc02-kvm-guest2:5050/metrics/snapshot | python -mjson.tool | 
> grep reserve
> "master/messages_reserve_resource": 0.0,
> "master/messages_unreserve_resource": 0.0,
> 
> 
> # curl -i -d slaveId=6250553a-2f39-4a92-9073-4618d130f433-S1  -d resources='[ 
> { "name": "cpus", "type": "SCALAR","scalar": { "value": 1 
> },"reservation":{"principal": "XiaoHaHa"}}  ]' -X POST  
> ipdc02-kvm-guest2:5050/master/reserve
> HTTP/1.1 200 OK
> Date: Fri, 26 Feb 2016 19:59:01 GMT
> Content-Length: 0
> 
> # curl http://ipdc02-kvm-guest2:5050/metrics/snapshot  | python -mjson.tool | 
> grep reserve
> "master/messages_reserve_resource": 1.0,
> "master/messages_unreserve_resource": 0.0,
> 
> 
> Thanks,
> 
> fan du
> 
>

Re: Review Request 44190: Made `Bytes` usable in `constexpr` expressions [stout].

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On March 11, 2016, 6:30 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44190/
> ---
> 
> (Updated March 11, 2016, 6:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4790
> https://issues.apache.org/jira/browse/MESOS-4790
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `Bytes` usable in `constexpr` expressions [stout].
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/bytes.hpp 
> 88a8bd4019fcbdaa5fcde4867cee9a198c83c1c6 
> 
> Diff: https://reviews.apache.org/r/44190/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44717: Marked a few Duration constants `constexpr`.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On March 11, 2016, 6:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44717/
> ---
> 
> (Updated March 11, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Marked a few Duration constants `constexpr`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp 
> ecec70672313666124ee6a951b3db09e9401b9ff 
> 
> Diff: https://reviews.apache.org/r/44717/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44138: Windows:[1/2] Lifted socket API into Stout.

[Daniel Pravat]

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

(Updated March 12, 2016, 2:02 a.m.)


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


Repository: mesos


Description
---

Windows:[1/2] Lifted socket API into Stout.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/socket.hpp 
PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/socket.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/socket.hpp 
PRE-CREATION 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 44747: Stout: Added implementation of `write` that works on Windows.

[Yi Sun]

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


Ship it!




Ship It!

- Yi Sun


On March 12, 2016, 1:28 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44747/
> ---
> 
> (Updated March 12, 2016, 1:28 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3641
> https://issues.apache.org/jira/browse/MESOS-3641
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Added implementation of `write` that works on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/write.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp 
> a2a5d0b7af673fee89f66e2722846af7b7fd059a 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/write.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/write.hpp 
> 1715bf5e6c590e7d14f59d517b30a281346365be 
> 
> Diff: https://reviews.apache.org/r/44747/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 44748: Stout: Added implementation of `read` that works on Windows.

[Yi Sun]

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


Ship it!




Ship It!

- Yi Sun


On March 12, 2016, 1:28 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44748/
> ---
> 
> (Updated March 12, 2016, 1:28 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3641
> https://issues.apache.org/jira/browse/MESOS-3641
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Added implementation of `read` that works on Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/read.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp 
> d494cbf8a2a24c88b0569634cfcbf29de0784797 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/read.hpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44748/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 44342: XFS disk resource isolator.

[James Peach]

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

(Updated March 12, 2016, 1:30 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Track sandbox directory usage by dynamically assigning XFS project
quotas. We track a range of XFS project IDs, assigning a project ID
and a project quota to each sandbox as it is created. When the task
reaches the quota, writes will fail with EDQUOT, and the task will have
an opportunity to handle that.

Quotas are not applied to volume resources since the isolator interface
has no insight into the volume lifecycle. Thus it is not currently
possible to accurately assign and reclaim project IDs.

If LOW is the lower bound of the project ID range and HIGH is the upper
bound, you can show the currently allocated project quotas using the
xfs_quota command:

  $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"

To show the project ID assigned to the file PATH, use the xfs_io command:

  $ xfs_io -r -c stat PATH


Diffs (updated)
-

  configure.ac 8e4f03593df4a8ba13f00292963e351acc3f71c1 
  docs/configuration.md f6e84023b90e560594429826ed7163310d62b265 
  docs/mesos-containerizer.md 15fb5bdbe74e059614b8948108f32cd04b623305 
  src/Makefile.am 70c32f0672e4525bc3a8e67a2aba9c874854bdc7 
  src/slave/containerizer/mesos/containerizer.cpp 
af3ff5750649497d8852b4761c78d4cae5455a02 
  src/slave/containerizer/mesos/isolators/disk/xfs.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/disk/xfs.cpp PRE-CREATION 
  src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
  src/slave/flags.cpp eb470154f30634b3db439be1c122ff93d3147afe 

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


Testing
---

Manual testing on Fedora 23 w/ XFS. Make check on Fedora and OS X.


Thanks,

James Peach

Review Request 44749: Separate XFS utilities.

[James Peach]

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

Review request for mesos, Jie Yu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Move the low level XFS utility helpers into a separate API so that we
can write tests for them.


Diffs
-

  src/Makefile.am 70c32f0672e4525bc3a8e67a2aba9c874854bdc7 
  src/linux/xfs.hpp PRE-CREATION 
  src/linux/xfs.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/disk/xfs.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/disk/xfs.cpp PRE-CREATION 

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


Testing
---

make check on Fedora 23.


Thanks,

James Peach

Review Request 44747: Stout: Added implementation of `write` that works on Windows.

[Alex Clemmer]

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

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


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


Repository: mesos


Description
---

Stout: Added implementation of `write` that works on Windows.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/write.hpp 
PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/stat.hpp 
a2a5d0b7af673fee89f66e2722846af7b7fd059a 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/write.hpp 
PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/write.hpp 
1715bf5e6c590e7d14f59d517b30a281346365be 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 44748: Stout: Added implementation of `read` that works on Windows.

[Alex Clemmer]

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

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


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


Repository: mesos


Description
---

Stout: Added implementation of `read` that works on Windows.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/read.hpp 
PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/read.hpp 
d494cbf8a2a24c88b0569634cfcbf29de0784797 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/read.hpp 
PRE-CREATION 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 44470: Implemented runtime isoaltor default entrypoint test.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43956, 44467, 44469, 44470]

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

- Mesos ReviewBot


On March 12, 2016, 12:16 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44470/
> ---
> 
> (Updated March 12, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4889
> https://issues.apache.org/jira/browse/MESOS-4889
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented runtime isoaltor default entrypoint test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/runtime_isolator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44470/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 44742: Improved documentation for multiple disks.

[Neil Conway]

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

(Updated March 12, 2016, 1:24 a.m.)


Review request for mesos and Joris Van Remoortere.


Changes
---

Various improvements


Repository: mesos


Description
---

Improved documentation for multiple disks.


Diffs (updated)
-

  docs/multiple-disk.md f0fca2517138a3eef41daaba9206d6d3cd3f6f30 

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


Testing (updated)
---

Previewed using site-docker.


Thanks,

Neil Conway

Re: Review Request 44456: Added Appc provisioner integration test.

[Jie Yu]

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


Fix it, then Ship it!





src/tests/containerizer/provisioner_appc_tests.cpp (lines 805 - 808)


Should we do that in LinuxRootfs?



src/tests/containerizer/provisioner_appc_tests.cpp (line 849)


path::join(mntDir, "");



src/tests/containerizer/provisioner_appc_tests.cpp (lines 878 - 898)


This pattern occurs several times. Can we introduce a helper in this file?


- Jie Yu


On March 11, 2016, 5:06 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44456/
> ---
> 
> (Updated March 11, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4818
> https://issues.apache.org/jira/browse/MESOS-4818
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Appc provisioner integration test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 6c8087e17aa8b7139ba12337d5be472b7099e77f 
> 
> Diff: https://reviews.apache.org/r/44456/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 44743: Improved grammar in `--help` output for master and agent.

[Neil Conway]

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

Review request for mesos and Joris Van Remoortere.


Repository: mesos


Description
---

Improved grammar in `--help` output for master and agent.


Diffs
-

  docs/configuration.md f6e84023b90e560594429826ed7163310d62b265 
  src/master/main.cpp 7c1656bcc266f6c94cb4befad37fa813a218b2fa 
  src/slave/flags.cpp eb470154f30634b3db439be1c122ff93d3147afe 
  src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 

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


Testing
---


Thanks,

Neil Conway

Review Request 44742: Improved documentation for multiple disks.

[Neil Conway]

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

Review request for mesos and Joris Van Remoortere.


Repository: mesos


Description
---

Improved documentation for multiple disks.


Diffs
-

  docs/multiple-disk.md f0fca2517138a3eef41daaba9206d6d3cd3f6f30 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 44734: Marked a few internal functions `static`.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos ReviewBot


On March 11, 2016, 10:33 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44734/
> ---
> 
> (Updated March 11, 2016, 10:33 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Marked a few internal functions `static`.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 3e92979567c7bcefa9ab5f32aa7b23000aa9deb8 
> 
> Diff: https://reviews.apache.org/r/44734/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44469: Implemented runtime isolator default cmd test.

[Gilbert Song]

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

(Updated March 11, 2016, 4:16 p.m.)


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


Repository: mesos


Description
---

Implemented runtime isolator default cmd test.


Diffs (updated)
-

  src/tests/containerizer/runtime_isolator_tests.cpp PRE-CREATION 

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


Testing
---

make check

sudo ./bin/mesos-tests.sh


Thanks,

Gilbert Song

Re: Review Request 44470: Implemented runtime isoaltor default entrypoint test.

[Gilbert Song]

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

(Updated March 11, 2016, 4:16 p.m.)


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


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


Repository: mesos


Description
---

Implemented runtime isoaltor default entrypoint test.


Diffs (updated)
-

  src/tests/containerizer/runtime_isolator_tests.cpp PRE-CREATION 

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


Testing
---

make check

sudo ./bin/mesos-tests.sh


Thanks,

Gilbert Song

Re: Review Request 43956: Created base docker image for test suite.

[Gilbert Song]

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

(Updated March 11, 2016, 4:15 p.m.)


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


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


Repository: mesos


Description
---

Created base docker image for test suite.


Diffs (updated)
-

  src/Makefile.am b24f0f58fa188c16770fe6a3c23ec06262cb0955 
  src/tests/containerizer/docker_archive.hpp PRE-CREATION 
  src/tests/containerizer/runtime_isolator_tests.cpp PRE-CREATION 

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


Testing
---

(Ubuntu14.04 + clang-3.6)

make -j24 check GTEST_FILTER="*ROOT_RUNTIME_CreateDockerLocalTar*" 

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


Thanks,

Gilbert Song

Re: Review Request 44467: Implemented local puller shell command test.

[Gilbert Song]

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

(Updated March 11, 2016, 4:15 p.m.)


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


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


Repository: mesos


Description
---

Implemented local puller shell command test.


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
5b685bfd842d0d98e8ea5ec5ddea8d2cd893dd81 

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


Testing
---

make check

sudo ./bin/mesos-tests.sh


Thanks,

Gilbert Song

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

> On March 11, 2016, 8:02 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 85
> > 
> >
> > I would suggest we have a `Info` for each container.
> > 
> > ```
> > struct Info
> > {
> >   ...
> > };
> > ```
> > 
> > Remember that for any field you put into the Info struct, you need to 
> > be able to 'recover' it in recover function. It's also possible that 
> > ExecutorInfo/TaskInfo are not available during recover (orphans due to 
> > wiped meta data). You need to think about how to recover (e.g., the name of 
> > the network) that the container has joined.
> 
> Avinash sridharan wrote:
> We also need to recover the IP address associated with the container. 
> Since this has to be returned as part of `NetworkINfo` . Need to figure out 
> how to recover this?

I am not sure. Why do we need to store/recover IP address? When destroy a 
container, the CNI plugin only needs container ID (which can be the same as our 
container Id), the namespace handle and the networkconfig file. The rest can 
all be fixed.

I think the trick part is: when there is an orphan container (no 
executorinfo/taskinfo), how can we figure out its network namespace handle and 
networconfig file so that we can destroy it (call CNI plugin DEL). We need to 
know the name of the network to get the networkcofig file, and the pid to get 
the network namespace handle.


- Jie


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


On March 10, 2016, 2:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44580: Made changes to the executor library around managing connections.

[Anand Mazumdar]

> On March 11, 2016, 10:44 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 462
> > 
> >
> > This if statement sounds like all combinations of "checkpoint", 
> > "connecting" and "recoveryTimeout" are possible? I don't think that's the 
> > case?
> > 
> > Should this be?
> > 
> > ```
> > if (recoveryTimer.isSome()) {
> >   CHECK(checkpoint);
> >   CHECK_EQ(CONNECTING, state);
> >   
> >   return;
> > }
> > ```

Good catch. Looks much simpler this way.

Since the state changes inside `disconnected()`, I removed the 
`CHECK_EQ(CONNECTING, state)` check.


> On March 11, 2016, 10:44 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 527
> > 
> >
> > kill this?

I moved it to doing the `CHECK` right before we do a `get()` for 
`recoveryTimeout`.


- Anand


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


On March 9, 2016, 6:03 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44580/
> ---
> 
> (Updated March 9, 2016, 6:03 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4858
> https://issues.apache.org/jira/browse/MESOS-4858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change makes the following modifications to the library:
> 
> - Removes passing connection objects to `defer` callbacks as it can
> sometimes lead to deadlocks around destruction in the same execution
> context.
> - Introduced 3 additional states `CONNECTING`, `SUBSCRIBING` and
> `SUBSCRIBED`. The `CONNECTING` state helps us in identifying if a
> connection attempt is in progress while the latter two states allows
> us to drop subscribe calls if one is already is in progress.
> - Creates a random `connectionID` to demarcate a new connection
> instance and  allowing to discard a state connection attempt.
> - Changes around setting the recovery timeout timer only once.
> This allows us to later discard the recoveryTimeout callback
> if we connected with the agent at a later point of time.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp c3e95ea7e4edf78f2a65ddc15e213aba66e69db2 
> 
> Diff: https://reviews.apache.org/r/44580/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 44580: Made changes to the executor library around managing connections.

[Anand Mazumdar]

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

(Updated March 11, 2016, 11:39 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments from Vinod


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


Repository: mesos


Description
---

This change makes the following modifications to the library:

- Removes passing connection objects to `defer` callbacks as it can
sometimes lead to deadlocks around destruction in the same execution
context.
- Introduced 3 additional states `CONNECTING`, `SUBSCRIBING` and
`SUBSCRIBED`. The `CONNECTING` state helps us in identifying if a
connection attempt is in progress while the latter two states allows
us to drop subscribe calls if one is already is in progress.
- Creates a random `connectionID` to demarcate a new connection
instance and  allowing to discard a state connection attempt.
- Changes around setting the recovery timeout timer only once.
This allows us to later discard the recoveryTimeout callback
if we connected with the agent at a later point of time.


Diffs (updated)
-

  src/executor/executor.cpp c3e95ea7e4edf78f2a65ddc15e213aba66e69db2 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 44733: Added fault tolerance tests for the V1 API.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44578, 44579, 44580, 44727, 44728, 44729, 44731, 44733]

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

- Mesos ReviewBot


On March 11, 2016, 10:16 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44733/
> ---
> 
> (Updated March 11, 2016, 10:16 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4629
> https://issues.apache.org/jira/browse/MESOS-4629
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Similar to already existing tests in `fault_tolerance_tests.cpp`,
> this change adds fault tolerance tests for HTTP schedulers/executors.
> Some tests have been omitted since they were no longer valid now due
> to the persistent streaming connection between the master and the
> scheduler in the new API.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8abef3bd3237a6e456faa7884637207dc4d63282 
>   src/tests/http_fault_tolerance_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44733/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 44580: Made changes to the executor library around managing connections.

[Vinod Kone]

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


Fix it, then Ship it!





src/executor/executor.cpp (line 424)


kill this.



src/executor/executor.cpp (line 444)


This if statement sounds like all combinations of "checkpoint", 
"connecting" and "recoveryTimeout" are possible? I don't think that's the case?

Should this be?

```
if (recoveryTimer.isSome()) {
  CHECK(checkpoint);
  CHECK_EQ(CONNECTING, state);
  
  return;
}
```



src/executor/executor.cpp (line 498)


kill this?



src/executor/executor.cpp (lines 500 - 502)


This comment talks about the case when we should return. But the code below 
is when we shouln't return.

How about

```
//...
if (recoveryTimer.isNone() || !recoveryTimer->timeout().expired) {
  return;
}

```


- Vinod Kone


On March 9, 2016, 6:03 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44580/
> ---
> 
> (Updated March 9, 2016, 6:03 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4858
> https://issues.apache.org/jira/browse/MESOS-4858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change makes the following modifications to the library:
> 
> - Removes passing connection objects to `defer` callbacks as it can
> sometimes lead to deadlocks around destruction in the same execution
> context.
> - Introduced 3 additional states `CONNECTING`, `SUBSCRIBING` and
> `SUBSCRIBED`. The `CONNECTING` state helps us in identifying if a
> connection attempt is in progress while the latter two states allows
> us to drop subscribe calls if one is already is in progress.
> - Creates a random `connectionID` to demarcate a new connection
> instance and  allowing to discard a state connection attempt.
> - Changes around setting the recovery timeout timer only once.
> This allows us to later discard the recoveryTimeout callback
> if we connected with the agent at a later point of time.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp c3e95ea7e4edf78f2a65ddc15e213aba66e69db2 
> 
> Diff: https://reviews.apache.org/r/44580/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 44734: Marked a few internal functions `static`.

[Neil Conway]

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

Review request for mesos.


Repository: mesos


Description
---

Marked a few internal functions `static`.


Diffs
-

  src/common/http.cpp 3e92979567c7bcefa9ab5f32aa7b23000aa9deb8 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 43911: Updated `/state` agent endpoint to use jsonify.

[Neil Conway]

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

(Updated March 11, 2016, 10:33 p.m.)


Review request for mesos and Michael Park.


Changes
---

Address code review.


Repository: mesos


Description
---

Updated `/state` agent endpoint to use jsonify.


Diffs (updated)
-

  src/common/http.hpp 61c63a09c428331f7561a9c4c0254bf4e1c8915f 
  src/common/http.cpp 3e92979567c7bcefa9ab5f32aa7b23000aa9deb8 
  src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
  src/tests/common/http_tests.cpp d1bafeb2c5298f9eda8f3927d4ff2ad62b1bb0be 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 43910: Enhanced a test case for the `/state` agent endpoint.

[Neil Conway]

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

(Updated March 11, 2016, 10:11 p.m.)


Review request for mesos and Michael Park.


Changes
---

Rebase.


Repository: mesos


Description
---

Enhanced a test case for the `/state` agent endpoint.


Diffs (updated)
-

  src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 

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


Testing
---

make check

Note that we don't currently check the conversion of `TaskInfo` -> JSON, which 
is used for `queuedTasks`. Would be nice to improve the test case so that the 
slave has a queued task, although this will probably require some 
`DROP_MESSAGE` trickery...


Thanks,

Neil Conway

Review Request 44728: Replaced `.get()` `Option` calls with `->` operator.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

This change replaces `.get()` calls with `->` in the scheduler library.


Diffs
-

  src/scheduler/scheduler.cpp b010a819132fb80810e7f8ce96778109f2e8b35e 

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


Testing
---


Thanks,

Anand Mazumdar

Review Request 44731: Modified `FaultToleranceTest.SchedulerExit` to wait for shutdown.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This change modified the existing test to wait for the shutdown
message instead of making a best-effort `AtMost` check for
the message.


Diffs
-

  src/tests/fault_tolerance_tests.cpp d193897e636efd0e3ef67bf67fcd6255a3de0341 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 44733: Added fault tolerance tests for the V1 API.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Similar to already existing tests in `fault_tolerance_tests.cpp`,
this change adds fault tolerance tests for HTTP schedulers/executors.
Some tests have been omitted since they were no longer valid now due
to the persistent streaming connection between the master and the
scheduler in the new API.


Diffs
-

  src/Makefile.am 8abef3bd3237a6e456faa7884637207dc4d63282 
  src/tests/http_fault_tolerance_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 44727: Made `ExamplesTest.TestHTTPFramework` use the example http executor.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/examples/test_http_framework.cpp 0f8f0b75d68ad9f5da8bd96ca670fad29933a399 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 44729: Close the connection upon framework teardown for HTTP frameworks.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This change fixes a `TODO` and closes the connection upon receieving a
teardown request from the HTTP framework.


Diffs
-

  src/master/master.cpp 5f66a8d2779227232c0c2b736825c8e4828fe5ec 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 44730: Refactored `model` for TaskInfo to common/.

[Neil Conway]

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

Review request for mesos.


Repository: mesos


Description
---

Also added a unit test.


Diffs
-

  src/common/http.hpp 61c63a09c428331f7561a9c4c0254bf4e1c8915f 
  src/common/http.cpp 3e92979567c7bcefa9ab5f32aa7b23000aa9deb8 
  src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
  src/tests/common/http_tests.cpp d1bafeb2c5298f9eda8f3927d4ff2ad62b1bb0be 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 43823: Updated `/tasks` master endpoint to use jsonify.

[Neil Conway]

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

(Updated March 11, 2016, 10:10 p.m.)


Review request for mesos and Michael Park.


Changes
---

Address reviews.


Repository: mesos


Description
---

Updated `/tasks` master endpoint to use jsonify.


Diffs (updated)
-

  src/master/http.cpp 893e0651b89de4df1813f8f4f365eab80a7b3bc2 

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


Testing
---

make check. Also verified that this endpoint is covered by the unit tests.


Thanks,

Neil Conway

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Avinash sridharan]

> On March 11, 2016, 8:02 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 85
> > 
> >
> > I would suggest we have a `Info` for each container.
> > 
> > ```
> > struct Info
> > {
> >   ...
> > };
> > ```
> > 
> > Remember that for any field you put into the Info struct, you need to 
> > be able to 'recover' it in recover function. It's also possible that 
> > ExecutorInfo/TaskInfo are not available during recover (orphans due to 
> > wiped meta data). You need to think about how to recover (e.g., the name of 
> > the network) that the container has joined.

We also need to recover the IP address associated with the container. Since 
this has to be returned as part of `NetworkINfo` . Need to figure out how to 
recover this?


- Avinash


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


On March 10, 2016, 2:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44579: Minor fix to output state during a `CHECK` failure.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 9, 2016, 6:03 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44579/
> ---
> 
> (Updated March 9, 2016, 6:03 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change outputs the state when a check fails inside the scheduler library.
> 
> 
> Diffs
> -
> 
>   src/scheduler/scheduler.cpp b010a819132fb80810e7f8ce96778109f2e8b35e 
> 
> Diff: https://reviews.apache.org/r/44579/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 44578: Pass `received` argument by const ref in the executor library.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 9, 2016, 6:03 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44578/
> ---
> 
> (Updated March 9, 2016, 6:03 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4858
> https://issues.apache.org/jira/browse/MESOS-4858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pass `received` argument by const ref in the executor library.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp c3e95ea7e4edf78f2a65ddc15e213aba66e69db2 
> 
> Diff: https://reviews.apache.org/r/44578/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 42036: Windows: Added `slave/flags.cpp` to Windows build.

[Alex Clemmer]

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

(Updated March 11, 2016, 9:51 p.m.)


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


Repository: mesos


Description
---

Windows: Added `slave/flags.cpp` to Windows build.


Diffs (updated)
-

  src/CMakeLists.txt e9f7c3a447aad79ab19d53879888a413a587408f 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 44287: Added MasterContender and MasterDetector abstract classes.

[Anurag Singh]

> On March 11, 2016, 9:40 p.m., Joseph Wu wrote:
> > I think you should send an email to the user and dev mailing lists to ask 
> > for high-level feedback on this interface.  We want to make sure the 
> > interface is broad enough to support different implementations.  (And I'm 
> > no expert on leader election.)

I'll do that and address your comments.


- Anurag


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


On March 10, 2016, 11:45 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44287/
> ---
> 
> (Updated March 10, 2016, 11:45 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MasterContender and MasterDetector abstract classes can be subclassed by
> user-provided classes to allow for arbitrary contender/detector 
> implementations.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/contender.hpp PRE-CREATION 
>   include/mesos/master/detector.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44287/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/44289/.
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44322: Implemented a generalized interface for the authorizer.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 11, 2016, 3:57 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44322/
> ---
> 
> (Updated March 11, 2016, 3:57 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.
> 
> 
> Bugs: MESOS-2950
> https://issues.apache.org/jira/browse/MESOS-2950
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implements the [Generic Authorizer Interface v 
> 0.3.1](https://docs.google.com/document/d/1-XARWJFUq0r_TgRHz_472NvLZNjbqE4G8c2JL44OSMQ).
> 
> It effectively separates the language used to define ACLs from the
> language used to query it allowing for arbitrary identity server
> backends.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 705482656788ac12e9d21e355b71fd2ede2be558 
>   include/mesos/authorizer/authorizer.proto PRE-CREATION 
>   src/CMakeLists.txt e9f7c3a447aad79ab19d53879888a413a587408f 
>   src/Makefile.am b24f0f58fa188c16770fe6a3c23ec06262cb0955 
>   src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
>   src/authorizer/local/authorizer.hpp 
> c87a9915bae6bae7744bd57abd12e8d857181051 
>   src/authorizer/local/authorizer.cpp 
> 15c857de79cd7dd2c29b6b8cfb81204b919f1b28 
>   src/master/http.cpp 54a2569ff5b10388177a9bd29c6ddc0387e5e8fd 
>   src/master/master.cpp 249e82ffcef35aa8df3c5b9faef5b9b25d68facc 
>   src/master/quota_handler.cpp a41c91f10bc0eedc754425b4de1b3e184c4ffb08 
>   src/master/weights_handler.cpp 9e4ab19fd760a56f1bbce915d1c7b63a0d1e5ed5 
>   src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 
>   src/tests/master_authorization_tests.cpp 
> 29c89fb11da792c3e71eb880a19657ea225b3cc8 
>   src/tests/mesos.hpp 9409da7ffe81ab4b1fc01213e27f1f639ba36581 
>   src/tests/mesos.cpp 577072136415191334c74b69a1c0b7eed8e77e8b 
>   src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 
> 
> Diff: https://reviews.apache.org/r/44322/diff/
> 
> 
> Testing
> ---
> 
> make -j4 check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 44722: Libprocess: Add `SOL_TCP` flag for Windows.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44078, 44722]

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

- Mesos ReviewBot


On March 11, 2016, 6:40 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44722/
> ---
> 
> (Updated March 11, 2016, 6:40 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Add `SOL_TCP` flag for Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/config.hpp 1e0c2a5a61c3d1a7b50057f876f88a157a5e4ed8 
> 
> Diff: https://reviews.apache.org/r/44722/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 44670: Added master_detector and master_contender flags.

[Joseph Wu]

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




src/master/main.cpp (line 238)


Can you prepend `--` for all references to flags in error messages?  Here 
and below.



src/master/main.cpp (lines 354 - 366)


Why not do this:
```
Try  _contender = nullptr;
if (flags.master_contender.isSome()) {
  _contender = MasterContender::createFromModule(flags.master_contender);
} else {
  _contender = MasterContender::create(zk);
}
```



src/master/main.cpp (lines 374 - 386)


Ditto code re-organization above.



src/slave/main.cpp (lines 268 - 279)


Ditto code re-organization above.


- Joseph Wu


On March 10, 2016, 3:46 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44670/
> ---
> 
> (Updated March 10, 2016, 3:46 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master_detector and master_contender flags allow modules to be
> used for specifying the MasterContender and MasterDetector
> implementations to use.
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp 6f53099eab9b0e5917e508bef24b2c85302b33e2 
>   src/master/flags.cpp be981ed6155edce18bdb55188c78d73182159418 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp eb470154f30634b3db439be1c122ff93d3147afe 
>   src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 
> 
> Diff: https://reviews.apache.org/r/44670/diff/
> 
> 
> Testing
> ---
> 
> In addition to all unit tests passing, we are currently using this 
> functionality in our environment with a custom consensus stack. In our world, 
> we have a C++ plugin that calls out to an HTTP REST service (implemented in 
> Java/Scala, not that it matters).
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44669: Added createFromModule methods to MasterContender and MasterDetector.

[Joseph Wu]

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




src/master/contenders/contender.cpp (line 100)


This should look more like:
```
if (type.isNone()) {
  // return Standalone.
}

// return Module.
```



src/master/detectors/detector.cpp (line 127)


Ditto here.


- Joseph Wu


On March 10, 2016, 3:46 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44669/
> ---
> 
> (Updated March 10, 2016, 3:46 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The createFromModule will be used to create a MasterContender/Detector
> from a module (specified using the --modules flag on the command
> line).
> 
> 
> Diffs
> -
> 
>   src/master/contenders/contender.cpp PRE-CREATION 
>   src/master/detectors/detector.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44669/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44289: Added support for contender and detector modules.

[Joseph Wu]

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




src/master/contenders/zookeeper.cpp (lines 27 - 33)


Do you really need these new includes?


- Joseph Wu


On March 10, 2016, 3:46 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44289/
> ---
> 
> (Updated March 10, 2016, 3:46 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for contender and detector modules.
> 
> 
> Diffs
> -
> 
>   include/mesos/module/contender.hpp PRE-CREATION 
>   include/mesos/module/detector.hpp PRE-CREATION 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/examples/test_contender_module.cpp PRE-CREATION 
>   src/examples/test_detector_module.cpp PRE-CREATION 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/contenders/zookeeper.cpp 
> 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
>   src/master/detectors/zookeeper.cpp 9274435802d6292b183be48f42b43999476e016e 
>   src/module/manager.cpp 6ae99504005581b22a44768949b1d305cec517d9 
>   src/tests/module.hpp 4b32f29f2ce76100433621a5cb6b8cc87c9b38f8 
>   src/tests/module.cpp 8cc305c0ef606b07eea39d548d3165a2bb2b042a 
> 
> Diff: https://reviews.apache.org/r/44289/diff/
> 
> 
> Testing
> ---
> 
> In addition to all unit tests passing, we are currently using this 
> functionality in our environment with a custom consensus stack. In our world, 
> we have a C++ plugin that calls out to an HTTP REST service (implemented in 
> Java/Scala, not that it matters).
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44545: Separated standalone and zookeeper classes.

[Joseph Wu]

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




src/master/contender.hpp (lines 17 - 18)


Nit: Header guard should be this: __MASTER_CONTENDER_ZOOKEEPER_HPP__



src/master/contender.cpp (line 35)


I'd suggest fully qualifying this include.  i.e. 
`master/contenders/zookeeper.hpp`.



src/master/contender.cpp (line 65)


These aren't yours, but could you fix the spacing here?

i.e. s/> >/>>/

Here and everywhere else in these moved files.



src/master/contenders/contender.hpp (lines 17 - 18)


Nit: __MASTER_CONTENDER_STANDALONE_HPP_



src/master/contenders/contender.cpp (lines 30 - 31)


Ditto: Fully qualify these.



src/master/contenders/contender.cpp (line 92)


Nit: We want to keep 2 newlines between functions.



src/master/contenders/standalone.cpp (line 30)


Ditto: Fully quality this.



src/master/detector.hpp (lines 17 - 18)


Nit: Header guard should be this: __MASTER_CONTENDER_STANDALONE_HPP__



src/master/detector.cpp (line 46)


Ditto: I suggest fully qualifying this include.



src/master/detectors/detector.hpp (lines 17 - 18)


Nit: __MASTER_DETECTOR_ZOOKEEPER_HPP__



src/master/detectors/detector.cpp (lines 46 - 47)


Ditto: Fully qualify these.



src/master/detectors/standalone.cpp (line 46)


Ditto: Fully qualify this.



src/master/detectors/standalone.cpp (lines 58 - 60)


I don't see where these helpers are deleted in your later reviews.


- Joseph Wu


On March 10, 2016, 3:46 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44545/
> ---
> 
> (Updated March 10, 2016, 3:46 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of keeping standalone and zookeper contender/detector class
> definitions and implementations in the same file, separated them. Also
> made the necessary changes in users of class headers to point to the
> new locations.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/contender.hpp 3fd20f8e94daab349b76d8f5ecc87398a187a847 
>   src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
>   src/master/contenders/contender.hpp PRE-CREATION 
>   src/master/contenders/contender.cpp PRE-CREATION 
>   src/master/contenders/standalone.cpp PRE-CREATION 
>   src/master/detector.hpp eb5d2a90b60c629150ddf04acf00f0edca1ca723 
>   src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e 
>   src/master/detectors/detector.hpp PRE-CREATION 
>   src/master/detectors/detector.cpp PRE-CREATION 
>   src/master/detectors/standalone.cpp PRE-CREATION 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/master/master.hpp ea26670e6c6c67314406fded510e8fdd46053dc8 
>   src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
>   src/tests/containerizer/external_containerizer_test.cpp 
> 8e1dbe306a088eb16cd3b9c6174b95fad5685da4 
>   src/tests/containerizer/isolator_tests.cpp 
> 342037ce0a5f8caa4e3cf1550b8f9a7cc328acf9 
>   src/tests/fault_tolerance_tests.cpp 
> d193897e636efd0e3ef67bf67fcd6255a3de0341 
>   src/tests/gc_tests.cpp 61a8abb9581dc4602b197a88a677b19386969cbf 
>   src/tests/master_allocator_tests.cpp 
> cba7c36471f93b678d94e1da0251a28a893696b1 
>   src/tests/master_authorization_tests.cpp 
> 29c89fb11da792c3e71eb880a19657ea225b3cc8 
>   src/tests/master_contender_detector_tests.cpp 
> 255ab8119a04b55bb4f1b61dee19c4be64499376 
>   src/tests/master_slave_reconciliation_tests.cpp 
> d41178eb41df519073fc0890c5716bbc9fed6ad2 
>   src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
>   src/tests/mesos.hpp 

Re: Review Request 44288: Changed MasterDetector/Contender namespace.

[Joseph Wu]

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



Mostly nits here.


include/mesos/v1/scheduler.hpp (line 39)


We don't put aliases in header files since this can pollute the global 
namespace.  You'll have to use the long form below :(



include/mesos/v1/scheduler.hpp (line 82)


In case you're wondering about my above comment and the resulting spacing 
for this line, here's a suggestion:
```
  const 
Option& 
detector);
```



src/master/detector.cpp (lines 214 - 215)


Nit: I'm guessing you didn't mean to change this.



src/master/detector.cpp (lines 248 - 250)


No need to change this.



src/master/detector.cpp (lines 256 - 257)


Nit: We put 4 spaces after the indent when spliting up function arguments 
across lines.
```
  return new StandaloneMasterDetector(
  internal::protobuf::createMasterInfo(pid));
```



src/master/detector.cpp (lines 439 - 440)


Nit: We usually prefer this spacing:
```
  } else if (label.isSome() &&
  label.get() == mesos::internal::master::MASTER_INFO_LABEL) {
```

Ditto below.



src/slave/slave.hpp (lines 84 - 85)


Nit: aliases in a header file are not allowed.


- Joseph Wu


On March 10, 2016, 3:45 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44288/
> ---
> 
> (Updated March 10, 2016, 3:45 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also modified users of MasterContender and MasterDetector to use this
> namespace.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
>   include/mesos/v1/scheduler.hpp 765935e97b6c1686ab464a5cf1cf2dfd816f51f1 
>   src/cli/resolve.cpp 257e29034abf32491511f9a4e476b6859714829d 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
>   src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/master/master.hpp ea26670e6c6c67314406fded510e8fdd46053dc8 
>   src/master/master.cpp 57ff4a39039f573b8586bc03f873f97826b97f6f 
>   src/sched/sched.cpp 525255eec808c3fe5c0e38b3d1a2086bbd4eb171 
>   src/scheduler/scheduler.cpp b010a819132fb80810e7f8ce96778109f2e8b35e 
>   src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 
>   src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
>   src/tests/fault_tolerance_tests.cpp 
> d193897e636efd0e3ef67bf67fcd6255a3de0341 
>   src/tests/master_allocator_tests.cpp 
> cba7c36471f93b678d94e1da0251a28a893696b1 
>   src/tests/master_authorization_tests.cpp 
> 29c89fb11da792c3e71eb880a19657ea225b3cc8 
>   src/tests/master_contender_detector_tests.cpp 
> 255ab8119a04b55bb4f1b61dee19c4be64499376 
>   src/tests/master_slave_reconciliation_tests.cpp 
> d41178eb41df519073fc0890c5716bbc9fed6ad2 
>   src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
>   src/tests/oversubscription_tests.cpp 
> e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
>   src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
>   src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 
>   src/tests/reservation_tests.cpp d7f9de6f2bce061316916260f356efdb96ecd482 
>   src/tests/scheduler_event_call_tests.cpp 
> 8c02ceeb3ec1783cb2f63f100700508e70f586e4 
>   src/tests/scheduler_http_api_tests.cpp 
> dfb0f51fec67a3951e396eab28eedb0dbf9493ae 
>   src/tests/scheduler_tests.cpp fa42fb42f2d18060a867ade547cebbdcaead07d4 
>   src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
>   src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 
> 
> Diff: https://reviews.apache.org/r/44288/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/44289/.
> 

Re: Review Request 44544: Moved contender and detector definitions into separate directories.

[Joseph Wu]

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



Looking through the review chain, I don't see where you've deleted the original 
`src/master/detector.*` and `src/master/contender.*` files.  

Am I missing something?  (The patch here only appears to add new files.)

- Joseph Wu


On March 10, 2016, 3:46 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44544/
> ---
> 
> (Updated March 10, 2016, 3:46 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved contender and detector definitions into separate directories.
> 
> 
> Diffs
> -
> 
>   src/master/contenders/contender.hpp PRE-CREATION 
>   src/master/contenders/contender.cpp PRE-CREATION 
>   src/master/detectors/detector.hpp PRE-CREATION 
>   src/master/detectors/detector.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44544/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44320: Moved authorizer.proto to acls.proto.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 11, 2016, 1:47 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44320/
> ---
> 
> (Updated March 11, 2016, 1:47 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.
> 
> 
> Bugs: MESOS-2950
> https://issues.apache.org/jira/browse/MESOS-2950
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the first step towards separating the language used to define
> the ACLs from the mechanism to query them.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.hpp PRE-CREATION 
>   include/mesos/authorizer/authorizer.hpp 
> 705482656788ac12e9d21e355b71fd2ede2be558 
>   include/mesos/authorizer/authorizer.proto 
> d65377c6c4ed9364efdc531b885c1a8af962ec34 
>   src/CMakeLists.txt e9f7c3a447aad79ab19d53879888a413a587408f 
>   src/Makefile.am b24f0f58fa188c16770fe6a3c23ec06262cb0955 
>   src/authorizer/acls.cpp PRE-CREATION 
>   src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
>   src/common/parse.hpp 78c7cf12ca6a475305254177db6b6b2319a1f72f 
>   src/examples/persistent_volume_framework.cpp 
> 4218b1563e10aaefe9abcdc20c90c13651959790 
>   src/examples/test_authorizer_module.cpp 
> 95d77fbff0cdfdb360a8597fbba28404b59d0042 
>   src/master/flags.hpp 6f53099eab9b0e5917e508bef24b2c85302b33e2 
>   src/master/quota_handler.cpp a41c91f10bc0eedc754425b4de1b3e184c4ffb08 
> 
> Diff: https://reviews.apache.org/r/44320/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 44287: Added MasterContender and MasterDetector abstract classes.

[Joseph Wu]

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



I think you should send an email to the user and dev mailing lists to ask for 
high-level feedback on this interface.  We want to make sure the interface is 
broad enough to support different implementations.  (And I'm no expert on 
leader election.)


include/mesos/master/contender.hpp (lines 35 - 37)


Micro-nit: We prefer the java-doc comment style for new public headers.  
You might as well make the change.

```
/**
 * Comment...
 */
```



include/mesos/master/contender.hpp (lines 41 - 47)


Can you expand on these comments a bit?

I think the comments should touch upon:

- `create` exists for backwards compatibility.
- `create` only works for the Standalone and Zookeeper types.  (Also add 
something about the expected input format.)
- `createFromModule` takes a symbol and creates a module.  Any 
configuration for this method should live in the `--modules` flag as a 
`Parameters` object.
- `createFromModule` can return the Standalone type, or any other type 
(assuming you convert the Zookeeper one into an example module).

Ditto for the detector.


- Joseph Wu


On March 10, 2016, 3:45 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44287/
> ---
> 
> (Updated March 10, 2016, 3:45 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MasterContender and MasterDetector abstract classes can be subclassed by
> user-provided classes to allow for arbitrary contender/detector 
> implementations.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/contender.hpp PRE-CREATION 
>   include/mesos/master/detector.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44287/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/44289/.
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44319: Removed initialize method from the authorizer interface.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 11, 2016, 1:45 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44319/
> ---
> 
> (Updated March 11, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.
> 
> 
> Bugs: MESOS-2950
> https://issues.apache.org/jira/browse/MESOS-2950
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed initialize method from the authorizer interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 705482656788ac12e9d21e355b71fd2ede2be558 
>   src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
>   src/authorizer/local/authorizer.hpp 
> c87a9915bae6bae7744bd57abd12e8d857181051 
>   src/authorizer/local/authorizer.cpp 
> 15c857de79cd7dd2c29b6b8cfb81204b919f1b28 
>   src/examples/test_authorizer_module.cpp 
> 95d77fbff0cdfdb360a8597fbba28404b59d0042 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/44319/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 44721: Avoided external linkage for sched constants.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44190, 44717, 44191, 44718, 44719, 44720, 44721]

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

- Mesos ReviewBot


On March 11, 2016, 6:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44721/
> ---
> 
> (Updated March 11, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoided external linkage for sched constants.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e9f7c3a447aad79ab19d53879888a413a587408f 
>   src/Makefile.am b24f0f58fa188c16770fe6a3c23ec06262cb0955 
>   src/sched/constants.hpp 523c6d94571ae22b1aa8fe786f5c9f965039ea7a 
>   src/sched/constants.cpp c00b8ba31d5f24f99e99a9ec7034b641929246c0 
>   src/sched/flags.hpp 57dbab8ad7f8d4829d15627caf0e69ef91702081 
> 
> Diff: https://reviews.apache.org/r/44721/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni.hpp (line 85)


I would suggest we have a `Info` for each container.

```
struct Info
{
  ...
};
```

Remember that for any field you put into the Info struct, you need to be 
able to 'recover' it in recover function. It's also possible that 
ExecutorInfo/TaskInfo are not available during recover (orphans due to wiped 
meta data). You need to think about how to recover (e.g., the name of the 
network) that the container has joined.


- Jie Yu


On March 10, 2016, 2:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44621: Added tests for http endpoints with bad authentication.

[Joerg Schad]

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

(Updated March 11, 2016, 7:49 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Addressed review.


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


Repository: mesos


Description
---

With enabling http authentication for http endpoints we should also add tests 
to check
that http request to those endpoints return "401 Unauthorized" if queried 
without or with
bad credentials.


Diffs (updated)
-

  src/tests/master_maintenance_tests.cpp 
3c7024cfbd7e5bef75f092eace8d0e8ca423 
  src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
  src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
  src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 44555: Implemented the framework and create() method of "network/cni" isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni.hpp (lines 81 - 82)


I guess storing the protobuf is not sufficient because the JSON might 
contain plugin specific information.

I would suggest we define a `NetworkConfigInfo` nested type here:
```
struct NetworkConfigInfo
{
  std::string path;
  cni::NetworkConfig config;
};

hashmap networkConfigInfos;
```



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 48)


Let's remove this 'empty' check here since you hae the os::exists check 
below.



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 53)


Ditto on removing this check.



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 73 - 77)


I think the CNI plugin should also support the case where the user does not 
specify a name for the network. In other words,  the CNI network isolator 
should support host network as well. So it's OK to have no plugins.



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 100 - 102)


No need for this check. We can always rely on the parse function below.



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 177 - 181)


Again, this check should be removed to support containers that do not 
specify NetworkInfo.



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 186)


2 lines apart. Please fix all places.



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 189)


'{' should be in the next line. Please fix it in all places.


- Jie Yu


On March 9, 2016, 6:01 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44555/
> ---
> 
> (Updated March 9, 2016, 6:01 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the framework and create() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44555/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44711: Updated authentication.md after most endpoints enable authentication.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44286, 44186, 44621, 44711]

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

- Mesos ReviewBot


On March 11, 2016, 4:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44711/
> ---
> 
> (Updated March 11, 2016, 4:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Greg Mann.
> 
> 
> Bugs: MESOS-4844
> https://issues.apache.org/jira/browse/MESOS-4844
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reflected default authentication of endpoints in documentation.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md dd538b512c6f822cd742fc6e2f3d4f7fbffadc06 
> 
> Diff: https://reviews.apache.org/r/44711/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44621: Added tests for http endpoints with bad authentication.

[Greg Mann]

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


Fix it, then Ship it!





src/tests/master_maintenance_tests.cpp (lines 1770 - 1772)


One line?



src/tests/master_maintenance_tests.cpp (lines 1770 - 1772)


One line?



src/tests/master_tests.cpp (line 4195)


Looks like there are no POST requests here?



src/tests/master_tests.cpp (lines 4197 - 4199)


s/observe in the/observe endpoints in the/

Or, you could just say "we have similar checks for other endpoints in their 
respective..." ?



src/tests/master_tests.cpp (lines 4212 - 4214)


One line? Here and below in this file



src/tests/role_tests.cpp (line 657)


Since this file contains other tests which have nothing to do with HTTP 
endpoints, perhaps this name should be a bit more descriptive? Something like 
`RolesEndpointBadAuthentication`?


- Greg Mann


On March 10, 2016, 2:41 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44621/
> ---
> 
> (Updated March 10, 2016, 2:41 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-4844
> https://issues.apache.org/jira/browse/MESOS-4844
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With enabling http authentication for http endpoints we should also add tests 
> to check
> that http request to those endpoints return "401 Unauthorized" if queried 
> without or with
> bad credentials.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 3faa8136cf57276295553910319480028f433e4c 
>   src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
>   src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
>   src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 
> 
> Diff: https://reviews.apache.org/r/44621/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44186: Added authentication to master endpoints.

[Joerg Schad]

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

(Updated March 11, 2016, 7:12 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Greg Mann.


Changes
---

Rebased and addressed review.


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


Repository: mesos


Description
---

Added Authentication to master http endpoints (except version, health, 
redirect, scheduler).


Diffs (updated)
-

  src/master/http.cpp 54a2569ff5b10388177a9bd29c6ddc0387e5e8fd 
  src/master/master.hpp 4fa88f1968c92c1216c0af22a476ef7aa57ae961 
  src/master/master.cpp 249e82ffcef35aa8df3c5b9faef5b9b25d68facc 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6aecd912fc84b72d2b64f7a842891fddcbc469ac 
  src/tests/fault_tolerance_tests.cpp d193897e636efd0e3ef67bf67fcd6255a3de0341 
  src/tests/health_check_tests.cpp d0fd27fd8a6b48511ef8cafab5dff59f65729d9f 
  src/tests/master_maintenance_tests.cpp 
3c7024cfbd7e5bef75f092eace8d0e8ca423 
  src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
  src/tests/persistent_volume_endpoints_tests.cpp 
81185a161498394020a27f1f5bf747bac5425f43 
  src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
  src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 
  src/tests/status_update_manager_tests.cpp 
d64d3b8c96270478f6b681c038de77c3a9eb68fe 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 44620: Documented how to make executors work with SSL.

[Jan Schlicht]

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

(Updated March 11, 2016, 8:11 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

Documented how to make executors work with SSL.


Diffs (updated)
-

  docs/ssl.md 3de2a3e931091e002dc4b259c70eadd89a52b059 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

[Avinash sridharan]

> On March 11, 2016, 6:19 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/spec.proto, line 23
> > 
> >
> > OK, this is my bad. I didn't realize that network configuration JSON 
> > will inline some plugin specific or ipam specific fields.
> > 
> > In that case, I would suggest that we only list well-known fields and 
> > do not list plugin or ipam specific field in this protobuf.
> > 
> > You can take a look at:
> > https://github.com/appc/cni/blob/master/pkg/types/types.go#L59
> > 
> > I think json->protobuf parsing should still work if there are missing 
> > fields in the protobuf.

I agree. Have raised an issue to isolate the plugin/ipam specific fields into 
their own dictionaries:
https://github.com/appc/cni/issues/128


- Avinash


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


On March 10, 2016, 2:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 10, 2016, 2:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/spec.proto (lines 25 - 28)


Let's put Route to the top level as well similar to what CNI has done.


- Jie Yu


On March 10, 2016, 2:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 10, 2016, 2:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44553: Added authentication to agent HTTP endpoints.

[Greg Mann]

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

(Updated March 11, 2016, 6:56 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


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


Repository: mesos


Description
---

Added authentication to agent HTTP endpoints.

This patch adds HTTP authentication to the `/state`, `/state.json`, and 
`/flags` endpoints. Tests are also updated to use authentication when hitting 
these endpoints, and a new test was added to probe these endpoints' behavior 
when bad credentials are supplied: `SlaveTest.HTTPEndpointsBadAuthentication`.


Diffs
-

  src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
  src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6aecd912fc84b72d2b64f7a842891fddcbc469ac 
  src/tests/fault_tolerance_tests.cpp d193897e636efd0e3ef67bf67fcd6255a3de0341 
  src/tests/health_check_tests.cpp d0fd27fd8a6b48511ef8cafab5dff59f65729d9f 
  src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 

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


Testing (updated)
---

Tests were updated to use authentication when hitting the affected agent 
endpoints, and new tests were added to probe the endpoint behavior when invalid 
credentials are supplied.

`sudo make check` was used to test on both OSX and Ubuntu 14.04. The new test 
was run 1000 times to look for flakiness.


Thanks,

Greg Mann

Re: Review Request 44186: Added authentication to master endpoints.

[Greg Mann]

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


Fix it, then Ship it!





src/master/http.cpp (line 1279)


Also need to update the Docker tests:

grep -nr "\"state\"" src/tests/*
src/tests/containerizer/docker_containerizer_tests.cpp:512:  
Future response = http::get(master.get(), "state");


- Greg Mann


On March 10, 2016, 10:31 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44186/
> ---
> 
> (Updated March 10, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Greg Mann.
> 
> 
> Bugs: MESOS-4844
> https://issues.apache.org/jira/browse/MESOS-4844
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Authentication to master http endpoints (except version, health, 
> redirect, scheduler).
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 7304bfd5350d763d9ed1d5acdc285874b6d8f5df 
>   src/master/master.hpp 4fa88f1968c92c1216c0af22a476ef7aa57ae961 
>   src/master/master.cpp 249e82ffcef35aa8df3c5b9faef5b9b25d68facc 
>   src/tests/fault_tolerance_tests.cpp 
> d193897e636efd0e3ef67bf67fcd6255a3de0341 
>   src/tests/health_check_tests.cpp d0fd27fd8a6b48511ef8cafab5dff59f65729d9f 
>   src/tests/master_maintenance_tests.cpp 
> 3faa8136cf57276295553910319480028f433e4c 
>   src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 81185a161498394020a27f1f5bf747bac5425f43 
>   src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
>   src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 
>   src/tests/status_update_manager_tests.cpp 
> d64d3b8c96270478f6b681c038de77c3a9eb68fe 
> 
> Diff: https://reviews.apache.org/r/44186/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44553: Added authentication to agent HTTP endpoints.

[Greg Mann]

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

(Updated March 11, 2016, 6:54 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Simplified added tests.


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


Repository: mesos


Description (updated)
---

Added authentication to agent HTTP endpoints.

This patch adds HTTP authentication to the `/state`, `/state.json`, and 
`/flags` endpoints. Tests are also updated to use authentication when hitting 
these endpoints, and a new test was added to probe these endpoints' behavior 
when bad credentials are supplied: `SlaveTest.HTTPEndpointsBadAuthentication`.


Diffs (updated)
-

  src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
  src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6aecd912fc84b72d2b64f7a842891fddcbc469ac 
  src/tests/fault_tolerance_tests.cpp d193897e636efd0e3ef67bf67fcd6255a3de0341 
  src/tests/health_check_tests.cpp d0fd27fd8a6b48511ef8cafab5dff59f65729d9f 
  src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 

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


Testing
---

Tests were updated to use authentication when hitting the affected agent 
endpoints, and new tests were added to probe the endpoint behavior when invalid 
credentials are supplied.

`sudo make check` was used to test on both OSX and Ubuntu 14.04. The new tests 
were run 1000 times to look for flakiness.


Thanks,

Greg Mann

Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Avinash sridharan]

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



Can we introduce the protobuf before the `prepare` method patch?


src/slave/containerizer/mesos/isolators/network/spec.proto (line 77)


Can we insert an optional `name` field here to keep track of the `Network` 
this result is for? This should not break the JSON schema.


- Avinash sridharan


On March 10, 2016, 3:29 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44622/
> ---
> 
> (Updated March 10, 2016, 3:29 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkResult".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44622/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

[Avinash sridharan]

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




src/slave/containerizer/mesos/isolators/network/cni.hpp (line 85)


Please see my comment of using a vector instead of map in the previous 
patches?



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 277)


either s/realpath/`realpath`

or s/realpath/location



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 329 - 389)


Firstly, lambdas should be short. Secondly, as per my other comment with 
code we will block the isolator. A better strategy would be to move this entire 
code into a private method, and invoke that function with a `defer`. This would 
allow you to await on the futures of the subprocess return without blocking the 
isolator. You can use this code as a reference 
(https://github.com/apache/mesos/blob/a1a9cd5939d25f82214a5c533bde96a3493f81f3/src/slave/containerizer/mesos/containerizer.cpp#L1318)
 This uses a static function, but I think you can use a private method and 
`defer` to the isolator libprocess thread to invoke it.



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 341)


s/, so here we will only get its stdout//



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 358)


If we block here, it will lock the isolator, and prevent the 
`MesosContainerizer` from launching any more containers?


- Avinash sridharan


On March 11, 2016, 2:10 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 11, 2016, 2:10 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44078: Windows: Added Socket compatibility `#define`s to windows.hpp.

[Alex Clemmer]

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

(Updated March 11, 2016, 6:39 p.m.)


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


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


Repository: mesos


Description
---

Windows: Added Socket compatibility `#define`s to windows.hpp.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
ee13d12fcffcd564c7ded2d2f541d7bbdf6633c1 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 44722: Libprocess: Add `SOL_TCP` flag for Windows.

[Alex Clemmer]

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

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


Repository: mesos


Description
---

Libprocess: Add `SOL_TCP` flag for Windows.


Diffs
-

  3rdparty/libprocess/src/config.hpp 1e0c2a5a61c3d1a7b50057f876f88a157a5e4ed8 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 44620: Documented how to make executors work with SSL.

[Joseph Wu]

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


Fix it, then Ship it!




Verified the paragraph shows up correctly via `support/site-docker/...`.


docs/ssl.md (line 101)


Tiny nit on the last sentence:

s/the executors container./the executor's container./


- Joseph Wu


On March 11, 2016, 7:21 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44620/
> ---
> 
> (Updated March 11, 2016, 7:21 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-4750
> https://issues.apache.org/jira/browse/MESOS-4750
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented how to make executors work with SSL.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md 3de2a3e931091e002dc4b259c70eadd89a52b059 
> 
> Diff: https://reviews.apache.org/r/44620/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 44721: Avoided external linkage for sched constants.

[Neil Conway]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

Avoided external linkage for sched constants.


Diffs
-

  src/CMakeLists.txt e9f7c3a447aad79ab19d53879888a413a587408f 
  src/Makefile.am b24f0f58fa188c16770fe6a3c23ec06262cb0955 
  src/sched/constants.hpp 523c6d94571ae22b1aa8fe786f5c9f965039ea7a 
  src/sched/constants.cpp c00b8ba31d5f24f99e99a9ec7034b641929246c0 
  src/sched/flags.hpp 57dbab8ad7f8d4829d15627caf0e69ef91702081 

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


Testing
---


Thanks,

Neil Conway

Review Request 44718: Replaced `const string` master constants with `constexpr char[]`.

[Neil Conway]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

Per style guide.


Diffs
-

  src/master/constants.hpp 2c3299bc1aaa0888f7e47a71965c56ada8ecc21f 
  src/master/flags.cpp c1dd127109f1ba96a8f9b95f3eb99dfeb43f7d28 
  src/tests/flags.hpp 542f462453e41cad3b4cb8174ac59f058e625b19 

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


Testing
---


Thanks,

Neil Conway

Review Request 44719: Avoided external linkage for slave constants.

[Neil Conway]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

Avoided external linkage for slave constants.


Diffs
-

  src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
  src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 

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


Testing
---


Thanks,

Neil Conway

Review Request 44720: Replaced `const string` slave constants with `constexpr char[]`.

[Neil Conway]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

Per style guide.


Diffs
-

  src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
  src/slave/flags.cpp eb470154f30634b3db439be1c122ff93d3147afe 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 44191: Avoided external linkage for master constants.

[Neil Conway]

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

(Updated March 11, 2016, 6:31 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

We were only using external linkage to workaround bugs in ancient
(4.1.x) of GCC. We can also mark most of these constants `constexpr`,
which should be both slightly more efficient (in theory) and
safer (see MESOS-1023).


Diffs (updated)
-

  src/CMakeLists.txt e9f7c3a447aad79ab19d53879888a413a587408f 
  src/Makefile.am b24f0f58fa188c16770fe6a3c23ec06262cb0955 
  src/master/constants.hpp 2c3299bc1aaa0888f7e47a71965c56ada8ecc21f 
  src/master/constants.cpp e316f9772d880b694faeee6d001dc56bc088c118 

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


Testing
---

make check

Note that I left a few string constants as `const std::string`; you might 
arguably want them to be `constexpr char[]` (per Google Style Guide).


Thanks,

Neil Conway

Review Request 44717: Marked a few Duration constants `constexpr`.

[Neil Conway]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

Marked a few Duration constants `constexpr`.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp 
ecec70672313666124ee6a951b3db09e9401b9ff 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 44191: Avoided external linkage for master constants.

[Neil Conway]

> On March 11, 2016, 12:38 a.m., Ben Mahler wrote:
> > src/master/constants.hpp, line 108
> > 
> >
> > Could you also go ahead and do the change to `constexpr char[]` if 
> > possible for strings here?

Done, as a separate patch.


- Neil


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


On March 1, 2016, 2:26 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44191/
> ---
> 
> (Updated March 1, 2016, 2:26 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4790
> https://issues.apache.org/jira/browse/MESOS-4790
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We were only using external linkage to workaround bugs in ancient
> (4.1.x) of GCC. We can also mark most of these constants `constexpr`,
> which should be both slightly more efficient (in theory) and
> safer (see MESOS-1023).
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt a62042fbd45b46217a88098875524f93d3079de3 
>   src/Makefile.am 5d8fe8bf33acb352589b2b3e0341987f9a41bc17 
>   src/master/constants.hpp 2c3299bc1aaa0888f7e47a71965c56ada8ecc21f 
>   src/master/constants.cpp e316f9772d880b694faeee6d001dc56bc088c118 
> 
> Diff: https://reviews.apache.org/r/44191/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Note that I left a few string constants as `const std::string`; you might 
> arguably want them to be `constexpr char[]` (per Google Style Guide).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44190: Made `Bytes` usable in `constexpr` expressions [stout].

[Neil Conway]

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

(Updated March 11, 2016, 6:30 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Split Duration changes into a separate patch.


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


Repository: mesos


Description (updated)
---

Made `Bytes` usable in `constexpr` expressions [stout].


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/bytes.hpp 
88a8bd4019fcbdaa5fcde4867cee9a198c83c1c6 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 43630: Especially updated scheduler tests to use the updated MesosTest helpers.

[Joseph Wu]

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

(Updated March 11, 2016, 10:24 a.m.)


Review request for mesos, Bernd Mathiske and Artem Harutyunyan.


Changes
---

Rebase on the PendingUnavailabilityTest changes.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Continuation of https://reviews.apache.org/r/43615/ with re-ordering of some 
local variables due to the order of destruction.


Diffs (updated)
-

  src/tests/master_maintenance_tests.cpp 
3c7024cfbd7e5bef75f092eace8d0e8ca423 
  src/tests/scheduler_tests.cpp fa42fb42f2d18060a867ade547cebbdcaead07d4 

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


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 44288: Changed MasterDetector/Contender namespace.

[Anurag Singh]

> On March 3, 2016, 10:15 p.m., Joseph Wu wrote:
> > src/master/contender.hpp, lines 17-18
> > 
> >
> > This whole file seems like a pretty substantial change.  I'd recommend 
> > pulling it out into a separate review (rather than hiding it in this 
> > review).
> > 
> > Also, you'll want to consider making folders "contenders" and 
> > "detectors".  Then renaming this file "standalone.hpp".
> 
> Anurag Singh wrote:
> Ok. Although then zookeeper's classes should also go into their own files.
> 
> Anurag Singh wrote:
> Looking at this again, the changes in this file are really just namespace 
> changes (the only thing substantial is the removal of the MasterContender 
> class definition) - I can't put them into a different commit without breaking 
> the build (I'm trying to make sure individual commits don't break builds, 
> which I think is a sensible goal). However, I can leave this commit as is but 
> create a separate commit to create the contenders/detectors directories. Is 
> that acceptable to you?
> 
> Anurag Singh wrote:
> Ping ... will the suggestion I made work for you?
> 
> Joseph Wu wrote:
> Don't worry about having atomic commits (especially for verbose changes).
> 
> There are a couple of changes to consider here, each of which might 
> deserve its own review:
> - Pulling out the interface deletions.  (You also consider pulling it 
> into the previous review, as you're really moving the code from private to 
> public headers.)
> - Adding a folder for each module you are adding ("contender" and 
> "detector").
> - Breaking apart the Standalone vs Zookeeper logic.
> 
> Anurag Singh wrote:
> While separating the zookeeper and standalone detectors and moving them 
> into detectors, I realized that the functions in the 'promises' namespace 
> will be duplicated unless I move them into their own header file. I can 
> either put them in include/mesos and change the namespace to mesos 
> mesos::internal::promises, or just  leave the header file in detectors. Which 
> one is preferable? bmahler had a todo for doing this  so I'm not sure if your 
> team already has some work planned in this area.
> 
> Joseph Wu wrote:
> I would *tentatively* recommend putting those helpers inside 
> `3rdparty/libprocess/include/process/collect.hpp`, where we have a bunch of 
> other helpers that operate on groups of `Futures`.  We definitely want those 
> helpers in a libprocess header somewhere, but I'm not sure what the most 
> acceptable place would be.  (I'll ask BenM about it.)
> 
> Anurag Singh wrote:
> The logic separation change has turned into a major change - if you see a 
> lot of issues with the commits, we should spin the separation change into an 
> issue by itself (I can work on that if needed). It will allow this review to 
> proceed faster - unfortunately we're blocked on our module changes until this 
> review completes.

Hi ... have you had a chance to take another look?


- Anurag


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


On March 10, 2016, 11:45 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44288/
> ---
> 
> (Updated March 10, 2016, 11:45 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also modified users of MasterContender and MasterDetector to use this
> namespace.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
>   include/mesos/v1/scheduler.hpp 765935e97b6c1686ab464a5cf1cf2dfd816f51f1 
>   src/cli/resolve.cpp 257e29034abf32491511f9a4e476b6859714829d 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
>   src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/master/master.hpp ea26670e6c6c67314406fded510e8fdd46053dc8 
>   src/master/master.cpp 57ff4a39039f573b8586bc03f873f97826b97f6f 
>   src/sched/sched.cpp 525255eec808c3fe5c0e38b3d1a2086bbd4eb171 
>   src/scheduler/scheduler.cpp b010a819132fb80810e7f8ce96778109f2e8b35e 
>   src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 
>   src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

[Jie Yu]

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




src/CMakeLists.txt (line 52)


+1 on moving into cni/ subdirectory.

I think we might want to introduce a few helper functions (e.g., parsing) 
in a separate hpp|cpp.



src/slave/containerizer/mesos/isolators/network/spec.proto (line 23)


OK, this is my bad. I didn't realize that network configuration JSON will 
inline some plugin specific or ipam specific fields.

In that case, I would suggest that we only list well-known fields and do 
not list plugin or ipam specific field in this protobuf.

You can take a look at:
https://github.com/appc/cni/blob/master/pkg/types/types.go#L59

I think json->protobuf parsing should still work if there are missing 
fields in the protobuf.



src/slave/containerizer/mesos/isolators/network/spec.proto (lines 31 - 34)


Those are ipam specific, let's remote those.



src/slave/containerizer/mesos/isolators/network/spec.proto (lines 45 - 55)


What's this?



src/slave/containerizer/mesos/isolators/network/spec.proto (line 60)


This is plugin specific, let's remove this from this protobuf.



src/slave/containerizer/mesos/isolators/network/spec.proto (line 61)


Ditto.



src/slave/containerizer/mesos/isolators/network/spec.proto (lines 63 - 67)


Remove these as well.


- Jie Yu


On March 10, 2016, 2:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 10, 2016, 2:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Avinash sridharan]

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




src/slave/containerizer/mesos/isolators/network/cni.hpp (line 85)


I don't think we should have a hashmap within a hashmap. Hashmap are 
expensive in terms of memory. The outer hashmap makes sense since the isolator 
might need to manage hundreds of containers, but the inner hashmap is just a 
mapping from the network name to the result of attaching the container to that 
network, and most containers would joing a single network. Maybe just have a 
vector of strings (network names) here, and later, a vector of `NetworkResults` 
(once you introduce the protobuf) and name an `optional` field in the protobuf?



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 228)


Can there be a use case where you want multiple NICs to be attached to the 
same network? Servers use this configuration when they want to utilize NIC 
bonding. To aggregate the bandwidth available on the NICs



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 236)


If the CNI configuration is host-local then we will be calling the 
host-local plugin during `isolate`. So why do we need this?


- Avinash sridharan


On March 10, 2016, 2:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>