Re: Review Request 37172: Maintenance Primitives: Set offer `unavailability` if slave is scheduled for maintenance.

[Joris Van Remoortere]

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

(Updated Sept. 14, 2015, 12:25 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.


Changes
---

Added comparison operators to v1 api.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/type_utils.hpp 4fb0037a224cab7ebeb6644b5274227fedb172c8 
  include/mesos/v1/mesos.hpp 0d695f36558fdc390461a8767a0c9db8e321a07a 
  src/master/master.cpp c90311fa2152810e7604a0a2dee630bd14929574 
  src/tests/master_maintenance_tests.cpp 
fb8dca3757a9565d5eb5a69eed10aa34602bb15c 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 37173: Maintenance Primitives: Added unavailability to Allocator's Slave struct.

[Joris Van Remoortere]

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

(Updated Sept. 14, 2015, 12:40 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.


Changes
---

Renamed `MaintenanceStatus` to `Maintenance`


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
  include/mesos/master/allocator.proto 10fd9a2d5fcbc18a9ca2d6c9c0ec1c605f21872b 
  src/master/allocator/mesos/allocator.hpp 
aa55755a9c3250579e9366bdbc17a2449e95d659 
  src/master/allocator/mesos/hierarchical.hpp 
fbf353d8bdd4322275057e392a958fca77ecd8b3 
  src/master/master.cpp c90311fa2152810e7604a0a2dee630bd14929574 
  src/tests/hierarchical_allocator_tests.cpp 
9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
  src/tests/master_allocator_tests.cpp 89331965553505f6b7eebf39ad27d943df816a24 
  src/tests/mesos.hpp 6b5031191b81a18f9596f547a1e0f67e35881cc3 
  src/tests/reservation_endpoints_tests.cpp 
dfab4971e04a81ac98ed118ea877bcca5db17bb5 
  src/tests/reservation_tests.cpp aeee36752573e3f401d3dca7d2d69c90d0e8bd6b 
  src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 37180: Maintenance Primitives: Implemented Master::inverseOffer.

[Joris Van Remoortere]

> On Aug. 31, 2015, 7:49 a.m., Jian Qiu wrote:
> > src/master/master.cpp, line 4904
> > 
> >
> > If the allocator calls inverse callback followed by an offer callback, 
> > will the two messages be handled in two separate threads of the framework? 
> > I think it will be framework's responsibility to ensure data 
> > synchronization between the two threads?

This is up to how the framework driver is implemented. We have a reference 
implementation, but with the V1 API there will be many implementations.
The current C++ scheduler driver handles 1 message type at a time, so they will 
be synchronized. If the framework decides to do asynchronous calls during those 
callback invocations, then it is up to the framework writer to synchronize them.
Does this make sense?
Since I'm closing this review feel free to reach out to me via e-mail to 
continue this discussion!


- Joris


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


On Sept. 13, 2015, 8:33 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37180/
> ---
> 
> (Updated Sept. 13, 2015, 8:33 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 12cc1ad45de3291ec22d4fe2b7ee11c4d7565c24 
>   src/master/master.cpp c90311fa2152810e7604a0a2dee630bd14929574 
> 
> Diff: https://reviews.apache.org/r/37180/diff/
> 
> 
> Testing
> ---
> 
> The tests break as expected.
> With the scheduler API change there are CHECKs that fail.
> Once we update the API these will be resolved.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

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

[haosdent huang]

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

(Updated Sept. 14, 2015, 12:59 p.m.)


Review request for mesos and Timothy Chen.


Changes
---

Rebase


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


Repository: mesos


Description
---

Fix broken health check in docker executor.


Diffs (updated)
-

  src/docker/docker.hpp aaf8884a20901ad3a440d545d4b6316b87fc3403 
  src/docker/docker.cpp 553e831029454d6d423842915b8bbfcaf19fa7f9 
  src/docker/executor.cpp 6647075b55e5a79264e3556bb906a1f26a2d673e 
  src/slave/containerizer/docker.cpp 289d4ec0fba9071dfe0cbf5391b5391d4566dd9c 
  src/tests/health_check_tests.cpp 157a56aa06677d8b7a2cef53b29ed05cb4b5d8ea 

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


Testing
---

# Add two new test cases, HealthCheckTest.ROOT_DOCKER_DockerHealthyTask and 
HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
# Docker health check command is run through "docker exec"
sudo ./bin/mesos-tests.sh 
--gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthyTask" --verbose
sudo ./bin/mesos-tests.sh 
--gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange" --verbose


Thanks,

haosdent huang

Re: Review Request 38287: Check if the futrue is failed before dispatch in freeze()

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38287]

All tests passed.

- Mesos ReviewBot


On Sept. 14, 2015, 7:28 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38287/
> ---
> 
> (Updated Sept. 14, 2015, 7:28 a.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.
> 
> 
> Bugs: MESOS-3272
> https://issues.apache.org/jira/browse/MESOS-3272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This check in cgroups::freezer::freeze() is necessary
> since the freezer process can be terminated in its
> initialize method
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/38287/diff/
> 
> 
> Testing
> ---
> 
> ./mesos-tests.sh 
> --gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer"
>  --verbose
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 37177: Maintenance Primitives: Added inverse offers.

[Joris Van Remoortere]

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

(Updated Sept. 14, 2015, 12:51 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.


Changes
---

rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/maintenance/maintenance.hpp 
7fec3ffe063e766dcec4952001fa5c6e20d9eec8 
  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
  src/master/allocator/mesos/allocator.hpp 
aa55755a9c3250579e9366bdbc17a2449e95d659 
  src/master/allocator/mesos/hierarchical.hpp 
fbf353d8bdd4322275057e392a958fca77ecd8b3 
  src/master/master.hpp 12cc1ad45de3291ec22d4fe2b7ee11c4d7565c24 
  src/master/master.cpp c90311fa2152810e7604a0a2dee630bd14929574 
  src/tests/hierarchical_allocator_tests.cpp 
9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
  src/tests/master_allocator_tests.cpp 89331965553505f6b7eebf39ad27d943df816a24 
  src/tests/mesos.hpp 6b5031191b81a18f9596f547a1e0f67e35881cc3 
  src/tests/reservation_endpoints_tests.cpp 
dfab4971e04a81ac98ed118ea877bcca5db17bb5 
  src/tests/reservation_tests.cpp aeee36752573e3f401d3dca7d2d69c90d0e8bd6b 
  src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519 
  src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 37622: Maintenance Primitives: Shutdown & remove slave when maintenance is started.

[Joris Van Remoortere]

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

(Updated Sept. 14, 2015, 1:08 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/http.cpp 73e8857833603cb0edb77c352f343070ee1b8c80 
  src/tests/master_maintenance_tests.cpp 
fb8dca3757a9565d5eb5a69eed10aa34602bb15c 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 37175: Maintenance Primitives: Added updateUnavailability to master.

[Joris Van Remoortere]

> On Sept. 14, 2015, 12:37 a.m., Qian Zhang wrote:
> > src/master/master.cpp, lines 4131-4134
> > 
> >
> > I think we have done this in Master::Http::maintenanceSchedule(), so 
> > why we do it again here? Maybe remove the duplicated code from 
> > Master::Http::maintenanceSchedule()?

You are right! Thanks for catching this :-)
Fixed in updated review.


- Joris


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


On Sept. 14, 2015, 12:46 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37175/
> ---
> 
> (Updated Sept. 14, 2015, 12:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> fbf353d8bdd4322275057e392a958fca77ecd8b3 
>   src/master/http.cpp 73e8857833603cb0edb77c352f343070ee1b8c80 
>   src/master/master.hpp 12cc1ad45de3291ec22d4fe2b7ee11c4d7565c24 
>   src/master/master.cpp c90311fa2152810e7604a0a2dee630bd14929574 
>   src/tests/master_maintenance_tests.cpp 
> fb8dca3757a9565d5eb5a69eed10aa34602bb15c 
>   src/tests/mesos.hpp 6b5031191b81a18f9596f547a1e0f67e35881cc3 
> 
> Diff: https://reviews.apache.org/r/37175/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37175: Maintenance Primitives: Added updateUnavailability to master.

[Joris Van Remoortere]

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

(Updated Sept. 14, 2015, 12:46 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
  src/master/allocator/mesos/allocator.hpp 
aa55755a9c3250579e9366bdbc17a2449e95d659 
  src/master/allocator/mesos/hierarchical.hpp 
fbf353d8bdd4322275057e392a958fca77ecd8b3 
  src/master/http.cpp 73e8857833603cb0edb77c352f343070ee1b8c80 
  src/master/master.hpp 12cc1ad45de3291ec22d4fe2b7ee11c4d7565c24 
  src/master/master.cpp c90311fa2152810e7604a0a2dee630bd14929574 
  src/tests/master_maintenance_tests.cpp 
fb8dca3757a9565d5eb5a69eed10aa34602bb15c 
  src/tests/mesos.hpp 6b5031191b81a18f9596f547a1e0f67e35881cc3 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 37282: Maintenance Primitives: Added InverseOffer to V1 API.

[Joris Van Remoortere]

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

(Updated Sept. 14, 2015, 12:54 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.


Changes
---

Added new-line.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/internal/devolve.hpp 6e4306df78b9b8d2054e6550209341fd7b0972d6 
  src/internal/devolve.cpp 0a069e51053b572a8d5dc95380732119504dd0c9 
  src/internal/evolve.hpp 13e9f52da98567038ec717f394f79e526a1521e9 
  src/internal/evolve.cpp 11ce9e77490e93f781ceebc33063d13953a11765 
  src/messages/messages.proto 8977d8e0f3b16003128b6b9cab556a7b224f083c 

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


Testing
---


Thanks,

Joris Van Remoortere

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

[Alexander Rukletsov]

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

(Updated Sept. 14, 2015, 10:52 a.m.)


Review request for mesos, haosdent huang, Joseph Wu, and Michael Park.


Changes
---

MPark's comments.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

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

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


Testing
---

make check (Mac OS 10.10.4)

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


Thanks,

Alexander Rukletsov

Re: Review Request 37170: Maintenance Primitives: Added `Machine` to Slave struct in Master.

[Joris Van Remoortere]

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

(Updated Sept. 14, 2015, 12:11 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.


Changes
---

Removed new-line.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/Makefile.am 8963cea9fd7e3bee450a1059e6383e5ab868a17b 
  src/master/http.cpp 73e8857833603cb0edb77c352f343070ee1b8c80 
  src/master/machine.hpp PRE-CREATION 
  src/master/maintenance.hpp bebaeb24efbd58df38796e3941f36a21791b6bfd 
  src/master/maintenance.cpp 277dd8270fbd497616e5b24db4ea7dd615e170e3 
  src/master/master.hpp 12cc1ad45de3291ec22d4fe2b7ee11c4d7565c24 
  src/master/master.cpp c90311fa2152810e7604a0a2dee630bd14929574 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 37280: Maintenance Primitives: Added updateInverseOffer to Allocator.

[Joris Van Remoortere]

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

(Updated Sept. 14, 2015, 12:57 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.


Changes
---

rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
  src/master/allocator/mesos/allocator.hpp 
aa55755a9c3250579e9366bdbc17a2449e95d659 
  src/master/allocator/mesos/hierarchical.hpp 
fbf353d8bdd4322275057e392a958fca77ecd8b3 
  src/tests/mesos.hpp 6b5031191b81a18f9596f547a1e0f67e35881cc3 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 37622: Maintenance Primitives: Shutdown & remove slave when maintenance is started.

[Joris Van Remoortere]

> On Sept. 14, 2015, 1:53 a.m., Qian Zhang wrote:
> > src/master/http.cpp, line 1606
> > 
> >
> > s/machineInfos/machines

Nice catch! Will be fixed in updated review.


- Joris


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


On Sept. 13, 2015, 8:34 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37622/
> ---
> 
> (Updated Sept. 13, 2015, 8:34 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 73e8857833603cb0edb77c352f343070ee1b8c80 
>   src/tests/master_maintenance_tests.cpp 
> fb8dca3757a9565d5eb5a69eed10aa34602bb15c 
> 
> Diff: https://reviews.apache.org/r/37622/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37284: Maintenance Primitives: Added support for Accept / Decline of InverseOffers in master.

[Joris Van Remoortere]

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

(Updated Sept. 14, 2015, 1:03 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.


Changes
---

Removed extra new-line.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/master.cpp c90311fa2152810e7604a0a2dee630bd14929574 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)

[Klaus Ma]

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



src/Makefile.am (line 1463)


Done



src/examples/dynamic_reservation_framework.cpp (line 40)


Done



src/examples/dynamic_reservation_framework.cpp (line 49)


Done



src/examples/dynamic_reservation_framework.cpp (line 180)


Done



src/examples/dynamic_reservation_framework.cpp (line 183)


Done



src/examples/dynamic_reservation_framework.cpp (line 184)


Done



src/examples/dynamic_reservation_framework.cpp (line 281)


Done



src/examples/dynamic_reservation_framework.cpp (line 283)


Done


- Klaus Ma


On Sept. 6, 2015, 4:11 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> ---
> 
> (Updated Sept. 6, 2015, 4:11 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3063
> https://issues.apache.org/jira/browse/MESOS-3063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Provide example for dynamic reservation features.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5fdca0f 
>   src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
>   src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
>   src/tests/examples_tests.cpp 3f56b30 
>   src/tests/flags.hpp 06da36d 
>   src/tests/script.cpp bcc1fab 
> 
> Diff: https://reviews.apache.org/r/37168/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)

[Klaus Ma]

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

(Updated Sept. 14, 2015, 1:48 p.m.)


Review request for mesos and Michael Park.


Changes
---

Address review comments


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


Repository: mesos


Description
---

Provide example for dynamic reservation features.


Diffs (updated)
-

  src/Makefile.am 8963cea 
  src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
  src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
  src/tests/examples_tests.cpp 3f56b30 
  src/tests/flags.hpp 06da36d 
  src/tests/script.cpp bcc1fab 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 37622: Maintenance Primitives: Shutdown & remove slave when maintenance is started.

[Joris Van Remoortere]

> On Sept. 14, 2015, 3:36 a.m., Benjamin Hindman wrote:
> > src/master/http.cpp, lines 1622-1624
> > 
> >
> > Why don't we want to send lost slave messages?
> 
> Benjamin Hindman wrote:
> Sorry, I meant: why don't we want to let lost slave messages get sent in 
> the normal flow? Why do we need/want to do it immediately and explicitly here?

I've removed the explicit removal here and created JIRA MESOS-3420 to discuss 
the semantics further.


- Joris


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


On Sept. 14, 2015, 1:08 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37622/
> ---
> 
> (Updated Sept. 14, 2015, 1:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 73e8857833603cb0edb77c352f343070ee1b8c80 
>   src/tests/master_maintenance_tests.cpp 
> fb8dca3757a9565d5eb5a69eed10aa34602bb15c 
> 
> Diff: https://reviews.apache.org/r/37622/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 38279: [MESOS-3366] Allow resources/attributes discovery

[Felix Abecassis]

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

(Updated Sept. 14, 2015, 10:38 a.m.)


Review request for mesos, Connor Doyle and Niklas Nielsen.


Changes
---

Adding myself as reviewer


Repository: mesos


Description
---

First API draft for MESOS-3366.

1) Only supports resources for now, we can add another hook for attributes with 
a very similar code.
2) The callback currently receives the full SlaveInfo structure and construct a 
new Resources object.
3) If there is multiple callbacks, each callback will see the changes made by 
previous callbacks and are free to override or merge the detected resources as 
they see fit.


Diffs
-

  include/mesos/hook.hpp d90bacc 
  src/examples/test_hook_module.cpp bc13a8a 
  src/hook/manager.hpp 30d8321 
  src/hook/manager.cpp 754c238 
  src/slave/slave.cpp 5e5522e 

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


Testing
---

make clean && make && make check


Thanks,

Felix Abecassis

Re: Review Request 38279: Enabled resources/attributes discovery

[Felix Abecassis]

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

(Updated Sept. 14, 2015, 10:39 a.m.)


Review request for mesos, Connor Doyle and Niklas Nielsen.


Summary (updated)
-

Enabled resources/attributes discovery


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


Repository: mesos


Description
---

First API draft for MESOS-3366.

1) Only supports resources for now, we can add another hook for attributes with 
a very similar code.
2) The callback currently receives the full SlaveInfo structure and construct a 
new Resources object.
3) If there is multiple callbacks, each callback will see the changes made by 
previous callbacks and are free to override or merge the detected resources as 
they see fit.


Diffs
-

  include/mesos/hook.hpp d90bacc 
  src/examples/test_hook_module.cpp bc13a8a 
  src/hook/manager.hpp 30d8321 
  src/hook/manager.cpp 754c238 
  src/slave/slave.cpp 5e5522e 

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


Testing
---

make clean && make && make check


Thanks,

Felix Abecassis

Re: Review Request 38253: Add containerId to ResourceUsage to enable QoS controller to target a container

[Klaus Ma]

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

(Updated Sept. 14, 2015, 10:41 a.m.)


Review request for mesos and Niklas Nielsen.


Summary (updated)
-

Add containerId to ResourceUsage to enable QoS controller to target a container


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


Repository: mesos


Description
---

We should ensure that we are addressing the container which the QoS controller 
intended to kill. Without this check, we may run into a scenario where the 
executor has terminated and one with the same id has started in the interim 
i.e. running in a different container than the one the QoS controller targeted.

This most likely requires us to add containerId to the ResourceUsage message 
and encode the containerID in the QoS Correction message.


Diffs
-

  include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
  include/mesos/slave/oversubscription.proto 
fa69a95689c8c75765f0800692655e8dde7dde33 
  src/slave/slave.cpp 5e5522e1254a5ed6084de36782753f9aad2894c4 
  src/tests/oversubscription_tests.cpp 0c5edafc139d9bfb6806d007a0af85e80893bb1a 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 38348: Correct typo in http.cpp

[Joseph Wu]

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


That's not a typo; "iff" is short for "if and only if".

- Joseph Wu


On Sept. 14, 2015, 1:48 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38348/
> ---
> 
> (Updated Sept. 14, 2015, 1:48 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Correct typo in http.cpp
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 73e8857833603cb0edb77c352f343070ee1b8c80 
> 
> Diff: https://reviews.apache.org/r/38348/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38289: Handle bad request in Docker registry client.

[Jojy Varghese]

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

Ship it!


Ship It!

- Jojy Varghese


On Sept. 11, 2015, 7:34 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38289/
> ---
> 
> (Updated Sept. 11, 2015, 7:34 p.m.)
> 
> 
> Review request for mesos and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Handle bad request in Docker registry client.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8963cea9fd7e3bee450a1059e6383e5ab868a17b 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp 
> ff29d562c7f1bd5f0579e97cdbd60c2b2f36329e 
> 
> Diff: https://reviews.apache.org/r/38289/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38364: Minor refactor for MesosContainerizer.

[Kapil Arya]

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

(Updated Sept. 14, 2015, 5:54 p.m.)


Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.


Changes
---

Updated description.


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


Repository: mesos


Description (updated)
---

This change moves two different pieces of code, that each iterate over list of 
ContainerPrepareInfos, close together for readability. It becomes even more 
relevant when looking at https://reviews.apache.org/r/38365/ that iterates over 
yet another member from ContainerPrepareInfo.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
1b83a8725b35435531038e37188b4c97189cef03 

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


Testing
---

make  check


Thanks,

Kapil Arya

Re: Review Request 38367: Added NetworkInfo message to ContainerInfo and TaskStatus.

[Niklas Nielsen]

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



include/mesos/mesos.proto (line 1163)


We still go by 70 col comments, right? If so, let's wrap the comment blocks.



include/mesos/mesos.proto (line 1164)


Wonder if we can enumerate more container information examples that may go 
into the container status (so it doesn't become a network-only structure)



include/mesos/mesos.proto (line 1362)


Request _and_ resolution, right?

Can we capture this here?



include/mesos/mesos.proto (line 1365)


We probably need to have a caveat about the pluggable nature of networking 
(and that if not enabled, this will be not be interpreted by mesos)



include/mesos/mesos.proto (line 1376)


We should probably mention that it is up to the mesos networking 'provider' 
to interpret this as either a hint or a requirement for the assigned ip.



include/mesos/mesos.proto (line 1386)


We could mention that these groups can be used for isolation



include/mesos/mesos.proto (line 1390)


pop? :)


- Niklas Nielsen


On Sept. 14, 2015, 1:55 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38367/
> ---
> 
> (Updated Sept. 14, 2015, 1:55 p.m.)
> 
> 
> Review request for mesos, Connor Doyle, Jie Yu, Niklas Nielsen, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-3013
> https://issues.apache.org/jira/browse/MESOS-3013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows the frameworks to specify an intent to enable ip-per-container. 
> The
> IP information is supplied back to the framework as well as state.json 
> endpoints
> by including NetworkInfo inside TaskStatus.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
>   src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60 
>   src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156 
>   src/slave/slave.cpp 5e5522e1254a5ed6084de36782753f9aad2894c4 
>   src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 
>   src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 
> 
> Diff: https://reviews.apache.org/r/38367/diff/
> 
> 
> Testing
> ---
> 
> Added new tests and ran make check.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Review Request 38378: Minor cleanup in perf_tests.cpp.

[Ben Mahler]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/containerizer/perf_tests.cpp 
bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38 

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


Testing
---

make check


Thanks,

Ben Mahler

Review Request 38382: Fixed the perf event isolator to continue sampling in the presence of failures.

[Ben Mahler]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

See [MESOS-3423](https://issues.apache.org/jira/browse/MESOS-3423).


Diffs
-

  src/slave/containerizer/isolators/cgroups/perf_event.cpp 
7dc8b7a99074b74ade019ef4df296780650a2e4e 

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


Testing
---

make check


Thanks,

Ben Mahler

Review Request 38375: Fixed process::collect and process::await to do discard propagation.

[Ben Mahler]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed process::collect and process::await to do discard propagation.


Diffs
-

  3rdparty/libprocess/Makefile.am 6361ac6485760fdeed87a2c049d6431783989462 
  3rdparty/libprocess/include/process/collect.hpp 
d07b6860a5b5cbed910b2e9e2ab2429bd70a3999 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
b61ca1e5be2054c1156205c62a1e8311a575bfa5 
  3rdparty/libprocess/src/tests/collect_tests.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/process_tests.cpp 
435663b10c1bfce07e8e84719aa14b5867c651c6 

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


Testing
---

Pulled out collect_tests.cpp and added a 'DiscardPropagation' test for both 
collect and await.


Thanks,

Ben Mahler

Review Request 38380: Minor cleanups in the perf event isolator.

[Ben Mahler]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This removes the special casing of the empty set of cgroups case to simplify 
the sample loop, along with some other minor cleanups.


Diffs
-

  src/slave/containerizer/isolators/cgroups/perf_event.cpp 
7dc8b7a99074b74ade019ef4df296780650a2e4e 

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


Testing
---

make check


Thanks,

Ben Mahler

Re: Review Request 38374: Added logics in LinuxFilesystemIsolator to recover and cleanup orphans.

[Jiang Yan Xu]

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

Ship it!


We should probably have a test case (or some checks in existing tests) for 
this. (and I should add one for `provisioner::recover()` for orphans)


src/slave/containerizer/isolators/filesystem/linux.cpp (lines 134 - 139)


Instead of doing it here and adding this block of comment we can put it in 
paths.hpp and it's more obviously there and probably requires no additional 
comments.


- Jiang Yan Xu


On Sept. 14, 2015, 3:30 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38374/
> ---
> 
> (Updated Sept. 14, 2015, 3:30 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added logics in LinuxFilesystemIsolator to recover and cleanup orphans.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.hpp 
> ab60f0ccffa06cb6f1913a6fd4fce8a2ccf6cd94 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
>   src/slave/paths.hpp 35b0439e89193b0933b33b67450008b0da9bbae7 
>   src/slave/paths.cpp f5697fb5bbb40064a55c4465210dcbdcd8630c87 
> 
> Diff: https://reviews.apache.org/r/38374/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38348: Correct typo in http.cpp

[Guangya Liu]

> On 九月 14, 2015, 5:27 p.m., Joseph Wu wrote:
> > That's not a typo; "iff" is short for "if and only if".

Thanks Joseph for pointing this oout. It is difficult for non native english 
speakers understand this, what about updating "iff" to "if and only if"?


- Guangya


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


On 九月 14, 2015, 8:48 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38348/
> ---
> 
> (Updated 九月 14, 2015, 8:48 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Correct typo in http.cpp
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 73e8857833603cb0edb77c352f343070ee1b8c80 
> 
> Diff: https://reviews.apache.org/r/38348/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 38364: Minor refactor for MesosContainerizer.

[Kapil Arya]

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

Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
1b83a8725b35435531038e37188b4c97189cef03 

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


Testing
---

make  check


Thanks,

Kapil Arya

Review Request 38363: Refactored container Launcher to accept namespaces dynamically.

[Kapil Arya]

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

Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.


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


Repository: mesos


Description
---

This prepares MesosContainerizer to allow isolators to specify namespaces for
each individual containerizer. Thus, allowing isolators to decide whether or not
to enable isolation for a particular container.


Diffs
-

  src/slave/containerizer/launcher.hpp 8cc832e99838683c14d8158bdf52a267299508c7 
  src/slave/containerizer/launcher.cpp 5267eecbdf31c88cd3c7dc7172b5aff7bac534b0 
  src/slave/containerizer/linux_launcher.hpp 
bf6bf3f27dac007429c224d4f6798ac68316accb 
  src/slave/containerizer/linux_launcher.cpp 
12acf4d08cb7fb1161820cf4bb480fd1bc2c6858 
  src/slave/containerizer/mesos/containerizer.cpp 
1b83a8725b35435531038e37188b4c97189cef03 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
ffa371fab69ce5eebe3b02afc2a1724a0f52110f 
  src/tests/containerizer/isolator_tests.cpp 
d34c82a7f24da6f60cfb22790f516dc6065b1f6f 
  src/tests/containerizer/launcher.hpp 6f020f94b042eef0e6a69ba8c26dfb697f3e81a3 

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


Testing
---

make check.


Thanks,

Kapil Arya

Review Request 38367: Added NetworkInfo message to ContainerInfo and TaskStatus.

[Kapil Arya]

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

Review request for mesos, Connor Doyle, Jie Yu, Niklas Nielsen, and Vinod Kone.


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


Repository: mesos


Description
---

This allows the frameworks to specify an intent to enable ip-per-container. The
IP information is supplied back to the framework as well as state.json endpoints
by including NetworkInfo inside TaskStatus.


Diffs
-

  include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
  src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60 
  src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156 
  src/slave/slave.cpp 5e5522e1254a5ed6084de36782753f9aad2894c4 
  src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 
  src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 

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


Testing
---

Added new tests and ran make check.


Thanks,

Kapil Arya

Re: Review Request 38366: Added helper to model Labels message for state.json.

[Kapil Arya]

> On Sept. 14, 2015, 5:40 p.m., Niklas Nielsen wrote:
> > High level comment; would it be worth change JSON::Protobuf() to support 
> > Labels instead?

Probably not. We are trying a shortcut when modelling Labels for state.json. 
Thus, I wouldn't want to change JSON::Protobuf which is supposed to translate 
Protobufs to JSONs.


- Kapil


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


On Sept. 14, 2015, 4:55 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38366/
> ---
> 
> (Updated Sept. 14, 2015, 4:55 p.m.)
> 
> 
> Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also updated Task modelling to show labels only if Task.has_labels() is true. 
> This way, the "labels" field won't shown if there are no labels. This makes 
> it consistent with the modelling of rest of the "optional" fields.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60 
>   src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156 
>   src/common/protobuf_utils.cpp 08612700c456017638a9978e5fe9cfa466294c46 
>   src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac 
>   src/tests/common/http_tests.cpp bf8712b11339b409514ab86c1f32eaf7e9c9a2f1 
> 
> Diff: https://reviews.apache.org/r/38366/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 38367: Added NetworkInfo message to ContainerInfo and TaskStatus.

[Niklas Nielsen]

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



src/slave/slave.cpp (line 2760)


s/the container IP address if/the container IP address with the IP from the 
agent PID, if/ or something that specifies where you are getting the IP from.

Would it be more precise to get the IP from the executor PID?



src/tests/master_tests.cpp (line 3293)


Can we const this?



src/tests/slave_tests.cpp (line 2269)


Can we const this string?



src/tests/slave_tests.cpp (line 2271)


Quite a long piece of straight line code; mind throwing in some comments 
breaking it up a bit and explaining what you are doing?


- Niklas Nielsen


On Sept. 14, 2015, 1:55 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38367/
> ---
> 
> (Updated Sept. 14, 2015, 1:55 p.m.)
> 
> 
> Review request for mesos, Connor Doyle, Jie Yu, Niklas Nielsen, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-3013
> https://issues.apache.org/jira/browse/MESOS-3013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows the frameworks to specify an intent to enable ip-per-container. 
> The
> IP information is supplied back to the framework as well as state.json 
> endpoints
> by including NetworkInfo inside TaskStatus.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
>   src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60 
>   src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156 
>   src/slave/slave.cpp 5e5522e1254a5ed6084de36782753f9aad2894c4 
>   src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 
>   src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 
> 
> Diff: https://reviews.apache.org/r/38367/diff/
> 
> 
> Testing
> ---
> 
> Added new tests and ran make check.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 38382: Fixed the perf event isolator to continue sampling in the presence of failures.

[Cong Wang]

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



src/slave/containerizer/isolators/cgroups/perf_event.cpp (line 411)


s/sampling will be halted// ?


- Cong Wang


On Sept. 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38382/
> ---
> 
> (Updated Sept. 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3423
> https://issues.apache.org/jira/browse/MESOS-3423
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See [MESOS-3423](https://issues.apache.org/jira/browse/MESOS-3423).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> 7dc8b7a99074b74ade019ef4df296780650a2e4e 
> 
> Diff: https://reviews.apache.org/r/38382/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 37283: Maintenance Primitives: Refactored Master maintenance test to use V1 API.

[Joseph Wu]

> On Sept. 14, 2015, 4:51 p.m., Qian Zhang wrote:
> > src/tests/master_maintenance_tests.cpp, line 418
> > 
> >
> > I checked v1/mesos.proto, and found regular offer also has 
> > unavailability:
> > ```
> > optional Unavailability unavailability = 9;
> > ```

That comment is saying, "the first set of offers in this test should not have 
`unavailability` set".

I think you misunderstood it to be: "offers don't have an `unavailability` 
field".


- Joseph


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


On Sept. 13, 2015, 1:33 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37283/
> ---
> 
> (Updated Sept. 13, 2015, 1:33 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> fb8dca3757a9565d5eb5a69eed10aa34602bb15c 
> 
> Diff: https://reviews.apache.org/r/37283/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 38386: Fix 'operator=' spacing in the registry_client.hpp

[Guangya Liu]

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

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


Repository: mesos


Description
---

Fix 'operator=' spacing in the registry_client.hpp


Diffs
-

  src/slave/containerizer/provisioners/docker/registry_client.hpp 
b5e28587bc9adc2c02805b13fbc5c693612c99fb 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37585: Maintenance primitives: Add a user doc.

[Joseph Wu]

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

(Updated Sept. 14, 2015, 2:32 p.m.)


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


Changes
---

One pass through the doc for Artem's comments.


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


Repository: mesos


Description
---

Heavily based on the design doc 
(https://docs.google.com/document/d/16k0lVwpSGVOyxPSyXKmGC-gbNmRlisNEe4p-fAUSojk/).

Includes a diagram of the maintenance mode transitions.

This documents the current working prototype (MVP), which starts here:
https://reviews.apache.org/r/36321/

One TODO remaining: Update with the release version that maintenance primitives 
will be released in.


Diffs (updated)
-

  docs/images/maintenance-primitives-modes.png PRE-CREATION 
  docs/maintenance.md PRE-CREATION 

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


Testing
---

Copied to: https://gist.github.com/kaysoky/b9789c88ee204e3b49a2
Checked for markdown correctness.


File Attachments


Same as the image in the binary diff. (Uploaded for reviewer convenience.)
  
https://reviews.apache.org/media/uploaded/files/2015/09/01/7d3153ca-37f4-4948-acce-b140a3eb71a9__maintenance-primitives-modes.png


Thanks,

Joseph Wu

Re: Review Request 38367: Added NetworkInfo message to ContainerInfo and TaskStatus.

[Vinod Kone]

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



include/mesos/mesos.proto (lines 1163 - 1164)


Do we have to say that this information is resolved at setup time? What 
about status changes during run time? I'm wondering whether we can leverage 
this for MESOS-3334.



include/mesos/mesos.proto (lines 1369 - 1370)


s/0/1/
s/1/2/

enum starting with "0" is bad because it is hard to distinguish the case 
when the enum is not set.



include/mesos/mesos.proto (line 1390)


s/. E.g./, e.g./



src/common/http.cpp (lines 153 - 194)


Why custom models? Can we not use the protobuf to json converters?



src/tests/master_tests.cpp (line 3247)


this test doesn't seem to test label values?



src/tests/master_tests.cpp (line 3320)


kill this?



src/tests/slave_tests.cpp (line 2223)


s/an is/is/ ?


- Vinod Kone


On Sept. 14, 2015, 8:55 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38367/
> ---
> 
> (Updated Sept. 14, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Connor Doyle, Jie Yu, Niklas Nielsen, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-3013
> https://issues.apache.org/jira/browse/MESOS-3013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows the frameworks to specify an intent to enable ip-per-container. 
> The
> IP information is supplied back to the framework as well as state.json 
> endpoints
> by including NetworkInfo inside TaskStatus.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
>   src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60 
>   src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156 
>   src/slave/slave.cpp 5e5522e1254a5ed6084de36782753f9aad2894c4 
>   src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 
>   src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 
> 
> Diff: https://reviews.apache.org/r/38367/diff/
> 
> 
> Testing
> ---
> 
> Added new tests and ran make check.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Review Request 38374: Added logics in LinuxFilesystemIsolator to recover and cleanup orphans.

[Jie Yu]

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

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


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


Repository: mesos


Description
---

Added logics in LinuxFilesystemIsolator to recover and cleanup orphans.


Diffs
-

  src/slave/containerizer/isolators/filesystem/linux.hpp 
ab60f0ccffa06cb6f1913a6fd4fce8a2ccf6cd94 
  src/slave/containerizer/isolators/filesystem/linux.cpp 
0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
  src/slave/paths.hpp 35b0439e89193b0933b33b67450008b0da9bbae7 
  src/slave/paths.cpp f5697fb5bbb40064a55c4465210dcbdcd8630c87 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 38378: Minor cleanup in perf_tests.cpp.

[Ben Mahler]

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

(Updated Sept. 14, 2015, 11:08 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Preserve original test semantics per Cong's feedback.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/containerizer/perf_tests.cpp 
bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38 

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


Testing
---

make check


Thanks,

Ben Mahler

Re: Review Request 38364: Minor refactor for MesosContainerizer.

[Niklas Nielsen]

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

Ship it!



src/slave/containerizer/mesos/containerizer.cpp (lines 830 - 833)


How about moving this up as well?


- Niklas Nielsen


On Sept. 14, 2015, 2:54 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38364/
> ---
> 
> (Updated Sept. 14, 2015, 2:54 p.m.)
> 
> 
> Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3387
> https://issues.apache.org/jira/browse/MESOS-3387
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change moves two different pieces of code, that each iterate over list 
> of ContainerPrepareInfos, close together for readability. It becomes even 
> more relevant when looking at https://reviews.apache.org/r/38365/ that 
> iterates over yet another member from ContainerPrepareInfo.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 1b83a8725b35435531038e37188b4c97189cef03 
> 
> Diff: https://reviews.apache.org/r/38364/diff/
> 
> 
> Testing
> ---
> 
> make  check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 38379: Handle empty set of cgroups as a no-op in perf::sample.

[Cong Wang]

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


Not sure how much value this patch has, because upper layer always calls it 
with non-empty cgroups. Also, you probably want to check for events.empty() too 
while you are on it.

- Cong Wang


On Sept. 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38379/
> ---
> 
> (Updated Sept. 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Handle empty set of cgroups as a no-op in perf::sample.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 0011482cf9d920485728798518d32af0e9627724 
>   src/tests/containerizer/perf_tests.cpp 
> bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38 
> 
> Diff: https://reviews.apache.org/r/38379/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 38378: Minor cleanup in perf_tests.cpp.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Sept. 14, 2015, 11:08 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38378/
> ---
> 
> (Updated Sept. 14, 2015, 11:08 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/perf_tests.cpp 
> bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38 
> 
> Diff: https://reviews.apache.org/r/38378/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 38377: Removed 'using namespace process' in perf event isolator.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Sept. 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38377/
> ---
> 
> (Updated Sept. 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> 7dc8b7a99074b74ade019ef4df296780650a2e4e 
> 
> Diff: https://reviews.apache.org/r/38377/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 37024: Exposes mesos version information in components.

[Ben Mahler]

> On Aug. 31, 2015, 10:47 p.m., Ben Mahler wrote:
> > Couple of comments:
> > 
> > Let's not call the actor VersionInfo, since that sounds like a protobuf 
> > message. How about 'VersionProcess' and we avoid the wrapper type entirely 
> > since Version is taken in stout?
> > 
> > Also, can we instantiate this from the main entrypoints rather than inside 
> > master/slave? Would be nice to offer it from the drivers as well (sched.cpp 
> > / exec.cpp).
> 
> Ben Mahler wrote:
> Could you also follow up with a test for this in a separate patch?
> 
> haosdent huang wrote:
> Sure
> 
> haosdent huang wrote:
> @bmahler Sorry for a bit unclear you reviews before.
> * If change VersionInfo -> VersionProcess, VersionInfoProcess seems don't 
> have meaningful names.
> * "the main entrypoints" refer to the 
> Master::initialize()/Slave::initialize()?

VersionInfo was referring to the protobuf message we may add later (much like 
SlaveInfo, MasterInfo, these follow the _Info pattern).

I'm suggesting only having a VersionProcess class, which is a Process, no need 
for the wrapper object (the one you've currently called VersionInfo is a 
"wrapper" of VersionInfoProcess", let's just avoid the wrapper for now).


- Ben


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


On Aug. 16, 2015, 5:38 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
> 
> (Updated Aug. 16, 2015, 5:38 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add an endpoint that exposes component version.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor.hpp 72eca97dd84fb1300b37764a3ef3a57fb5e676c2 
>   include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd 
>   src/Makefile.am 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
>   src/exec/exec.cpp 31e0c2f17a9092d18285828111d27628fb07bc02 
>   src/local/local.cpp 4d98bf23705027f3ba0cbb571289f21b288fe7db 
>   src/master/main.cpp bafc605d6c20bd264b932e44ee80373a3f692734 
>   src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 
>   src/slave/main.cpp 364dc7fc7ab2e3cef01aea7267dafa014b60e2b9 
>   src/version/version_info.hpp PRE-CREATION 
>   src/version/version_info.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37024/diff/
> 
> 
> Testing
> ---
> 
> Manual test result:
> 
> ```
> $ curl http://localhost:5050/version 2>/dev/null|jq .
> 
> {
>   "version": "0.24.0",
>   "build_user": "haosdent",
>   "build_time": 1439702338,
>   "build_date": "2015-08-16 13:18:58"
> }
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37283: Maintenance Primitives: Refactored Master maintenance test to use V1 API.

[Qian Zhang]

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



src/tests/master_maintenance_tests.cpp (line 405)


I checked v1/mesos.proto, and found regular offer also has unavailability:
```
optional Unavailability unavailability = 9;
```


- Qian Zhang


On Sept. 14, 2015, 4:33 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37283/
> ---
> 
> (Updated Sept. 14, 2015, 4:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> fb8dca3757a9565d5eb5a69eed10aa34602bb15c 
> 
> Diff: https://reviews.apache.org/r/37283/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 38376: Fix 'operator=' spacing in the V1 Scheduler interface.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 九月 14, 2015, 10:49 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38376/
> ---
> 
> (Updated 九月 14, 2015, 10:49 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix 'operator=' spacing in the V1 Scheduler interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/scheduler.hpp 7911da0bf0bd571dcdc440aa2aa6f2ea8f9e8b67 
> 
> Diff: https://reviews.apache.org/r/38376/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Jie Yu]

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



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


Two lines apart please. Please do a sweep to fix those.



src/slave/containerizer/provisioners/docker/provisioner.cpp (lines 101 - 110)


Could you please follow the code in AppcProvisioner to get the realpath of 
'rootDir' here?

Eventually, I think we should unify the implementation of the Provisioner! 
Looking at the code here, more than 90% the code are copied! That's too bad!



src/slave/containerizer/provisioners/docker/provisioner.cpp (lines 247 - 273)


The recovery logic has been changed slightly. Please make sure the recovery 
logic here is the same as that in AppcProvisioner.

Honestly, I think it might be easier to just unify the Provisioner (instead 
of copying the code). I don't think there'll be too much additional work. I can 
help you with this.



src/slave/containerizer/provisioners/docker/provisioner.cpp (lines 376 - 379)


The logic here also changes in AppcProvisioner. Please make sure it's in 
sync. Or just unify the Provisioner.


- Jie Yu


On Sept. 11, 2015, 7:42 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 11, 2015, 7:42 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, Till Toenshoff, and Jiang 
> Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Joining all the commits around provisioner and local store into one review so 
> it's easier to review, as patches
> are changing code on top of each other.
> 
> All the commits are going to committed together.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8963cea 
>   src/slave/containerizer/provisioner.hpp 9e0e0b8 
>   src/slave/containerizer/provisioner.cpp 2ac9008 
>   src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/provisioner.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/provisioner.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 799c963 
>   src/slave/flags.cpp b676bac 
>   src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 37585: Maintenance primitives: Add a user doc.

[Joseph Wu]

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

(Updated Sept. 14, 2015, 2:32 p.m.)


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


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


Repository: mesos


Description (updated)
---

Heavily based on the design doc 
(https://docs.google.com/document/d/16k0lVwpSGVOyxPSyXKmGC-gbNmRlisNEe4p-fAUSojk/).

Includes a diagram of the maintenance mode transitions.


Diffs
-

  docs/images/maintenance-primitives-modes.png PRE-CREATION 
  docs/maintenance.md PRE-CREATION 

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


Testing
---

Copied to: https://gist.github.com/kaysoky/b9789c88ee204e3b49a2
Checked for markdown correctness.


File Attachments


Same as the image in the binary diff. (Uploaded for reviewer convenience.)
  
https://reviews.apache.org/media/uploaded/files/2015/09/01/7d3153ca-37f4-4948-acce-b140a3eb71a9__maintenance-primitives-modes.png


Thanks,

Joseph Wu

Re: Review Request 38366: Added helper to model Labels message for state.json.

[Niklas Nielsen]

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


High level comment; would it be worth change JSON::Protobuf() to support Labels 
instead?

- Niklas Nielsen


On Sept. 14, 2015, 1:55 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38366/
> ---
> 
> (Updated Sept. 14, 2015, 1:55 p.m.)
> 
> 
> Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also updated Task modelling to show labels only if Task.has_labels() is true. 
> This way, the "labels" field won't shown if there are no labels. This makes 
> it consistent with the modelling of rest of the "optional" fields.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60 
>   src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156 
>   src/common/protobuf_utils.cpp 08612700c456017638a9978e5fe9cfa466294c46 
>   src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac 
>   src/tests/common/http_tests.cpp bf8712b11339b409514ab86c1f32eaf7e9c9a2f1 
> 
> Diff: https://reviews.apache.org/r/38366/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 38364: Minor refactor for MesosContainerizer.

[Niklas Nielsen]

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


Can you expand on the change (before and after) effect in the RR description? 
Hard to tell from the current title and description.

- Niklas Nielsen


On Sept. 14, 2015, 1:55 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38364/
> ---
> 
> (Updated Sept. 14, 2015, 1:55 p.m.)
> 
> 
> Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3387
> https://issues.apache.org/jira/browse/MESOS-3387
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 1b83a8725b35435531038e37188b4c97189cef03 
> 
> Diff: https://reviews.apache.org/r/38364/diff/
> 
> 
> Testing
> ---
> 
> make  check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Review Request 38376: Fix 'operator=' spacing in the V1 Scheduler interface.

[Joseph Wu]

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

Review request for mesos and Joris Van Remoortere.


Repository: mesos


Description
---

Fix 'operator=' spacing in the V1 Scheduler interface.


Diffs
-

  include/mesos/v1/scheduler.hpp 7911da0bf0bd571dcdc440aa2aa6f2ea8f9e8b67 

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


Testing
---

`make check`


Thanks,

Joseph Wu

Re: Review Request 38375: Fixed process::collect and process::await to do discard propagation.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Sept. 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38375/
> ---
> 
> (Updated Sept. 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3426
> https://issues.apache.org/jira/browse/MESOS-3426
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed process::collect and process::await to do discard propagation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 6361ac6485760fdeed87a2c049d6431783989462 
>   3rdparty/libprocess/include/process/collect.hpp 
> d07b6860a5b5cbed910b2e9e2ab2429bd70a3999 
>   3rdparty/libprocess/src/tests/CMakeLists.txt 
> b61ca1e5be2054c1156205c62a1e8311a575bfa5 
>   3rdparty/libprocess/src/tests/collect_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 435663b10c1bfce07e8e84719aa14b5867c651c6 
> 
> Diff: https://reviews.apache.org/r/38375/diff/
> 
> 
> Testing
> ---
> 
> Pulled out collect_tests.cpp and added a 'DiscardPropagation' test for both 
> collect and await.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 38380: Minor cleanups in the perf event isolator.

[Ben Mahler]

> On Sept. 14, 2015, 11:48 p.m., Cong Wang wrote:
> > src/slave/containerizer/isolators/cgroups/perf_event.cpp, line 374
> > 
> >
> > I don't think cgroup can be destroyed while perf running, kernel should 
> > return EBUSY, no?

I'll remove the 'while' addition here, thanks!


> On Sept. 14, 2015, 11:48 p.m., Cong Wang wrote:
> > src/slave/containerizer/isolators/cgroups/perf_event.cpp, line 436
> > 
> >
> > Just nit: the original comment is useful for understanding the code, 
> > event after this change.

Good to know, I personally didn't find it insightful, but I'll add back 
something.


- Ben


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


On Sept. 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38380/
> ---
> 
> (Updated Sept. 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This removes the special casing of the empty set of cgroups case to simplify 
> the sample loop, along with some other minor cleanups.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> 7dc8b7a99074b74ade019ef4df296780650a2e4e 
> 
> Diff: https://reviews.apache.org/r/38380/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 38382: Fixed the perf event isolator to continue sampling in the presence of failures.

[Ben Mahler]

> On Sept. 14, 2015, 11:38 p.m., Cong Wang wrote:
> > src/slave/containerizer/isolators/cgroups/perf_event.cpp, line 413
> > 
> >
> > s/sampling will be halted// ?

Great catch, thank you!


- Ben


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


On Sept. 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38382/
> ---
> 
> (Updated Sept. 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3423
> https://issues.apache.org/jira/browse/MESOS-3423
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See [MESOS-3423](https://issues.apache.org/jira/browse/MESOS-3423).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> 7dc8b7a99074b74ade019ef4df296780650a2e4e 
> 
> Diff: https://reviews.apache.org/r/38382/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 38289: Handle bad request in Docker registry client.

[Timothy Chen]

> On Sept. 14, 2015, 6:11 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 277
> > 
> >
> > Why are we parsing the error JSON to extract the error string from JSON 
> > here and not dump the entire error string to the end-user. Is the rationale 
> > that this makes the error messages readable and more consistent ?
> > 
> > However, the cons to this are:
> > 1. We don't seem to be following this design any-where else in our 
> > code-base ?
> > 2. We have already omitted fields `code` and `detail` from the error 
> > response ( http://docs.docker.com/registry/spec/api/ ). If Docker adds more 
> > fields in the future or deletes/renames some of them, we would need to 
> > revisit them again?
> > 
> > Won't providing the entire JSON to the end-user be more helpful in 
> > identifying the root-case and helping him/her resolve the issue faster ?

I think this is valid, but at the same time the JSON text is fairly verbose I'm 
was not inclined to print the whole JSON text in the slave log. The message 
according to the spec actually holds all the details we need to print.
I think you do bring some great points, that we're tied with the spec and we'll 
need to revisit as time goes on.
I think for now let's keep this as we're already tied with the spec, and we can 
change things moving forward as this is just one of the vey first patch of the 
client.


> On Sept. 14, 2015, 6:11 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 268
> > 
> >
> > Can we just do BadRequest().status here to eliminate the hard-coded 
> > string constant  ?

Good idea, I was wondering if we had to do this. I'll do this another review.


> On Sept. 14, 2015, 6:11 p.m., Anand Mazumdar wrote:
> > src/Makefile.am, line 1694
> > 
> >
> > Can we do this renaming/moving test files in a separate patch ? This 
> > looks un-related to handling BadRequest in the Registry Client. What do you 
> > think ?

Sorry it's already merged :)


- Timothy


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


On Sept. 11, 2015, 7:34 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38289/
> ---
> 
> (Updated Sept. 11, 2015, 7:34 p.m.)
> 
> 
> Review request for mesos and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Handle bad request in Docker registry client.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8963cea9fd7e3bee450a1059e6383e5ab868a17b 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp 
> ff29d562c7f1bd5f0579e97cdbd60c2b2f36329e 
> 
> Diff: https://reviews.apache.org/r/38289/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38363: Refactored container Launcher to accept namespaces dynamically.

[Niklas Nielsen]

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



src/tests/containerizer/isolator_tests.cpp (line 202)


Does it make sense to have None() as the default value?



src/tests/containerizer/isolator_tests.cpp (line 506)


Have you verified the status (non-error) of namespaces() before you get 
here?


- Niklas Nielsen


On Sept. 14, 2015, 1:55 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38363/
> ---
> 
> (Updated Sept. 14, 2015, 1:55 p.m.)
> 
> 
> Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3387
> https://issues.apache.org/jira/browse/MESOS-3387
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This prepares MesosContainerizer to allow isolators to specify namespaces for
> each individual containerizer. Thus, allowing isolators to decide whether or 
> not
> to enable isolation for a particular container.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/launcher.hpp 
> 8cc832e99838683c14d8158bdf52a267299508c7 
>   src/slave/containerizer/launcher.cpp 
> 5267eecbdf31c88cd3c7dc7172b5aff7bac534b0 
>   src/slave/containerizer/linux_launcher.hpp 
> bf6bf3f27dac007429c224d4f6798ac68316accb 
>   src/slave/containerizer/linux_launcher.cpp 
> 12acf4d08cb7fb1161820cf4bb480fd1bc2c6858 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 1b83a8725b35435531038e37188b4c97189cef03 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> ffa371fab69ce5eebe3b02afc2a1724a0f52110f 
>   src/tests/containerizer/isolator_tests.cpp 
> d34c82a7f24da6f60cfb22790f516dc6065b1f6f 
>   src/tests/containerizer/launcher.hpp 
> 6f020f94b042eef0e6a69ba8c26dfb697f3e81a3 
> 
> Diff: https://reviews.apache.org/r/38363/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 38102: MESOS-3046 (Stout's UUID re-seeds a new random generator during each call to UUID::random)

[Ben Mahler]

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



3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp (line 31)


Please use THREAD_LOCAL from stout/thread_local.hpp and store a heap object 
as done in all of our other uses of THREAD_LOCAL.


- Ben Mahler


On Sept. 5, 2015, 3:08 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38102/
> ---
> 
> (Updated Sept. 5, 2015, 3:08 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3046
> https://issues.apache.org/jira/browse/MESOS-3046
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> __Phenomenon:__
> Performance downgrade
> 
> __Root Cause:__
> stout's UUID abstraction is re-seeding the random generator during each call 
> to UUID::random(), which is really expensive.
> 
> __Solution:__
> Seeding the random generator only once per thread.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp 
> e90dabb0c572923a50490ecb17867dc50c6d161d 
> 
> Diff: https://reviews.apache.org/r/38102/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38381: Removed hard-coded reap interval from perf event isolator.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Sept. 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38381/
> ---
> 
> (Updated Sept. 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3423
> https://issues.apache.org/jira/browse/MESOS-3423
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> 7dc8b7a99074b74ade019ef4df296780650a2e4e 
> 
> Diff: https://reviews.apache.org/r/38381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 38379: Handle empty set of cgroups as a no-op in perf::sample.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Sept. 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38379/
> ---
> 
> (Updated Sept. 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Handle empty set of cgroups as a no-op in perf::sample.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 0011482cf9d920485728798518d32af0e9627724 
>   src/tests/containerizer/perf_tests.cpp 
> bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38 
> 
> Diff: https://reviews.apache.org/r/38379/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 38380: Minor cleanups in the perf event isolator.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Sept. 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38380/
> ---
> 
> (Updated Sept. 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This removes the special casing of the empty set of cgroups case to simplify 
> the sample loop, along with some other minor cleanups.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> 7dc8b7a99074b74ade019ef4df296780650a2e4e 
> 
> Diff: https://reviews.apache.org/r/38380/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 38333: Made container sandbox a shared mount to address MESOS-3349.

[Jie Yu]

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

(Updated Sept. 14, 2015, 10:29 p.m.)


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


Changes
---

Addressed comments. NNFR.


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


Repository: mesos


Description
---

Made container sandbox a shared mount to address MESOS-3349.

See the discussion in https://reviews.apache.org/r/38329/ for more context.

The idea is to mark container sandbox a shared mount (do a self bind mount 
first) so that persistent volume mounts can be propagated.

This is less invasive than marking '/' as a shared mount.

One followup for this patch is to set the default filesystem isolator to posix 
as the linux isolator will manipulate host mount table.

We also need to address the TODO in `LinuxFilesystemIsolator::_recover` so that 
tests do not leak mounts in the host mount table.


Diffs (updated)
-

  src/slave/containerizer/isolators/filesystem/linux.cpp 
0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
  src/slave/containerizer/provisioners/appc/provisioner.cpp 
cd29a00fa0db8af294c10bb7a2e0cb4252bd2993 
  src/slave/containerizer/provisioners/backends/bind.cpp 
1cdae61786790dc6a475ae5f73c8cc92d2bbf739 

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


Testing
---

sudo make check on Centos5 and Centos6


Thanks,

Jie Yu

Re: Review Request 38378: Minor cleanup in perf_tests.cpp.

[Cong Wang]

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



src/tests/containerizer/perf_tests.cpp (line 51)


You are changing the semantic, the original test case is to verify a list 
of events which contains one invalid event, you changed to to verify just one 
invalid event. They are different. The original one makes more sense to me.


- Cong Wang


On Sept. 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38378/
> ---
> 
> (Updated Sept. 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/perf_tests.cpp 
> bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38 
> 
> Diff: https://reviews.apache.org/r/38378/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37168]

All tests passed.

- Mesos ReviewBot


On Sept. 14, 2015, 1:48 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> ---
> 
> (Updated Sept. 14, 2015, 1:48 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3063
> https://issues.apache.org/jira/browse/MESOS-3063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Provide example for dynamic reservation features.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8963cea 
>   src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
>   src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
>   src/tests/examples_tests.cpp 3f56b30 
>   src/tests/flags.hpp 06da36d 
>   src/tests/script.cpp bcc1fab 
> 
> Diff: https://reviews.apache.org/r/37168/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 37969: Maintenance primitives: Tweak validation error messages to return JSON rather than protobuf.

[Joseph Wu]

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

(Updated Sept. 14, 2015, 5:10 p.m.)


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


Changes
---

Rebase.


Repository: mesos


Description
---

Replaces `__.DebugString()` with `stringify(JSON::Protobuf(__))`, which looks 
nicer and matches the JSON expected by the HTTP endpoints.

Addresses a comment found here: 
https://reviews.apache.org/r/37358/#comment152513


Diffs (updated)
-

  src/master/http.cpp f7ce9aa56b453c1d37b99dd836d956ab292ab62e 
  src/master/maintenance.cpp 87308a659db05f0676bd02a56ff41fe9d953ba71 

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


Testing
---

`make check`

This change will tweak the output from:
```
Machine 'hostname: "myComputer"
' is not valid because ...

```
(`DebugString` ^ puts a newline there.)

To: 
```
Machine '{"hostname":"myComputer"}' is not valid because ...
```


Thanks,

Joseph Wu

Review Request 38366: Added helper to model Labels message for state.json.

[Kapil Arya]

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

Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.


Repository: mesos


Description
---

Also updated Task modelling to show labels only if Task.has_labels() is true. 
This way, the "labels" field won't shown if there are no labels. This makes it 
consistent with the modelling of rest of the "optional" fields.


Diffs
-

  src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60 
  src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156 
  src/common/protobuf_utils.cpp 08612700c456017638a9978e5fe9cfa466294c46 
  src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac 
  src/tests/common/http_tests.cpp bf8712b11339b409514ab86c1f32eaf7e9c9a2f1 

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


Testing
---

make check


Thanks,

Kapil Arya

Review Request 38368: Replaced slaveTaskStatusLabelDecorator hook with slaveTaskStatusDecorator hook.

[Kapil Arya]

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

Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.


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


Repository: mesos


Description
---

This allows the hook to not only update TaskStatus::labels, but also
TaskStatus::container_status.

Enhanced example hook module and relevant test to reflect the change.


Diffs
-

  include/mesos/hook.hpp d90baccba4ac73eb777c8848e40ba737e756032f 
  src/examples/test_hook_module.cpp bc13a8ad0308668f31310b3aa65243bfb41b87b5 
  src/hook/manager.hpp 30d8321f459cacdfc0397ab7cd4e81710655351a 
  src/hook/manager.cpp 754c238fcf728d6aa5b897ed5d9f46c251345334 
  src/slave/slave.cpp 5e5522e1254a5ed6084de36782753f9aad2894c4 
  src/tests/common/http_tests.cpp bf8712b11339b409514ab86c1f32eaf7e9c9a2f1 
  src/tests/hook_tests.cpp deb4343089a30073e8f1f811731b075f7d968846 

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


Testing
---

make check.


Thanks,

Kapil Arya

Review Request 38365: Updated Isolator::prepare to return list of required namespaces.

[Kapil Arya]

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

Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.


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


Repository: mesos


Description
---

This allows the Isolators to decide whether or not to provide resource isolation
on a per-container level.


Diffs
-

  include/mesos/slave/isolator.hpp 4db23dc242039607e6444d15aa800207b415ca02 
  include/mesos/slave/isolator.proto 12ea6ac3552c70a172ae9e8506f4b5d96457a3ec 
  src/slave/containerizer/isolator.hpp fbb7c8ab908192ae64f34e466c0c24705b3a134b 
  src/slave/containerizer/isolator.cpp 7973100ea1a58938c50962120b9ecb6722b2ee4e 
  src/slave/containerizer/isolators/filesystem/linux.hpp 
ab60f0ccffa06cb6f1913a6fd4fce8a2ccf6cd94 
  src/slave/containerizer/isolators/filesystem/linux.cpp 
0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
  src/slave/containerizer/isolators/filesystem/shared.hpp 
a21bc79d342ece50c4924fc0ebd2186e57b3e899 
  src/slave/containerizer/isolators/filesystem/shared.cpp 
4b4520e30ce1d1818bd3a13260f6dd55ab3900c9 
  src/slave/containerizer/isolators/namespaces/pid.hpp 
b22f5ba8e3743bb243ed2c5d204ab4ba21088630 
  src/slave/containerizer/isolators/namespaces/pid.cpp 
35cb6645c9abc0cf533b844e2b2cccf4374bfd68 
  src/slave/containerizer/isolators/network/port_mapping.hpp 
4bca0b81bf69fb4cd75e05aacd02d3e818e32d09 
  src/slave/containerizer/isolators/network/port_mapping.cpp 
34ba2294b0bd7d57aa9de073692a2ea8ec62681d 
  src/slave/containerizer/mesos/containerizer.cpp 
1b83a8725b35435531038e37188b4c97189cef03 
  src/tests/containerizer/isolator_tests.cpp 
d34c82a7f24da6f60cfb22790f516dc6065b1f6f 

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


Testing
---

make check


Thanks,

Kapil Arya

Re: Review Request 38370: Updated docker executor to set container IP in TaskStatus::NetworkInfo.

[Timothy Chen]

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



src/docker/executor.cpp (line 162)


Seems odd to have other tools referenced here in Mesos codebase. And also I 
think that even if Mesos-DNS migrates to use labels, are we expecting all users 
to force upgrade as their old version won't work with latest Mesos?


- Timothy Chen


On Sept. 14, 2015, 8:55 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38370/
> ---
> 
> (Updated Sept. 14, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos, Connor Doyle, Jie Yu, Niklas Nielsen, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-3013
> https://issues.apache.org/jira/browse/MESOS-3013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated docker executor to set container IP in TaskStatus::NetworkInfo.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 6647075b55e5a79264e3556bb906a1f26a2d673e 
> 
> Diff: https://reviews.apache.org/r/38370/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 38368: Replaced slaveTaskStatusLabelDecorator hook with slaveTaskStatusDecorator hook.

[Niklas Nielsen]

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



src/examples/test_hook_module.cpp (line 197)


Mind throwing in a comment about this test step?



src/examples/test_hook_module.cpp (line 198)


How about setting the protocol (for testing)? So maybe have two network 
infos (one with each)?



src/slave/slave.cpp (lines 2757 - 2765)


Can we either add a warning if anything else is changed (probably hard) or 
at least throw in a comment, that we only apply changes from container status 
and labels?



src/tests/hook_tests.cpp (line 513)


Mind preceeding this test code blob with a comment about the setup?


- Niklas Nielsen


On Sept. 14, 2015, 1:55 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38368/
> ---
> 
> (Updated Sept. 14, 2015, 1:55 p.m.)
> 
> 
> Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3013
> https://issues.apache.org/jira/browse/MESOS-3013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows the hook to not only update TaskStatus::labels, but also
> TaskStatus::container_status.
> 
> Enhanced example hook module and relevant test to reflect the change.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp d90baccba4ac73eb777c8848e40ba737e756032f 
>   src/examples/test_hook_module.cpp bc13a8ad0308668f31310b3aa65243bfb41b87b5 
>   src/hook/manager.hpp 30d8321f459cacdfc0397ab7cd4e81710655351a 
>   src/hook/manager.cpp 754c238fcf728d6aa5b897ed5d9f46c251345334 
>   src/slave/slave.cpp 5e5522e1254a5ed6084de36782753f9aad2894c4 
>   src/tests/common/http_tests.cpp bf8712b11339b409514ab86c1f32eaf7e9c9a2f1 
>   src/tests/hook_tests.cpp deb4343089a30073e8f1f811731b075f7d968846 
> 
> Diff: https://reviews.apache.org/r/38368/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 38365: Updated Isolator::prepare to return list of required namespaces.

[Niklas Nielsen]

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

Ship it!



include/mesos/slave/isolator.proto (line 75)


Taken we still wrap comments at 70, this needs to be wrapped :)


- Niklas Nielsen


On Sept. 14, 2015, 1:55 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38365/
> ---
> 
> (Updated Sept. 14, 2015, 1:55 p.m.)
> 
> 
> Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3387
> https://issues.apache.org/jira/browse/MESOS-3387
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows the Isolators to decide whether or not to provide resource 
> isolation
> on a per-container level.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.hpp 4db23dc242039607e6444d15aa800207b415ca02 
>   include/mesos/slave/isolator.proto 12ea6ac3552c70a172ae9e8506f4b5d96457a3ec 
>   src/slave/containerizer/isolator.hpp 
> fbb7c8ab908192ae64f34e466c0c24705b3a134b 
>   src/slave/containerizer/isolator.cpp 
> 7973100ea1a58938c50962120b9ecb6722b2ee4e 
>   src/slave/containerizer/isolators/filesystem/linux.hpp 
> ab60f0ccffa06cb6f1913a6fd4fce8a2ccf6cd94 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
>   src/slave/containerizer/isolators/filesystem/shared.hpp 
> a21bc79d342ece50c4924fc0ebd2186e57b3e899 
>   src/slave/containerizer/isolators/filesystem/shared.cpp 
> 4b4520e30ce1d1818bd3a13260f6dd55ab3900c9 
>   src/slave/containerizer/isolators/namespaces/pid.hpp 
> b22f5ba8e3743bb243ed2c5d204ab4ba21088630 
>   src/slave/containerizer/isolators/namespaces/pid.cpp 
> 35cb6645c9abc0cf533b844e2b2cccf4374bfd68 
>   src/slave/containerizer/isolators/network/port_mapping.hpp 
> 4bca0b81bf69fb4cd75e05aacd02d3e818e32d09 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 34ba2294b0bd7d57aa9de073692a2ea8ec62681d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 1b83a8725b35435531038e37188b4c97189cef03 
>   src/tests/containerizer/isolator_tests.cpp 
> d34c82a7f24da6f60cfb22790f516dc6065b1f6f 
> 
> Diff: https://reviews.apache.org/r/38365/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Review Request 38381: Removed hard-coded reap interval from perf event isolator.

[Ben Mahler]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/containerizer/isolators/cgroups/perf_event.cpp 
7dc8b7a99074b74ade019ef4df296780650a2e4e 

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


Testing
---

make check


Thanks,

Ben Mahler

Review Request 38377: Removed 'using namespace process' in perf event isolator.

[Ben Mahler]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/containerizer/isolators/cgroups/perf_event.cpp 
7dc8b7a99074b74ade019ef4df296780650a2e4e 

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


Testing
---

make check


Thanks,

Ben Mahler

Review Request 38379: Handle empty set of cgroups as a no-op in perf::sample.

[Ben Mahler]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Handle empty set of cgroups as a no-op in perf::sample.


Diffs
-

  src/linux/perf.cpp 0011482cf9d920485728798518d32af0e9627724 
  src/tests/containerizer/perf_tests.cpp 
bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38 

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


Testing
---

Added a test.


Thanks,

Ben Mahler

Re: Review Request 38379: Handle empty set of cgroups as a no-op in perf::sample.

[Ben Mahler]

> On Sept. 14, 2015, 11:21 p.m., Cong Wang wrote:
> > Not sure how much value this patch has, because upper layer always calls it 
> > with non-empty cgroups. Also, you probably want to check for events.empty() 
> > too while you are on it.

This was done for https://reviews.apache.org/r/38380/, which removes the 
special case logic for empty set.


- Ben


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


On Sept. 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38379/
> ---
> 
> (Updated Sept. 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Handle empty set of cgroups as a no-op in perf::sample.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 0011482cf9d920485728798518d32af0e9627724 
>   src/tests/containerizer/perf_tests.cpp 
> bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38 
> 
> Diff: https://reviews.apache.org/r/38379/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 38382: Fixed the perf event isolator to continue sampling in the presence of failures.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Sept. 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38382/
> ---
> 
> (Updated Sept. 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3423
> https://issues.apache.org/jira/browse/MESOS-3423
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See [MESOS-3423](https://issues.apache.org/jira/browse/MESOS-3423).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> 7dc8b7a99074b74ade019ef4df296780650a2e4e 
> 
> Diff: https://reviews.apache.org/r/38382/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 38374: Added logics in LinuxFilesystemIsolator to recover and cleanup orphans.

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Sept. 14, 2015, 10:30 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38374/
> ---
> 
> (Updated Sept. 14, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added logics in LinuxFilesystemIsolator to recover and cleanup orphans.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.hpp 
> ab60f0ccffa06cb6f1913a6fd4fce8a2ccf6cd94 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
>   src/slave/paths.hpp 35b0439e89193b0933b33b67450008b0da9bbae7 
>   src/slave/paths.cpp f5697fb5bbb40064a55c4465210dcbdcd8630c87 
> 
> Diff: https://reviews.apache.org/r/38374/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38381: Removed hard-coded reap interval from perf event isolator.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 九月 14, 2015, 10:57 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38381/
> ---
> 
> (Updated 九月 14, 2015, 10:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-3423
> https://issues.apache.org/jira/browse/MESOS-3423
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> 7dc8b7a99074b74ade019ef4df296780650a2e4e 
> 
> Diff: https://reviews.apache.org/r/38381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 38376: Fix 'operator=' spacing in the V1 Scheduler interface.

[Anand Mazumdar]

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

Ship it!


Ship It!

- Anand Mazumdar


On Sept. 14, 2015, 10:49 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38376/
> ---
> 
> (Updated Sept. 14, 2015, 10:49 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix 'operator=' spacing in the V1 Scheduler interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/scheduler.hpp 7911da0bf0bd571dcdc440aa2aa6f2ea8f9e8b67 
> 
> Diff: https://reviews.apache.org/r/38376/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37540: Add perf event API

[Vinod Kone]

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


my main question is regarding division of responsibilities between the wrapper 
class and the process class. Looks like opening the fd and mmaping is done by 
the wrapper and the rest by the process. Can everything be done by the process?


src/linux/perf.hpp (line 92)


const &



src/linux/perf.hpp (line 95)


s/, returns/. Returns/



src/linux/perf.hpp (line 96)


s/,/;/

s/cgroups/cgroup names/ ?



src/linux/perf.hpp (line 99)


const



src/linux/perf.hpp (line 126)


const &



src/linux/perf.cpp (line 508)


s/, after/; after/



src/linux/perf.cpp (line 562)


sharing the map between this class and the constituent process class is 
unfortunate. what happens if the process class deletes the pointer?

is it possible to have the process class create and own the map?



src/linux/perf.cpp (line 564)


don't think you need CHECK_NOTNULL here because you are setting 'process' 
variable right above.

also indent by 2 spaces instead of 4.



src/linux/perf.cpp (lines 605 - 610)


indent these by 4 spaces from the beginning of the column (i.e., 4 spaces 
from 'void').



src/linux/perf.cpp (line 628)


CHECK_NOTNULL(attr); ?



src/linux/perf.cpp (line 660)


CHECK_NOTNULL(attr)?



src/linux/perf.cpp (line 699)


The fd returned by os::open() above is the pid of the cgroup? It's worth 
adding a comment.



src/linux/perf.cpp (line 746)


is there any reason why we want close() as part of the public API? sounds 
like users can simply call delete on the PerfEvent object? i would recommend 
removing this method or making it private.

more importantly, what happens if close() is called twice? is that safe?



src/linux/perf.cpp (lines 748 - 751)


can this be done by the process instead?



src/linux/perf.cpp (line 754)


default is 'true'. so no need to explicitly specify it.


- Vinod Kone


On Sept. 4, 2015, 11:13 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> ---
> 
> (Updated Sept. 4, 2015, 11:13 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Abstract Linux kernel perf event API and provide API to collect schedule 
> events.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5 
>   src/linux/perf.cpp 0011482cf9d920485728798518d32af0e9627724 
>   src/tests/containerizer/perf_tests.cpp 
> bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38 
> 
> Diff: https://reviews.apache.org/r/37540/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 38386: Fix 'operator=' spacing in the registry_client.hpp

[Joseph Wu]

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

Ship it!


Ship It!

- Joseph Wu


On Sept. 14, 2015, 5:14 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38386/
> ---
> 
> (Updated Sept. 14, 2015, 5:14 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Joseph Wu, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix 'operator=' spacing in the registry_client.hpp
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> b5e28587bc9adc2c02805b13fbc5c693612c99fb 
> 
> Diff: https://reviews.apache.org/r/38386/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38348: Correct typo in http.cpp

[Joseph Wu]

> On Sept. 14, 2015, 10:27 a.m., Joseph Wu wrote:
> > That's not a typo; "iff" is short for "if and only if".
> 
> Guangya Liu wrote:
> Thanks Joseph for pointing this oout. It is difficult for non native 
> english speakers understand this, what about updating "iff" to "if and only 
> if"?

I believe "iff" only appears in logic and math (and computer science by 
extension).  It's not something you would expect in an english dictionary.

I think no change is necessary in this case.


- Joseph


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


On Sept. 14, 2015, 1:48 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38348/
> ---
> 
> (Updated Sept. 14, 2015, 1:48 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Correct typo in http.cpp
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 73e8857833603cb0edb77c352f343070ee1b8c80 
> 
> Diff: https://reviews.apache.org/r/38348/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38172: Stout: Simplified hashset implementation.

[Ben Mahler]

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


Just a drive by on comment wrapping changes.


3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp (lines 40 - 41)


Let's avoid retroactively changing comment wrapping like this, since 80 
isn't a hard requirement. It would also be great if we could wrap to reduce 
jaggedness, I find the existing wrapping easier to read, you?


- Ben Mahler


On Sept. 8, 2015, 9:52 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38172/
> ---
> 
> (Updated Sept. 8, 2015, 9:52 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp 
> 1839d28638cd82dae10ba9b0f99c1a97cf34f9c9 
> 
> Diff: https://reviews.apache.org/r/38172/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

[Marco Massenzio]

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


LGTM
Can you please add a test with a few vars defined in Env and then in Cmd Line 
too, and verify that it "works as intended"?

Thanks for bearing with me and changing your code.
I think it looks clean and nice now :)

- Marco Massenzio


On Sept. 13, 2015, 5:30 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
> 
> (Updated Sept. 13, 2015, 5:30 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park.
> 
> 
> Bugs: MESOS-3340
> https://issues.apache.org/jira/browse/MESOS-3340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, it appears that re-defining a flag on the command-line that was 
> already defined via a OS Env var (MESOS_*) causes the Master to fail with a 
> not very helpful message.
> 
> For example, if one has MESOS_QUORUM defined, this happens:
> 
> $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 
> --hostname=192.168.1.4 --ip=192.168.1.4
> Duplicate flag 'quorum' on command line
> 
> which is not very helpful.
> 
> Current solution is to throw error if any duplication; over-write may make 
> user confused about the result.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6 
> 
> Diff: https://reviews.apache.org/r/38259/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

[Klaus Ma]

> On Sept. 15, 2015, 4:54 a.m., Marco Massenzio wrote:
> > LGTM
> > Can you please add a test with a few vars defined in Env and then in Cmd 
> > Line too, and verify that it "works as intended"?
> > 
> > Thanks for bearing with me and changing your code.
> > I think it looks clean and nice now :)

There used to be a test case on "DuplicatesFromEnvironment", I updated it 
according to our design behavior: overwrite env by cli.


- Klaus


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


On Sept. 13, 2015, 5:30 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38259/
> ---
> 
> (Updated Sept. 13, 2015, 5:30 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park.
> 
> 
> Bugs: MESOS-3340
> https://issues.apache.org/jira/browse/MESOS-3340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, it appears that re-defining a flag on the command-line that was 
> already defined via a OS Env var (MESOS_*) causes the Master to fail with a 
> not very helpful message.
> 
> For example, if one has MESOS_QUORUM defined, this happens:
> 
> $ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 
> --hostname=192.168.1.4 --ip=192.168.1.4
> Duplicate flag 'quorum' on command line
> 
> which is not very helpful.
> 
> Current solution is to throw error if any duplication; over-write may make 
> user confused about the result.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6 
> 
> Diff: https://reviews.apache.org/r/38259/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38279: Enabled resources/attributes discovery

[Niklas Nielsen]

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


Per Connors comment, we need a test to exercise the new code


include/mesos/hook.hpp (line 116)


Think we need to sharpen the naming of the hook; the prepended 'slave' just 
indicates in which component i.e. not in the master. So it reads 'Resources 
Decorator' seems a bit too broad to me.
How about 'InitialResourcesDecorator' or something that indicates that it's 
during initialization.



src/slave/slave.cpp (line 383)


newline


- Niklas Nielsen


On Sept. 14, 2015, 10:39 a.m., Felix Abecassis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> ---
> 
> (Updated Sept. 14, 2015, 10:39 a.m.)
> 
> 
> Review request for mesos, Connor Doyle and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> First API draft for MESOS-3366.
> 
> 1) Only supports resources for now, we can add another hook for attributes 
> with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct 
> a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by 
> previous callbacks and are free to override or merge the detected resources 
> as they see fit.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp d90bacc 
>   src/examples/test_hook_module.cpp bc13a8a 
>   src/hook/manager.hpp 30d8321 
>   src/hook/manager.cpp 754c238 
>   src/slave/slave.cpp 5e5522e 
> 
> Diff: https://reviews.apache.org/r/38279/diff/
> 
> 
> Testing
> ---
> 
> make clean && make && make check
> 
> 
> Thanks,
> 
> Felix Abecassis
> 
>

Re: Review Request 38031: [3/5] Integer Precision for JSON <-> Protobuf conversions.

[Joseph Wu]

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

(Updated Sept. 14, 2015, 1:37 p.m.)


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


Changes
---

Slight refactoring of switch blobs.  Removed default switch cases.  Added some 
notes/comments.


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


Repository: mesos


Description
---

* Changes JSON::Number to keep track of whether it is floating, signed 
integral, or unsigned integral.
* Changes how JSON::Number is stringified, to ensure that stringified doubles 
are parsed as JSON doubles.
* Added one test for integer precision between String <-> JSON <-> Protobuf 
conversions.
* Added one test for JSON::Number comparisons.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
f28138c4682c41e94ab6c7641a78d66b2f9daa5f 
  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
57d5fdf45273c620655b44b5f5572290cffa4bf6 
  3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
850650c269e9be24c0f1ae81b8aa8725f8a0c151 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
c56d6a3098293eb3659b3066f10e875927ec3ac3 

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


Testing
---

No testing done until the last patch in the chain.

However, this patch does breaks some libprocess and mesos tests (because 
JSON::Number is stored differently), which are fixed in the following two 
reviews.


Thanks,

Joseph Wu

Re: Review Request 38370: Updated docker executor to set container IP in TaskStatus::NetworkInfo.

[Niklas Nielsen]

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


+1 on Tim's comment. I think you can just generalize the text to '3rd party 
tools'

Is there a corresponding test we could change to verify that this works as 
expected?

- Niklas Nielsen


On Sept. 14, 2015, 1:55 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38370/
> ---
> 
> (Updated Sept. 14, 2015, 1:55 p.m.)
> 
> 
> Review request for mesos, Connor Doyle, Jie Yu, Niklas Nielsen, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-3013
> https://issues.apache.org/jira/browse/MESOS-3013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated docker executor to set container IP in TaskStatus::NetworkInfo.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 6647075b55e5a79264e3556bb906a1f26a2d673e 
> 
> Diff: https://reviews.apache.org/r/38370/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 38287: Check if the futrue is failed before dispatch in freeze()

[Jian Qiu]

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

(Updated 九月 14, 2015, 7:28 a.m.)


Review request for mesos, Ian Downes, Jie Yu, and Paul Brett.


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


Repository: mesos


Description
---

This check in cgroups::freezer::freeze() is necessary
since the freezer process can be terminated in its
initialize method


Diffs (updated)
-

  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 

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


Testing
---

./mesos-tests.sh 
--gtest_filter="CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_FreezeNonFreezer"
 --verbose


Thanks,

Jian Qiu

Re: Review Request 38343: mesos: Fixed punctuation in log message.

[Niklas Nielsen]

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

Ship it!


Ship It!

- Niklas Nielsen


On Sept. 13, 2015, 10:09 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38343/
> ---
> 
> (Updated Sept. 13, 2015, 10:09 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Niklas Nielsen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per style guide, log messages should not end in a period.
> 
> 
> Diffs
> -
> 
>   src/authentication/cram_md5/authenticator.cpp 
> f751ee15a8e8fa47b645d4add0ebe457fa5b49fb 
> 
> Diff: https://reviews.apache.org/r/38343/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37505]

All tests passed.

- Mesos ReviewBot


On Sept. 14, 2015, 12:59 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37505/
> ---
> 
> (Updated Sept. 14, 2015, 12:59 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-3136
> https://issues.apache.org/jira/browse/MESOS-3136
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix broken health check in docker executor.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp aaf8884a20901ad3a440d545d4b6316b87fc3403 
>   src/docker/docker.cpp 553e831029454d6d423842915b8bbfcaf19fa7f9 
>   src/docker/executor.cpp 6647075b55e5a79264e3556bb906a1f26a2d673e 
>   src/slave/containerizer/docker.cpp 289d4ec0fba9071dfe0cbf5391b5391d4566dd9c 
>   src/tests/health_check_tests.cpp 157a56aa06677d8b7a2cef53b29ed05cb4b5d8ea 
> 
> Diff: https://reviews.apache.org/r/37505/diff/
> 
> 
> Testing
> ---
> 
> # Add two new test cases, HealthCheckTest.ROOT_DOCKER_DockerHealthyTask and 
> HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
> # Docker health check command is run through "docker exec"
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthyTask" --verbose
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange" 
> --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 38306: Libprocess: Removed namespace pollution from ssl gtest.

[Michael Park]

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

Ship it!



3rdparty/libprocess/include/process/ssl/gtest.hpp (lines 267 - 269)


An alternative formatting here would be:

```cpp
foreachpair (
const std::string& name, const std::string& value, environment) {
  os::setenv(name, value);
}
```

Up to you which one you like better :)


- Michael Park


On Sept. 11, 2015, 5:09 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38306/
> ---
> 
> (Updated Sept. 11, 2015, 5:09 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joseph Wu, Michael Park, and 
> Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This got introduced accidentally while factoring out common test
> fixtures.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> a6051b75f0a359ef1bc04e48444f9fd6da9a12dd 
> 
> Diff: https://reviews.apache.org/r/38306/diff/
> 
> 
> Testing
> ---
> 
> make check with and without ssl enabled.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 38333: Made container sandbox a shared mount to address MESOS-3349.

[Jiang Yan Xu]

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

Ship it!



src/slave/containerizer/isolators/filesystem/linux.cpp (lines 721 - 723)


You are logging the sandbox as "working directory" and "sandbox" 
respectively for the two (w/ new rootfs and w/o) cases in `prepare()`, should 
we aim for symmetry here when in `cleanup()`?

Or we can say `<< "Unmounting sandbox/work directory '" <<`?



src/slave/containerizer/isolators/filesystem/linux.cpp (line 728)


Ditto about the name "sandbox" here.



src/slave/containerizer/provisioners/appc/provisioner.cpp (lines 368 - 369)


Oh...

I thought `root` was being copied but I guess the compilers says otherwise?



src/slave/containerizer/provisioners/appc/provisioner.cpp (line 379)


Adjust the expected likelyhood a bit by s/likely/possible/?



src/slave/containerizer/provisioners/appc/provisioner.cpp (line 380)


Expand to explain the reason for EBUSY?

```
EBUSY because of the race between cleaning up this container and new 
containers copying the host mount table
```

It's not immediately obvious without some comments.



src/slave/containerizer/provisioners/backends/bind.cpp (line 169)


s/returns/return/.


- Jiang Yan Xu


On Sept. 12, 2015, 11:33 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38333/
> ---
> 
> (Updated Sept. 12, 2015, 11:33 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made container sandbox a shared mount to address MESOS-3349.
> 
> See the discussion in https://reviews.apache.org/r/38329/ for more context.
> 
> The idea is to mark container sandbox a shared mount (do a self bind mount 
> first) so that persistent volume mounts can be propagated.
> 
> This is less invasive than marking '/' as a shared mount.
> 
> One followup for this patch is to set the default filesystem isolator to 
> posix as the linux isolator will manipulate host mount table.
> 
> We also need to address the TODO in `LinuxFilesystemIsolator::_recover` so 
> that tests do not leak mounts in the host mount table.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
>   src/slave/containerizer/provisioners/appc/provisioner.cpp 
> cd29a00fa0db8af294c10bb7a2e0cb4252bd2993 
>   src/slave/containerizer/provisioners/backends/bind.cpp 
> 1cdae61786790dc6a475ae5f73c8cc92d2bbf739 
> 
> Diff: https://reviews.apache.org/r/38333/diff/
> 
> 
> Testing
> ---
> 
> sudo make check on Centos5 and Centos6
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38279: Enabled resources/attributes discovery

[Connor Doyle]

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



src/examples/test_hook_module.cpp (line 208)


Please add a test to verify that the modified resources are reflected in 
the resource offers formed by the master.  There are some good examples to 
follow in `src/tests/hook_tests.cpp`.


- Connor Doyle


On Sept. 14, 2015, 5:39 p.m., Felix Abecassis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> ---
> 
> (Updated Sept. 14, 2015, 5:39 p.m.)
> 
> 
> Review request for mesos, Connor Doyle and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> First API draft for MESOS-3366.
> 
> 1) Only supports resources for now, we can add another hook for attributes 
> with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct 
> a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by 
> previous callbacks and are free to override or merge the detected resources 
> as they see fit.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp d90bacc 
>   src/examples/test_hook_module.cpp bc13a8a 
>   src/hook/manager.hpp 30d8321 
>   src/hook/manager.cpp 754c238 
>   src/slave/slave.cpp 5e5522e 
> 
> Diff: https://reviews.apache.org/r/38279/diff/
> 
> 
> Testing
> ---
> 
> make clean && make && make check
> 
> 
> Thanks,
> 
> Felix Abecassis
> 
>

Re: Review Request 38279: Enabled resources/attributes discovery

[Connor Doyle]

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



src/hook/manager.cpp (line 261)


Please also add a test that verifies that the hooks are executed in the 
expected order.  For example, have two hooks that both modify the `cpus` 
resource and verify that the second value is indeed retained.


- Connor Doyle


On Sept. 14, 2015, 5:39 p.m., Felix Abecassis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38279/
> ---
> 
> (Updated Sept. 14, 2015, 5:39 p.m.)
> 
> 
> Review request for mesos, Connor Doyle and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3366
> https://issues.apache.org/jira/browse/MESOS-3366
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> First API draft for MESOS-3366.
> 
> 1) Only supports resources for now, we can add another hook for attributes 
> with a very similar code.
> 2) The callback currently receives the full SlaveInfo structure and construct 
> a new Resources object.
> 3) If there is multiple callbacks, each callback will see the changes made by 
> previous callbacks and are free to override or merge the detected resources 
> as they see fit.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp d90bacc 
>   src/examples/test_hook_module.cpp bc13a8a 
>   src/hook/manager.hpp 30d8321 
>   src/hook/manager.cpp 754c238 
>   src/slave/slave.cpp 5e5522e 
> 
> Diff: https://reviews.apache.org/r/38279/diff/
> 
> 
> Testing
> ---
> 
> make clean && make && make check
> 
> 
> Thanks,
> 
> Felix Abecassis
> 
>

Re: Review Request 38289: Handle bad request in Docker registry client.

[Anand Mazumdar]

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


LGTM. Just a minor query around why do we want to parse the error response JSON 
and not directly return the entire response JSON ?


src/Makefile.am (line 1693)


Can we do this renaming/moving test files in a separate patch ? This looks 
un-related to handling BadRequest in the Registry Client. What do you think ?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 266)


Not yours , but can we just do OK().status ?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 268)


Can we just do BadRequest().status here to eliminate the hard-coded string 
constant  ?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 277)


Why are we parsing the error JSON to extract the error string from JSON 
here and not dump the entire error string to the end-user. Is the rationale 
that this makes the error messages readable and more consistent ?

However, the cons to this are:
1. We don't seem to be following this design any-where else in our 
code-base ?
2. We have already omitted fields `code` and `detail` from the error 
response ( http://docs.docker.com/registry/spec/api/ ). If Docker adds more 
fields in the future or deletes/renames some of them, we would need to revisit 
them again?

Won't providing the entire JSON to the end-user be more helpful in 
identifying the root-case and helping him/her resolve the issue faster ?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 454)


Nit: 'Docker-Content-Digest' in quotes.


- Anand Mazumdar


On Sept. 11, 2015, 7:34 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38289/
> ---
> 
> (Updated Sept. 11, 2015, 7:34 p.m.)
> 
> 
> Review request for mesos and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Handle bad request in Docker registry client.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8963cea9fd7e3bee450a1059e6383e5ab868a17b 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp 
> ff29d562c7f1bd5f0579e97cdbd60c2b2f36329e 
> 
> Diff: https://reviews.apache.org/r/38289/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38253: Add containerId to ResourceUsage to enable QoS controller to target a container

[Niklas Nielsen]

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



include/mesos/mesos.proto (line 820)


Let's add a comment about this field



include/mesos/slave/oversubscription.proto (lines 46 - 47)


We need to add a comment about the semantics of executor_id vs 
container_id. For example, what happens if both is set? Or do they need to be 
set together? (Loooks like it from the code below)



src/slave/slave.cpp (lines 4367 - 4368)


Style nit:

```
  const ContainerID& containerId =
  kill.has_container_id() ? kill.container_id() : 
executor->containerId;
```

Also, what happens if the container id is wrong? Think we need to do some 
validation here.



src/tests/oversubscription_tests.cpp (line 807)


s/to QoS Controller/to the QoS Controller/



src/tests/oversubscription_tests.cpp (line 859)


Mind adding a TODO about doing a full blown object comparison?


- Niklas Nielsen


On Sept. 14, 2015, 10:41 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38253/
> ---
> 
> (Updated Sept. 14, 2015, 10:41 a.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2875
> https://issues.apache.org/jira/browse/MESOS-2875
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We should ensure that we are addressing the container which the QoS 
> controller intended to kill. Without this check, we may run into a scenario 
> where the executor has terminated and one with the same id has started in the 
> interim i.e. running in a different container than the one the QoS controller 
> targeted.
> 
> This most likely requires us to add containerId to the ResourceUsage message 
> and encode the containerID in the QoS Correction message.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
>   include/mesos/slave/oversubscription.proto 
> fa69a95689c8c75765f0800692655e8dde7dde33 
>   src/slave/slave.cpp 5e5522e1254a5ed6084de36782753f9aad2894c4 
>   src/tests/oversubscription_tests.cpp 
> 0c5edafc139d9bfb6806d007a0af85e80893bb1a 
> 
> Diff: https://reviews.apache.org/r/38253/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>