Re: Review Request 55910: Prevent unintended mutation in the allocator.

[Guangya Liu]

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


Fix it, then Ship it!




Ship It!


src/master/allocator/mesos/hierarchical.cpp (lines 237 - 241)


Just a nit here: It seems a bit strange to me here of getting roles as 
following:
1) Construct `frameworks`
2) Get related framework info
3) Get related framework role

How about keep the logic as before but put the logic of construct of 
`frameworks` to #275 here?


- Guangya Liu


On 一月 25, 2017, 2:40 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> ---
> 
> (Updated 一月 25, 2017, 2:40 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
> 
> With this change, a few const functions can be marked as such.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
> 
> Diff: https://reviews.apache.org/r/55910/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52534: Dispatch filter expiration twice.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [52534, 51027]

Failed command: python support/apply-reviews.py -n -r 52534

Error:
2017-01-27 06:46:11 URL:https://reviews.apache.org/r/52534/diff/raw/ 
[4204/4204] -> "52534.patch" [1]
error: patch failed: src/master/allocator/mesos/hierarchical.hpp:226
error: src/master/allocator/mesos/hierarchical.hpp: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/16871/console

- Mesos Reviewbot


On Jan. 27, 2017, 2:34 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52534/
> ---
> 
> (Updated Jan. 27, 2017, 2:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan 
> Xu.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - With an asynchronous `batch()` allocation,
>   this ensures that filters do not expire
>   before the next allocation.
> - This patch should be reverted when allocation
>   occurs on resource recovery.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
> 
> Diff: https://reviews.apache.org/r/52534/diff/
> 
> 
> Testing
> ---
> 
> With https://reviews.apache.org/r/51027/: 
> 
> GTEST_FILTER="-*SmallOfferFilter*" make check -j8
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55790]

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

- Mesos Reviewbot


On Jan. 27, 2017, 1:27 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55790/
> ---
> 
> (Updated Jan. 27, 2017, 1:27 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6858
> https://issues.apache.org/jira/browse/MESOS-6858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for the full set of DNS resolver configuration items that
> a CNI IPAM plugin can specify. This implements updating the container's
> resolv.conf with the 'domain', 'search', and 'options' keywords.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 84dc157e7d9e332a6da0f1fc33303e9ef9bdc147 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> ccd511ec14810dcc1020dec5e1641141f3a319b4 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 
> ac48159dadcea422f605e723db94a7f3bb573fa2 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> cb893d3ef005a9cc60c40768fa669b27c4863020 
> 
> Diff: https://reviews.apache.org/r/55790/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55973: Update the tests to handle MULTI_ROLE support.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [55973, 56006, 56005, 56004, 55972, 55971, 55970, 55969, 
55968, 55967, 55870, 55910, 55909, 55908, 55868, 55824, 55866, 55863, 55828, 
55829, 55827, 55826, 55825, 54836, 54842]

Failed command: python support/apply-reviews.py -n -r 55870

Error:
2017-01-27 04:04:34 URL:https://reviews.apache.org/r/55870/diff/raw/ 
[47956/47956] -> "55870.patch" [1]
error: patch failed: src/master/master.cpp:6528
error: src/master/master.cpp: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/16869/console

- Mesos Reviewbot


On Jan. 27, 2017, 12:34 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55973/
> ---
> 
> (Updated Jan. 27, 2017, 12:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A number of tests and example frameworks assume that allocated
> resources did not look different from unallocated resources.
> This updates the tests to reflect the presence of
> `Resource.AllocationInfo`.
> 
> 
> Diffs
> -
> 
>   src/examples/disk_full_framework.cpp 
> e13d4c8a427905793dda9bb01c52b6d372c19150 
>   src/examples/dynamic_reservation_framework.cpp 
> 4d3db965e18d79ed3e1759bb5f6cb41104562f47 
>   src/examples/no_executor_framework.cpp 
> e82ae9a9ea23c7d181b071f0e3f5071b3856d8a4 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
>   src/examples/test_framework.cpp 068b85d551012a390ba2a68bc88807103d3c31f3 
>   src/examples/test_http_framework.cpp 
> 258cb512803d1ed07c7a5ce1465e5663e77b0977 
>   src/tests/api_tests.cpp 400ac6fcd420508509291249cad50679fbe9807d 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> ba268b6918be0e7226a975c38eff5620b076083f 
>   src/tests/hook_tests.cpp f4ef629768beabc014e14472c5818d117d51abe7 
>   src/tests/master_allocator_tests.cpp 
> 996762f25453f7a8a5e0b7b97006ee2a603cf8c4 
>   src/tests/master_tests.cpp da7094dbbafbb0ab1153a0a4a6fcabd63888d67a 
>   src/tests/mesos.hpp 71e919702bccd31bf1e14205cd1e30d473883d48 
>   src/tests/oversubscription_tests.cpp 
> 22ae069ab71c82c6a6e2f5783b13ebd74a9ccf25 
>   src/tests/partition_tests.cpp f03c5bd96861b6643a4ecd9e37775447f86c3710 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 695029fe0cb91d668d6a8495f3ccd0c69c883cd2 
>   src/tests/persistent_volume_tests.cpp 
> 468a85b4a6ce09592341afd07ce12a03f5fc4f73 
>   src/tests/reservation_endpoints_tests.cpp 
> 7bc59c2ded9f7e6f1ccaa37456f654d18efcb2e2 
>   src/tests/reservation_tests.cpp b7061de0dfcc79548d507c7d70376c82d5d647ec 
>   src/tests/resource_offers_tests.cpp 
> 72ceb8696a7277f9add6d89adb55aa08665bdeb5 
>   src/tests/role_tests.cpp 0d0f1a94ff9aaec6ea04280282a52ac88dc6b273 
>   src/tests/slave_recovery_tests.cpp 9cf1d461cf5c6ac1f51ac963360a17e37f558505 
> 
> Diff: https://reviews.apache.org/r/55973/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 56005: Added a safety CHECK when accessing activeRoles in the master.

[Guangya Liu]

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


Ship it!




Ship It!

- Guangya Liu


On 一月 27, 2017, 12:31 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56005/
> ---
> 
> (Updated 一月 27, 2017, 12:31 a.m.)
> 
> 
> Review request for mesos, Michael Park and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently there is a use of the [] operator that is not preceded
> by a CHECK that the key is contained in the map. This leads to a
> hard to diagnose issue if there are bugs that violate this
> assumption.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 0f2c7cd32161576fad742ef350dff64874b80854 
> 
> Diff: https://reviews.apache.org/r/56005/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 56006: Added CHECK logging to the allocator.

[Guangya Liu]

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




src/master/allocator/mesos/hierarchical.cpp (line 432)


How about make the log messsage more clear such as 

```
CHECK(framework.roles == newRoles)
  << stringify(framework.roles) << " does not match " << 
stringify(newRoles);

```


- Guangya Liu


On 一月 27, 2017, 12:32 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56006/
> ---
> 
> (Updated 一月 27, 2017, 12:32 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added CHECK logging to the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
> 
> Diff: https://reviews.apache.org/r/56006/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 49571: Added a benchmark test for allocations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [53096, 45962, 55359, 49571]

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

- Mesos Reviewbot


On Jan. 26, 2017, 10:06 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49571/
> ---
> 
> (Updated Jan. 26, 2017, 10:06 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5771
> https://issues.apache.org/jira/browse/MESOS-5771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allocations test has the following resource configurations:
> (1) REGULAR: Offers from every slave have regular resources.
> (2) SHARED: Offers from every slave include a shared resource.
> (3) REGULAR: Offers from every alternate slave contain only regular
> resources; and offers from every other alternate slave contains
> a shared resource.
> 
> This test is parameterized based on number of agents, number of
> frameworks and resource configuration.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/tests/resources_utils.cpp be1feb97be552af582dbba3a54fa5fa0714b3eb2 
> 
> Diff: https://reviews.apache.org/r/49571/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> Allocations benchmark test results
> ==
> Support of shared resources has a small impact (roughly 10%) on runtime 
> performance in allocations as compared to HEAD (without shared resources). 
> Also, there is no visible impact in performance when shared resources are 
> added in the tests.
> 
> Following is a snapshot with 1000 agents and 200 frameworks.
> 
> With the patch (and no shared resources)
> 
> [ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
> Using 1000 agents and 200 frameworks with resource type 0
> Added 200 frameworks in 6907us
> Added 1000 agents in 2.057098secs
> round 0 allocate() took 1.689164secs to make 1000 offers
> round 50 allocate() took 1.672373secs to make 1000 offers
> round 100 allocate() took 1.680571secs to make 1000 offers
> round 150 allocate() took 1.674683secs to make 1000 offers
> round 199 allocate() took 1.671525secs to make 1000 offers
> 
> With the patch (and shared resources on all agents)
> ---
> [ RUN  ] 
> AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
> Using 1000 agents and 200 frameworks with resource type 1
> Added 200 frameworks in 6888us
> Added 1000 agents in 2.096218secs
> round 0 allocate() took 1.704491secs to make 1000 offers
> round 50 allocate() took 1.718623secs to make 1000 offers
> round 100 allocate() took 1.716224secs to make 1000 offers
> round 150 allocate() took 1.707343secs to make 1000 offers
> round 199 allocate() took 1.727467secs to make 1000 offers
> 
> With the patch (and shared resources on alternate agents)
> -
> [ RUN  ] 
> AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
> Using 1000 agents and 200 frameworks with resource type 2
> Added 200 frameworks in 7304us
> Added 1000 agents in 2.071009secs
> round 0 allocate() took 1.689045secs to make 1000 offers
> round 50 allocate() took 1.691524secs to make 1000 offers
> round 100 allocate() took 1.688873secs to make 1000 offers
> round 150 allocate() took 1.688713secs to make 1000 offers
> round 199 allocate() took 1.691223secs to make 1000 offers
> 
> Based on HEAD, with all regular resources (no shared resources in HEAD 
> supported)
> -
> [ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
> Using 1000 agents and 200 frameworks with resource type 0
> Added 200 frameworks in 6801us
> Added 1000 agents in 1.721447secs
> round 0 allocate() took 1.502953secs to make 1000 offers
> round 50 allocate() took 1.520157secs to make 1000 offers
> round 100 allocate() took 1.517221secs to make 1000 offers
> round 150 allocate() took 1.526446secs to make 1000 offers
> round 199 allocate() took 1.538005secs to make 1000 offers
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51028: Fix tests with rapidly triggered allocations.

[Jacob Janco]

> On Jan. 19, 2017, 3:15 a.m., Benjamin Mahler wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2994-2998
> > 
> >
> > This test is already paused, all of these should be run with a paused 
> > clock, are any of them not?

Earlier in the test: 
// Allow the allocation timer to measure time.
Clock::resume();


- Jacob


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


On Jan. 27, 2017, 2:33 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51028/
> ---
> 
> (Updated Jan. 27, 2017, 2:33 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Per MESOS-3157, if cluster events with the possibility
>   of triggering an allocation occur rapidly and test
>   assertions depend on gleaning information from assumed
>   order, it is likely they will fail due to lack of parity
>   between event and actual allocation. Ensure that expected
>   allocations occur.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 
> 
> Diff: https://reviews.apache.org/r/51028/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Jan. 21, 2017, 1:24 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1314
> > 
> >
> > We can use the `|=` operator now that it's added.

Added, was waiting for that commit =D


- Jacob


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


On Jan. 27, 2017, 2:33 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Jan. 27, 2017, 2:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check with the filters below
> 
> Broken tests: 
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 
> 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), 
> fix in 51621
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51028: Fix tests with rapidly triggered allocations.

[Jacob Janco]

> On Jan. 23, 2017, 11:08 p.m., Jiang Yan Xu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3765
> > 
> >
> > Is `s/performing/; performed/?` better?

yea I like that better


- Jacob


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


On Jan. 27, 2017, 2:33 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51028/
> ---
> 
> (Updated Jan. 27, 2017, 2:33 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Per MESOS-3157, if cluster events with the possibility
>   of triggering an allocation occur rapidly and test
>   assertions depend on gleaning information from assumed
>   order, it is likely they will fail due to lack of parity
>   between event and actual allocation. Ensure that expected
>   allocations occur.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 
> 
> Diff: https://reviews.apache.org/r/51028/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 51028: Fix tests with rapidly triggered allocations.

[Jacob Janco]

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

(Updated Jan. 27, 2017, 2:33 a.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

- Per MESOS-3157, if cluster events with the possibility
  of triggering an allocation occur rapidly and test
  assertions depend on gleaning information from assumed
  order, it is likely they will fail due to lack of parity
  between event and actual allocation. Ensure that expected
  allocations occur.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 

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


Testing
---

make check


Thanks,

Jacob Janco

Re: Review Request 52534: Dispatch filter expiration twice.

[Jacob Janco]

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

(Updated Jan. 27, 2017, 2:34 a.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jiang Yan 
Xu.


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


Repository: mesos


Description
---

- With an asynchronous `batch()` allocation,
  this ensures that filters do not expire
  before the next allocation.
- This patch should be reverted when allocation
  occurs on resource recovery.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
9b66c23f26b37c02ed850c06c4518ea99078b02d 
  src/master/allocator/mesos/hierarchical.cpp 
c2211be7458755aeb91ef078e4bfe92ac474044a 

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


Testing
---

With https://reviews.apache.org/r/51027/: 

GTEST_FILTER="-*SmallOfferFilter*" make check -j8


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

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

(Updated Jan. 27, 2017, 2:33 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, 
and Jiang Yan Xu.


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


Repository: mesos


Description
---

- Triggered allocations dispatch allocate() only
  if there is no pending allocation in the queue.
- Allocation candidates are accumulated and only
  cleared when enqueued allocations are processed.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
9b66c23f26b37c02ed850c06c4518ea99078b02d 
  src/master/allocator/mesos/hierarchical.cpp 
c2211be7458755aeb91ef078e4bfe92ac474044a 

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


Testing
---

make check with the filters below

Broken tests: 
- TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
- TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
- TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
- TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
- TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
- TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), 
fix in 51621


Thanks,

Jacob Janco

Re: Review Request 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Jan. 19, 2017, 2:45 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1273-1280
> > 
> >
> > This change introduces an additional trip through the allocator's queue 
> > after the allocation completes and before the delay is called.
> > 
> > This would avoid it:
> > 
> > ```
> > auto pid = self();
> > 
> > // TODO: Use process::loop for the allocation loop.
> > allocate()
> >   .onAny([pid]() {
> > delay(allocationInterval, self(), ::batch);
> >   }
> > ```
> > 
> > Not sure if this was intentional or not.

This was unintentional, defer does not need to be called here.


- Jacob


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


On Jan. 12, 2017, 6:55 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Jan. 12, 2017, 6:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> a6424d624864155e1c87a28a63b784512c5c8722 
>   src/master/allocator/mesos/hierarchical.cpp 
> 91b1ec43940a788459f045ca4a4b82d4e8373bca 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check with the filters below
> 
> Broken tests: 
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 
> 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), 
> fix in 51621
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[James Peach]

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

(Updated Jan. 27, 2017, 1:27 a.m.)


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


Changes
---

Address review feedback.


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


Repository: mesos


Description
---

Add support for the full set of DNS resolver configuration items that
a CNI IPAM plugin can specify. This implements updating the container's
resolv.conf with the 'domain', 'search', and 'options' keywords.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
84dc157e7d9e332a6da0f1fc33303e9ef9bdc147 
  src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
ccd511ec14810dcc1020dec5e1641141f3a319b4 
  src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 
ac48159dadcea422f605e723db94a7f3bb573fa2 
  src/tests/containerizer/cni_isolator_tests.cpp 
cb893d3ef005a9cc60c40768fa669b27c4863020 

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


Testing
---

sudo make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 55600: CMake: Transitioned Stout to automatic source grouping.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Jan. 16, 2017, 10:41 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55600/
> ---
> 
> (Updated Jan. 16, 2017, 10:41 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout currently manually groups its source code by folder in order to
> present the source nicely in IDEs like XCode and Visual Studio.
> 
> With the recently-introduced CMake function, `GROUP_SOURCE`, this
> process is automated away. This commit will remove these manual groups
> and replace it with a call to this function.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/cmake/StoutConfigure.cmake 
> bc27ac687bae4e1798eece562027ba33c6b32348 
>   3rdparty/stout/tests/CMakeLists.txt 
> a8a3ac772aa243c848a6fd8d7a0d45acfe1b98ae 
> 
> Diff: https://reviews.apache.org/r/55600/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55600: CMake: Transitioned Stout to automatic source grouping.

[Joseph Wu]

> On Jan. 24, 2017, 4:27 p.m., Joseph Wu wrote:
> > 3rdparty/stout/cmake/StoutConfigure.cmake, line 47
> > 
> >
> > Why not use `*.hpp` instead?
> 
> Alex Clemmer wrote:
> Generally, we also would like to include .h files generated by protobufs, 
> either those that exist, or those that might exist in the future. In Stout, 
> for example, there is `protobuf_tests.proto` in the `tests/` folder. The 
> Stout headers don't have any .proto files that I know of right now, but I 
> think it's better just to use this one pattern everywhere.
> 
> Joseph Wu wrote:
> Ok, but let's be explicit, say `*.h(pp)?`.  Otherwise, we'll be matching 
> against `.hooligans` and `.hoopla` :)
> 
> Similar below.

Nevermind.  I didn't realize this was a glob expression, rather than a regex.


- Joseph


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


On Jan. 16, 2017, 10:41 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55600/
> ---
> 
> (Updated Jan. 16, 2017, 10:41 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout currently manually groups its source code by folder in order to
> present the source nicely in IDEs like XCode and Visual Studio.
> 
> With the recently-introduced CMake function, `GROUP_SOURCE`, this
> process is automated away. This commit will remove these manual groups
> and replace it with a call to this function.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/cmake/StoutConfigure.cmake 
> bc27ac687bae4e1798eece562027ba33c6b32348 
>   3rdparty/stout/tests/CMakeLists.txt 
> a8a3ac772aa243c848a6fd8d7a0d45acfe1b98ae 
> 
> Diff: https://reviews.apache.org/r/55600/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 55973: Update the tests to handle MULTI_ROLE support.

[Benjamin Mahler]

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

Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
Park.


Repository: mesos


Description
---

A number of tests and example frameworks assume that allocated
resources did not look different from unallocated resources.
This updates the tests to reflect the presence of
`Resource.AllocationInfo`.


Diffs
-

  src/examples/disk_full_framework.cpp e13d4c8a427905793dda9bb01c52b6d372c19150 
  src/examples/dynamic_reservation_framework.cpp 
4d3db965e18d79ed3e1759bb5f6cb41104562f47 
  src/examples/no_executor_framework.cpp 
e82ae9a9ea23c7d181b071f0e3f5071b3856d8a4 
  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 
  src/examples/test_framework.cpp 068b85d551012a390ba2a68bc88807103d3c31f3 
  src/examples/test_http_framework.cpp 258cb512803d1ed07c7a5ce1465e5663e77b0977 
  src/tests/api_tests.cpp 400ac6fcd420508509291249cad50679fbe9807d 
  src/tests/containerizer/cgroups_isolator_tests.cpp 
ba268b6918be0e7226a975c38eff5620b076083f 
  src/tests/hook_tests.cpp f4ef629768beabc014e14472c5818d117d51abe7 
  src/tests/master_allocator_tests.cpp 996762f25453f7a8a5e0b7b97006ee2a603cf8c4 
  src/tests/master_tests.cpp da7094dbbafbb0ab1153a0a4a6fcabd63888d67a 
  src/tests/mesos.hpp 71e919702bccd31bf1e14205cd1e30d473883d48 
  src/tests/oversubscription_tests.cpp 22ae069ab71c82c6a6e2f5783b13ebd74a9ccf25 
  src/tests/partition_tests.cpp f03c5bd96861b6643a4ecd9e37775447f86c3710 
  src/tests/persistent_volume_endpoints_tests.cpp 
695029fe0cb91d668d6a8495f3ccd0c69c883cd2 
  src/tests/persistent_volume_tests.cpp 
468a85b4a6ce09592341afd07ce12a03f5fc4f73 
  src/tests/reservation_endpoints_tests.cpp 
7bc59c2ded9f7e6f1ccaa37456f654d18efcb2e2 
  src/tests/reservation_tests.cpp b7061de0dfcc79548d507c7d70376c82d5d647ec 
  src/tests/resource_offers_tests.cpp 72ceb8696a7277f9add6d89adb55aa08665bdeb5 
  src/tests/role_tests.cpp 0d0f1a94ff9aaec6ea04280282a52ac88dc6b273 
  src/tests/slave_recovery_tests.cpp 9cf1d461cf5c6ac1f51ac963360a17e37f558505 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 56006: Added CHECK logging to the allocator.

[Benjamin Mahler]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Added CHECK logging to the allocator.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
c2211be7458755aeb91ef078e4bfe92ac474044a 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 56005: Added a safety CHECK when accessing activeRoles in the master.

[Benjamin Mahler]

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

Review request for mesos, Michael Park and Neil Conway.


Repository: mesos


Description
---

Currently there is a use of the [] operator that is not preceded
by a CHECK that the key is contained in the map. This leads to a
hard to diagnose issue if there are bugs that violate this
assumption.


Diffs
-

  src/master/master.cpp 0f2c7cd32161576fad742ef350dff64874b80854 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 56004: Fixed MULTI_ROLE related bugs when updating framework info.

[Benjamin Mahler]

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

Review request for mesos, Benjamin Bannier and Michael Park.


Repository: mesos


Description
---

The first issue is that we need to update the capabilities member
to reflect the new capabilities.

The second issue is that when we allow an upgrade or downgrade
to or from MULTI_ROLE, we need to update the `role` and `roles`
fields of `FrameworkInfo`.


Diffs
-

  src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 55972: Updated master to handle non-MULTI_ROLE agents.

[Benjamin Mahler]

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

Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
Park.


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


Repository: mesos


Description
---

With the addition of MULTI_ROLE framweork support, allocated
resources have a `Resource.AllocationInfo` set. While the new
MULTI_ROLE capable agents will be sending allocated resources,
the old agents may not be since they may not preserve the
unknown fields.

To cope with this, the master ensures the `Resource.AllocationInfo`
is set for non-MULTI_ROLE capable agents, by injecting it. Note
that we can only do this so long as it is unambiguous! This will
be prevented by not allowing frameworks with multiple to use
agents without the MULTI_ROLE support, see: MESOS-6940.


Diffs
-

  src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45 
  src/master/master.cpp 0f2c7cd32161576fad742ef350dff64874b80854 

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


Testing
---

The tests pass at the end of this review chain.


Thanks,

Benjamin Mahler

Re: Review Request 55971: Updated the agent to be MULTI_ROLE capable.

[Benjamin Mahler]

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

(Updated Jan. 27, 2017, 12:27 a.m.)


Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
Park.


Changes
---

Fixed an incorrect CHECK assumption.


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


Repository: mesos


Description
---

With the addition of MULTI_ROLE support, the agent needs to ensure
that allocated resources reported to the master have the
`Resource.AllocationInfo` set. The approach taken here is to set
it only in memory upon recovering tasks and executors. Note that
once we allow a framework to modify its roles, we need to update
the agent to re-persist the resources with injected allocation
info (rather than just setting it in memory).


Diffs (updated)
-

  src/slave/resource_estimators/fixed.cpp 
2c1268c3423091c6a419020a3af97255de55db0a 
  src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891 

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


Testing
---

The tests pass at the end of this review chain.


Thanks,

Benjamin Mahler

Re: Review Request 56001: Fixed a few executor segfaults during cleanup.

[Anand Mazumdar]

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


Fix it, then Ship it!




LGTM!


src/tests/default_executor_tests.cpp (line 996)


s/dies/failed


- Anand Mazumdar


On Jan. 27, 2017, 12:01 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56001/
> ---
> 
> (Updated Jan. 27, 2017, 12:01 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6989
> https://issues.apache.org/jira/browse/MESOS-6989
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The addition of `process::finalize` at the end of some binaries
> led to some segfaulting at the end of the binaries' lifetimes.
> This is mostly due calling destructors of libprocess actor
> wrappers, which require an initialized libprocess to function.
> 
> This commit explicitly calls the destructors on the default
> and docker executor actors prior to calling `process::finalize`.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp f353341c9509532653e2ef8802f20259ce67930c 
>   src/launcher/default_executor.cpp a03794934adb93868734f8cf00b337a1bff9b5ab 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> fa734e6b4b5d61e944cd70f1378f2d2eb534fe22 
>   src/tests/default_executor_tests.cpp 
> 8fc7fbeedfab8786c672af1b21cb7fa9a3347266 
> 
> Diff: https://reviews.apache.org/r/56001/diff/
> 
> 
> Testing
> ---
> 
> sudo src/mesos-tests --gtest_filter="*ROOT*DOCKER*" --verbose
> 
> Checked that all executors exited with status code 0, rather than 11.
> 
> ---
> 
> Modified two tests to check on exit status code explicitly.
> 
> sudo src/mesos-tests 
> --gtest_filter="*CommitSuicideOnTaskFailure*:*ROOT_DOCKER_Kill" --verbose
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[Avinash sridharan]

> On Jan. 25, 2017, 6:15 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 1006
> > 
> >
> > Can we keep the `LOG(INFO)` dumping the contents of `resolv.conf`?
> 
> James Peach wrote:
> This seems more usefult for debugging that for operations. Maybe a VLOG? 
> I'm also not sure that nameservers are the what you want to see in this.

Generally nameserver are pretty critical for containers to work (service 
discovery). In practice I have seen containers fail due to bad nameservers 
being setup and its pretty hard to debug those issues, since DNS configuration 
doesn't show up in container logs. Given that there is an explicit nameserver 
being thrown here thought LOG(INFO) would be helpful to the operators to 
understand how container connectivity would work.


> On Jan. 25, 2017, 6:15 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 950
> > 
> >
> > nameservers should be IP addresses? Otherwise it becomes a chicken and 
> > egg problem.
> 
> James Peach wrote:
> Sure, but there's no enforcement of this in any of the API layers. From 
> the perspective of the tests this is an arbitrary string.

I agree. Was just pointing this out that having a random IP address is more 
conformant, and would be more readable.


- Avinash


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


On Jan. 26, 2017, 11:19 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55790/
> ---
> 
> (Updated Jan. 26, 2017, 11:19 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6858
> https://issues.apache.org/jira/browse/MESOS-6858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for the full set of DNS resolver configuration items that
> a CNI IPAM plugin can specify. This implements updating the container's
> resolv.conf with the 'domain', 'search', and 'options' keywords.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 84dc157e7d9e332a6da0f1fc33303e9ef9bdc147 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> ccd511ec14810dcc1020dec5e1641141f3a319b4 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 
> ac48159dadcea422f605e723db94a7f3bb573fa2 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> cb893d3ef005a9cc60c40768fa669b27c4863020 
> 
> Diff: https://reviews.apache.org/r/55790/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 56001: Fixed a few executor segfaults during cleanup.

[Joseph Wu]

> On Jan. 26, 2017, 2:47 p.m., Anand Mazumdar wrote:
> > src/docker/executor.cpp, line 817
> > 
> >
> > Not yours, but we should have a comment here too as to why we need an 
> > explicit `finalize()` here in a separate patch.

This is mine :)  I added the `finalize`s in the first place.  I'll add the 
comments (in 4 places, including 2 of the files in this review) in a separate 
commit.


- Joseph


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


On Jan. 26, 2017, 4:01 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56001/
> ---
> 
> (Updated Jan. 26, 2017, 4:01 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6989
> https://issues.apache.org/jira/browse/MESOS-6989
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The addition of `process::finalize` at the end of some binaries
> led to some segfaulting at the end of the binaries' lifetimes.
> This is mostly due calling destructors of libprocess actor
> wrappers, which require an initialized libprocess to function.
> 
> This commit explicitly calls the destructors on the default
> and docker executor actors prior to calling `process::finalize`.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp f353341c9509532653e2ef8802f20259ce67930c 
>   src/launcher/default_executor.cpp a03794934adb93868734f8cf00b337a1bff9b5ab 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> fa734e6b4b5d61e944cd70f1378f2d2eb534fe22 
>   src/tests/default_executor_tests.cpp 
> 8fc7fbeedfab8786c672af1b21cb7fa9a3347266 
> 
> Diff: https://reviews.apache.org/r/56001/diff/
> 
> 
> Testing
> ---
> 
> sudo src/mesos-tests --gtest_filter="*ROOT*DOCKER*" --verbose
> 
> Checked that all executors exited with status code 0, rather than 11.
> 
> ---
> 
> Modified two tests to check on exit status code explicitly.
> 
> sudo src/mesos-tests 
> --gtest_filter="*CommitSuicideOnTaskFailure*:*ROOT_DOCKER_Kill" --verbose
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 56001: Fixed a few executor segfaults during cleanup.

[Joseph Wu]

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

(Updated Jan. 26, 2017, 4:01 p.m.)


Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.


Changes
---

Tweaked two tests to check for exit codes.  Tweaked/added comments.


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


Repository: mesos


Description
---

The addition of `process::finalize` at the end of some binaries
led to some segfaulting at the end of the binaries' lifetimes.
This is mostly due calling destructors of libprocess actor
wrappers, which require an initialized libprocess to function.

This commit explicitly calls the destructors on the default
and docker executor actors prior to calling `process::finalize`.


Diffs (updated)
-

  src/docker/executor.cpp f353341c9509532653e2ef8802f20259ce67930c 
  src/launcher/default_executor.cpp a03794934adb93868734f8cf00b337a1bff9b5ab 
  src/tests/containerizer/docker_containerizer_tests.cpp 
fa734e6b4b5d61e944cd70f1378f2d2eb534fe22 
  src/tests/default_executor_tests.cpp 8fc7fbeedfab8786c672af1b21cb7fa9a3347266 

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


Testing (updated)
---

sudo src/mesos-tests --gtest_filter="*ROOT*DOCKER*" --verbose

Checked that all executors exited with status code 0, rather than 11.

---

Modified two tests to check on exit status code explicitly.

sudo src/mesos-tests 
--gtest_filter="*CommitSuicideOnTaskFailure*:*ROOT_DOCKER_Kill" --verbose


Thanks,

Joseph Wu

Re: Review Request 55995: Restored quota correctly during allocator recovery.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55995]

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

- Mesos Reviewbot


On Jan. 26, 2017, 8:29 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55995/
> ---
> 
> (Updated Jan. 26, 2017, 8:29 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-7008
> https://issues.apache.org/jira/browse/MESOS-7008
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding neglected to restore the quota during allocator
> recovery if the expected agent count is zero.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
>   src/tests/master_quota_tests.cpp 54ae66f09a0b9d1e3be0c44bd4a8bbf5d27ba688 
> 
> Diff: https://reviews.apache.org/r/55995/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[James Peach]

> On Jan. 25, 2017, 6:15 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 945
> > 
> >
> > why do a `dns.Clear()`?

Consistency. Each block is the same and it remove the risk of breakage from 
reordering or adding new checks.


> On Jan. 25, 2017, 6:15 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 950
> > 
> >
> > nameservers should be IP addresses? Otherwise it becomes a chicken and 
> > egg problem.

Sure, but there's no enforcement of this in any of the API layers. From the 
perspective of the tests this is an arbitrary string.


- James


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


On Jan. 26, 2017, 11:19 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55790/
> ---
> 
> (Updated Jan. 26, 2017, 11:19 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6858
> https://issues.apache.org/jira/browse/MESOS-6858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for the full set of DNS resolver configuration items that
> a CNI IPAM plugin can specify. This implements updating the container's
> resolv.conf with the 'domain', 'search', and 'options' keywords.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 84dc157e7d9e332a6da0f1fc33303e9ef9bdc147 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> ccd511ec14810dcc1020dec5e1641141f3a319b4 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 
> ac48159dadcea422f605e723db94a7f3bb573fa2 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> cb893d3ef005a9cc60c40768fa669b27c4863020 
> 
> Diff: https://reviews.apache.org/r/55790/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[James Peach]

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

(Updated Jan. 26, 2017, 11:19 p.m.)


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


Changes
---

Rebase and address review feedback.


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


Repository: mesos


Description
---

Add support for the full set of DNS resolver configuration items that
a CNI IPAM plugin can specify. This implements updating the container's
resolv.conf with the 'domain', 'search', and 'options' keywords.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
84dc157e7d9e332a6da0f1fc33303e9ef9bdc147 
  src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
ccd511ec14810dcc1020dec5e1641141f3a319b4 
  src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 
ac48159dadcea422f605e723db94a7f3bb573fa2 
  src/tests/containerizer/cni_isolator_tests.cpp 
cb893d3ef005a9cc60c40768fa669b27c4863020 

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


Testing
---

sudo make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 55829: Updated resources quantity stripping to strip AllocationInfo.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Jan. 22, 2017, 6:09 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55829/
> ---
> 
> (Updated Jan. 22, 2017, 6:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6965
> https://issues.apache.org/jira/browse/MESOS-6965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `Resources::createStrippedScalarQuantity()` attempts to
> create a notion of a "quantity" of resources. In order to do this,
> all distinguishing metadata between resources of the same name are
> stripped. This currently includes, disk metadata, reservation
> metadata (only for dynamic reservations), and shared resource
> metadata. To maintain the notion of a quantity, this patch also
> strips the allocation metadata.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55829/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 56001: Fixed a few executor segfaults during cleanup.

[Anand Mazumdar]

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



Nice catch!

As per our offline discussion, can you modify an existing test for both the 
default/docker executor to also check if the `status` field is set, it 
shouldn't have a non-zero value.


src/docker/executor.cpp (lines 814 - 815)


Can you add a comment here for posterity as to why this is needed?



src/docker/executor.cpp (line 817)


Not yours, but we should have a comment here too as to why we need an 
explicit `finalize()` here in a separate patch.



src/launcher/default_executor.cpp (line 1142)


Ditto.


- Anand Mazumdar


On Jan. 26, 2017, 10:28 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56001/
> ---
> 
> (Updated Jan. 26, 2017, 10:28 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6989
> https://issues.apache.org/jira/browse/MESOS-6989
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The addition of `process::finalize` at the end of some binaries
> led to some segfaulting at the end of the binaries' lifetimes.
> This is mostly due calling destructors of libprocess actor
> wrappers, which require an initialized libprocess to function.
> 
> This commit explicitly calls the destructors on the default
> and docker executor actors prior to calling `process::finalize`.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp f353341c9509532653e2ef8802f20259ce67930c 
>   src/launcher/default_executor.cpp a03794934adb93868734f8cf00b337a1bff9b5ab 
> 
> Diff: https://reviews.apache.org/r/56001/diff/
> 
> 
> Testing
> ---
> 
> sudo src/mesos-tests --gtest_filter="*ROOT*DOCKER*" --verbose
> 
> Checked that all executors exited with status code 0, rather than 11.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 55790: Support the full CNI DNS specification.

[James Peach]

> On Jan. 25, 2017, 6:15 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 1006
> > 
> >
> > Can we keep the `LOG(INFO)` dumping the contents of `resolv.conf`?

This seems more usefult for debugging that for operations. Maybe a VLOG? I'm 
also not sure that nameservers are the what you want to see in this.


- James


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


On Jan. 20, 2017, 10:45 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55790/
> ---
> 
> (Updated Jan. 20, 2017, 10:45 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6858
> https://issues.apache.org/jira/browse/MESOS-6858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for the full set of DNS resolver configuration items that
> a CNI IPAM plugin can specify. This implements updating the container's
> resolv.conf with the 'domain', 'search', and 'options' keywords.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> e1cac954848e618a03ddb82fd6d040ae1d948e82 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> ccd511ec14810dcc1020dec5e1641141f3a319b4 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 
> ac48159dadcea422f605e723db94a7f3bb573fa2 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> cb893d3ef005a9cc60c40768fa669b27c4863020 
> 
> Diff: https://reviews.apache.org/r/55790/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 56001: Fixed a few executor segfaults during cleanup.

[Joseph Wu]

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

Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

The addition of `process::finalize` at the end of some binaries
led to some segfaulting at the end of the binaries' lifetimes.
This is mostly due calling destructors of libprocess actor
wrappers, which require an initialized libprocess to function.

This commit explicitly calls the destructors on the default
and docker executor actors prior to calling `process::finalize`.


Diffs
-

  src/docker/executor.cpp f353341c9509532653e2ef8802f20259ce67930c 
  src/launcher/default_executor.cpp a03794934adb93868734f8cf00b337a1bff9b5ab 

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


Testing
---

sudo src/mesos-tests --gtest_filter="*ROOT*DOCKER*" --verbose

Checked that all executors exited with status code 0, rather than 11.


Thanks,

Joseph Wu

Re: Review Request 55599: CMake: Added `GroupSource` function to automate IDE source grouping.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Jan. 26, 2017, 1:12 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55599/
> ---
> 
> (Updated Jan. 26, 2017, 1:12 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CMake allows users to declare groups of source files, which it uses to
> (among other things) present source in a directory-like tree of files in
> IDEs like XCode and Visual Studio.
> 
> Currently this is a manual process: we group the source in each folder
> manually and declare it as a source group to CMake.
> 
> This commit will introduce a CMake macro that automates this process
> away, providing control over many aspects, such as where the group tree
> should be rooted, what the root should be named, and so on.
> 
> This macro indirectly partially addresses MESOS-3542, which will help us
> to separate out Mesos builds into many libraries, rather than one
> single, monolithic libmesos.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/cmake/GroupSource.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55599/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

(Updated Jan. 26, 2017, 10:06 p.m.)


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


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 49571: Added a benchmark test for allocations.

[Anindya Sinha]

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

(Updated Jan. 26, 2017, 10:06 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Allocations test has the following resource configurations:
(1) REGULAR: Offers from every slave have regular resources.
(2) SHARED: Offers from every slave include a shared resource.
(3) REGULAR: Offers from every alternate slave contain only regular
resources; and offers from every other alternate slave contains
a shared resource.

This test is parameterized based on number of agents, number of
frameworks and resource configuration.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 
  src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
  src/tests/resources_utils.cpp be1feb97be552af582dbba3a54fa5fa0714b3eb2 

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


Testing
---

All tests passed.

Allocations benchmark test results
==
Support of shared resources has a small impact (roughly 10%) on runtime 
performance in allocations as compared to HEAD (without shared resources). 
Also, there is no visible impact in performance when shared resources are added 
in the tests.

Following is a snapshot with 1000 agents and 200 frameworks.

With the patch (and no shared resources)

[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6907us
Added 1000 agents in 2.057098secs
round 0 allocate() took 1.689164secs to make 1000 offers
round 50 allocate() took 1.672373secs to make 1000 offers
round 100 allocate() took 1.680571secs to make 1000 offers
round 150 allocate() took 1.674683secs to make 1000 offers
round 199 allocate() took 1.671525secs to make 1000 offers

With the patch (and shared resources on all agents)
---
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
Using 1000 agents and 200 frameworks with resource type 1
Added 200 frameworks in 6888us
Added 1000 agents in 2.096218secs
round 0 allocate() took 1.704491secs to make 1000 offers
round 50 allocate() took 1.718623secs to make 1000 offers
round 100 allocate() took 1.716224secs to make 1000 offers
round 150 allocate() took 1.707343secs to make 1000 offers
round 199 allocate() took 1.727467secs to make 1000 offers

With the patch (and shared resources on alternate agents)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
Using 1000 agents and 200 frameworks with resource type 2
Added 200 frameworks in 7304us
Added 1000 agents in 2.071009secs
round 0 allocate() took 1.689045secs to make 1000 offers
round 50 allocate() took 1.691524secs to make 1000 offers
round 100 allocate() took 1.688873secs to make 1000 offers
round 150 allocate() took 1.688713secs to make 1000 offers
round 199 allocate() took 1.691223secs to make 1000 offers

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6801us
Added 1000 agents in 1.721447secs
round 0 allocate() took 1.502953secs to make 1000 offers
round 50 allocate() took 1.520157secs to make 1000 offers
round 100 allocate() took 1.517221secs to make 1000 offers
round 150 allocate() took 1.526446secs to make 1000 offers
round 199 allocate() took 1.538005secs to make 1000 offers


Thanks,

Anindya Sinha

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Anindya Sinha]

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

(Updated Jan. 26, 2017, 10:06 p.m.)


Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

We handle shared resources for `LAUNCH` operations separately in the
`updateAllocation()` API to add additional copies of shared resources.
But the updates to the allocations in the allocator and sorters
can be consolidated together.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
c2211be7458755aeb91ef078e4bfe92ac474044a 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

[Anindya Sinha]

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

(Updated Jan. 26, 2017, 10:06 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we
remove the previous resources at this agent and add the new resources
at this agent.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
9b66c23f26b37c02ed850c06c4518ea99078b02d 
  src/master/allocator/mesos/hierarchical.cpp 
c2211be7458755aeb91ef078e4bfe92ac474044a 
  src/tests/persistent_volume_tests.cpp 
468a85b4a6ce09592341afd07ce12a03f5fc4f73 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 55550: Windows: Enabled health checker tests.

[Joseph Wu]

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


Ship it!




Just a few minor things that I can tweak before committing.


src/tests/health_check_tests.cpp (lines 705 - 714)


No need to duplicate this comment.



src/tests/health_check_tests.cpp (lines 1167 - 1169)


Whitespace can be removed here.



src/tests/health_check_tests.cpp (lines 1199 - 1208)


Same here.


- Joseph Wu


On Jan. 15, 2017, 1:35 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/0/
> ---
> 
> (Updated Jan. 15, 2017, 1:35 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6709
> https://issues.apache.org/jira/browse/MESOS-6709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Enabled health checker tests.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 0a6d2dd295408dcc0434f3573e307e685f9abfe4 
> 
> Diff: https://reviews.apache.org/r/0/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 55971: Updated the agent to be MULTI_ROLE capable.

[Benjamin Mahler]

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

Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
Park.


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


Repository: mesos


Description
---

With the addition of MULTI_ROLE support, the agent needs to ensure
that allocated resources reported to the master have the
`Resource.AllocationInfo` set. The approach taken here is to set
it only in memory upon recovering tasks and executors. Note that
once we allow a framework to modify its roles, we need to update
the agent to re-persist the resources with injected allocation
info (rather than just setting it in memory).


Diffs
-

  src/slave/resource_estimators/fixed.cpp 
2c1268c3423091c6a419020a3af97255de55db0a 
  src/slave/slave.cpp 0548b04073c0ba4adfc4433d75fd18c2ba79d891 

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


Testing
---

The tests pass at the end of this review chain.


Thanks,

Benjamin Mahler

Re: Review Request 55549: Windows: Added health checker to build.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Jan. 26, 2017, 12:45 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55549/
> ---
> 
> (Updated Jan. 26, 2017, 12:45 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6709
> https://issues.apache.org/jira/browse/MESOS-6709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a blocker for MESOS-6709, which requires both the health checker
> and the TCP connector to build and work on Windows.
> 
> 
> Diffs
> -
> 
>   src/checks/CMakeLists.txt ce195e2820c70dd7ebec1f06a6382f58fc729af7 
>   src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5 
>   src/checks/tcp_connect.cpp ad1e932da53c8f6b0ae77dfa6b6bb3d642273af9 
> 
> Diff: https://reviews.apache.org/r/55549/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55549: Windows: Added health checker to build.

[Joseph Wu]

> On Jan. 24, 2017, 1:37 p.m., Joseph Wu wrote:
> > src/health-check/tcp_connect.cpp, lines 33-38
> > 
> >
> > Libprocess headers should go above stout headers.
> 
> Alex Clemmer wrote:
> Oh, did this change recently, or have I just always done this wrong?

You've gotten this right in the past.  But it's easy to forget.


- Joseph


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


On Jan. 26, 2017, 12:45 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55549/
> ---
> 
> (Updated Jan. 26, 2017, 12:45 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6709
> https://issues.apache.org/jira/browse/MESOS-6709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a blocker for MESOS-6709, which requires both the health checker
> and the TCP connector to build and work on Windows.
> 
> 
> Diffs
> -
> 
>   src/checks/CMakeLists.txt ce195e2820c70dd7ebec1f06a6382f58fc729af7 
>   src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5 
>   src/checks/tcp_connect.cpp ad1e932da53c8f6b0ae77dfa6b6bb3d642273af9 
> 
> Diff: https://reviews.apache.org/r/55549/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55600: CMake: Transitioned Stout to automatic source grouping.

[Joseph Wu]

> On Jan. 24, 2017, 4:27 p.m., Joseph Wu wrote:
> > 3rdparty/stout/cmake/StoutConfigure.cmake, line 47
> > 
> >
> > Why not use `*.hpp` instead?
> 
> Alex Clemmer wrote:
> Generally, we also would like to include .h files generated by protobufs, 
> either those that exist, or those that might exist in the future. In Stout, 
> for example, there is `protobuf_tests.proto` in the `tests/` folder. The 
> Stout headers don't have any .proto files that I know of right now, but I 
> think it's better just to use this one pattern everywhere.

Ok, but let's be explicit, say `*.h(pp)?`.  Otherwise, we'll be matching 
against `.hooligans` and `.hoopla` :)

Similar below.


- Joseph


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


On Jan. 16, 2017, 10:41 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55600/
> ---
> 
> (Updated Jan. 16, 2017, 10:41 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout currently manually groups its source code by folder in order to
> present the source nicely in IDEs like XCode and Visual Studio.
> 
> With the recently-introduced CMake function, `GROUP_SOURCE`, this
> process is automated away. This commit will remove these manual groups
> and replace it with a call to this function.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/cmake/StoutConfigure.cmake 
> bc27ac687bae4e1798eece562027ba33c6b32348 
>   3rdparty/stout/tests/CMakeLists.txt 
> a8a3ac772aa243c848a6fd8d7a0d45acfe1b98ae 
> 
> Diff: https://reviews.apache.org/r/55600/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 55995: Restored quota correctly during allocator recovery.

[Neil Conway]

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

Review request for mesos, Alexander Rukletsov and Benjamin Bannier.


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


Repository: mesos


Description
---

The previous coding neglected to restore the quota during allocator
recovery if the expected agent count is zero.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
c2211be7458755aeb91ef078e4bfe92ac474044a 
  src/tests/master_quota_tests.cpp 54ae66f09a0b9d1e3be0c44bd4a8bbf5d27ba688 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 55946: Added agent capabilities as part of agent (re-)registration process.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Jan. 26, 2017, 11:46 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55946/
> ---
> 
> (Updated Jan. 26, 2017, 11:46 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent capabilities are communicated during registration and stored
> in `Slave` struct in master. Upon agent reregistration, capabilities
> are updated.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp cb1b82d93919edf910bfbece8ef1a33cade9dc8b 
>   src/master/master.cpp 2432ed4844dcf52f26ef5b4132d5ad429d1b4317 
> 
> Diff: https://reviews.apache.org/r/55946/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 55944: Modified agent capabilities struct to reflect protobuf changes.

[Benjamin Mahler]

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


Ship it!





src/master/master.hpp (lines 243 - 247)


It's a little odd that master was touched in this patch, should we put this 
in your next patch?


- Benjamin Mahler


On Jan. 26, 2017, 11:46 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55944/
> ---
> 
> (Updated Jan. 26, 2017, 11:46 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As agent capabilities are moved from SlaveInfo into Registration
> messages, this patch modifies the data structure to reflect that
> change.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp cb1b82d93919edf910bfbece8ef1a33cade9dc8b 
>   src/master/master.cpp 2432ed4844dcf52f26ef5b4132d5ad429d1b4317 
>   src/tests/protobuf_utils_tests.cpp 39604e99d684b018adfcb7d6adf1826a0cf1e22c 
> 
> Diff: https://reviews.apache.org/r/55944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 55945: Added a `install` template method to support 8 arguments.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Jan. 26, 2017, 11:46 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55945/
> ---
> 
> (Updated Jan. 26, 2017, 11:46 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> ReregisterSlave method will need to take agent capabilities as the
> eighth argument. So we need `install` to support 8 arguments.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/protobuf.hpp 
> ad2f2322a5d431f017422c0f6c922fda57ee9dc2 
> 
> Diff: https://reviews.apache.org/r/55945/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 55943: Moved agent capabilities from SlaveInfo to (re-)registerSlaveMessage.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Jan. 26, 2017, 11:11 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55943/
> ---
> 
> (Updated Jan. 26, 2017, 11:11 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We want agents to be able to upgrade to support new capabilities after
> reregistration, however we don't support udpating SlaveInfo yet. This
> patch moves agent capabilities protobuf message from SlaveInfo to the
> (re-)registerSlaveMessage so that capabilities are communicated through
> the agent registration and re-registration.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 044b7e697f77c9e2cefb938bda4307d328b33766 
>   include/mesos/v1/mesos.proto 7706de89d91926aedb10e4c882dffcf050f03bb1 
>   src/messages/messages.proto 7cbac56099689ffc378d83bae3a16abb49b50dd9 
> 
> Diff: https://reviews.apache.org/r/55943/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 53517: Added test case for cgroup namespace isolator.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [53296, 53516, 54105, 53517]

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

- Mesos Reviewbot


On Jan. 26, 2017, 8:19 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53517/
> ---
> 
> (Updated Jan. 26, 2017, 8:19 a.m.)
> 
> 
> Review request for mesos, Jason Lai, Jie Yu, James Peach, Qian Zhang, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5410
> https://issues.apache.org/jira/browse/MESOS-5410
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case for cgroup namespace isolator.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/namespaces_isolator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53517/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 55954: Changed 'Environment.Variable.Value' from required to optional.

[Jan Schlicht]

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


Ship it!




- Jan Schlicht


On Jan. 26, 2017, 3:36 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55954/
> ---
> 
> (Updated Jan. 26, 2017, 3:36 a.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Bugs: MESOS-6991
> https://issues.apache.org/jira/browse/MESOS-6991
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To prepare for future work which will enable the
> modular fetching of secrets, we should change the
> Environment.Variable.Value field from required to
> optional. This way, the field can be left empty
> and filled in by a secret fetching module.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 044b7e697f77c9e2cefb938bda4307d328b33766 
>   include/mesos/v1/mesos.proto 7706de89d91926aedb10e4c882dffcf050f03bb1 
>   src/checks/checker.cpp 8d7285af40d9633608178ec239d3b8d65d3a2725 
>   src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5 
>   src/common/validation.hpp fee955139d9699caa651d9bbd03342f2eba8143f 
>   src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b 
>   src/master/validation.cpp e3f71be16ff21a1b4e10ad59846d9b06691bc1b9 
>   src/slave/validation.cpp 3fd32feb3cd0e6d30a66a917e20fd9ca50b84dc2 
> 
> Diff: https://reviews.apache.org/r/55954/diff/
> 
> 
> Testing
> ---
> 
> Testing information at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 55942: Added a test for validating streaming request/response headers.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55936, 55937, 55938, 55939, 55978, 55940, 55941, 55942]

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

- Mesos Reviewbot


On Jan. 26, 2017, 4:49 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55942/
> ---
> 
> (Updated Jan. 26, 2017, 4:49 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6936
> https://issues.apache.org/jira/browse/MESOS-6936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for validating streaming request/response headers.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 400ac6fcd420508509291249cad50679fbe9807d 
> 
> Diff: https://reviews.apache.org/r/55942/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 55946: Added agent capabilities as part of agent (re-)registration process.

[Jay Guo]

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

(Updated Jan. 26, 2017, 7:46 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

rebase


Repository: mesos


Description (updated)
---

Agent capabilities are communicated during registration and stored
in `Slave` struct in master. Upon agent reregistration, capabilities
are updated.


Diffs (updated)
-

  src/master/master.hpp cb1b82d93919edf910bfbece8ef1a33cade9dc8b 
  src/master/master.cpp 2432ed4844dcf52f26ef5b4132d5ad429d1b4317 

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 55945: Added a `install` template method to support 8 arguments.

[Jay Guo]

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

(Updated Jan. 26, 2017, 7:46 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

rebase


Repository: mesos


Description
---

ReregisterSlave method will need to take agent capabilities as the
eighth argument. So we need `install` to support 8 arguments.


Diffs (updated)
-

  3rdparty/libprocess/include/process/protobuf.hpp 
ad2f2322a5d431f017422c0f6c922fda57ee9dc2 

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 55944: Modified agent capabilities struct to reflect protobuf changes.

[Jay Guo]

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

(Updated Jan. 26, 2017, 7:46 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

rebase


Repository: mesos


Description
---

As agent capabilities are moved from SlaveInfo into Registration
messages, this patch modifies the data structure to reflect that
change.


Diffs (updated)
-

  src/master/master.hpp cb1b82d93919edf910bfbece8ef1a33cade9dc8b 
  src/master/master.cpp 2432ed4844dcf52f26ef5b4132d5ad429d1b4317 
  src/tests/protobuf_utils_tests.cpp 39604e99d684b018adfcb7d6adf1826a0cf1e22c 

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 55943: Moved agent capabilities from SlaveInfo to (re-)registerSlaveMessage.

[Jay Guo]

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

(Updated Jan. 26, 2017, 7:11 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

rebase


Repository: mesos


Description
---

We want agents to be able to upgrade to support new capabilities after
reregistration, however we don't support udpating SlaveInfo yet. This
patch moves agent capabilities protobuf message from SlaveInfo to the
(re-)registerSlaveMessage so that capabilities are communicated through
the agent registration and re-registration.


Diffs (updated)
-

  include/mesos/mesos.proto 044b7e697f77c9e2cefb938bda4307d328b33766 
  include/mesos/v1/mesos.proto 7706de89d91926aedb10e4c882dffcf050f03bb1 
  src/messages/messages.proto 7cbac56099689ffc378d83bae3a16abb49b50dd9 

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 53993: Updated quota doc to support quota update.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [53993, 54135, 53991, 52103, 53691, 53679, 52284]

Failed command: python support/apply-reviews.py -n -r 52103

Error:
2017-01-26 11:06:12 URL:https://reviews.apache.org/r/52103/diff/raw/ 
[27771/27771] -> "52103.patch" [1]
error: patch failed: src/master/quota_handler.cpp:130
error: src/master/quota_handler.cpp: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/16861/console

- Mesos Reviewbot


On Nov. 28, 2016, 7:26 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53993/
> ---
> 
> (Updated Nov. 28, 2016, 7:26 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Xiaojian Huang.
> 
> 
> Bugs: MESOS-4941
> https://issues.apache.org/jira/browse/MESOS-4941
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated quota doc to support quota update.
> 
> 
> Diffs
> -
> 
>   docs/quota.md d7e8d58eeeac9d9ffc1a6f7614c7ddce6d7a9b89 
> 
> Diff: https://reviews.apache.org/r/53993/diff/
> 
> 
> Testing
> ---
> 
> https://github.com/zhitaoli/mesos/blob/quota_update_sept/docs/quota.md
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 54987: Updated `docs/monitoring/md` for new agent event queue metrics.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [54986, 54987]

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

- Mesos Reviewbot


On Jan. 26, 2017, 4:29 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54987/
> ---
> 
> (Updated Jan. 26, 2017, 4:29 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Jason Lai.
> 
> 
> Bugs: MESOS-6831
> https://issues.apache.org/jira/browse/MESOS-6831
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `docs/monitoring/md` for new agent event queue metrics.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md f32ee40278afea60f8b1a836798d6fe063d1f222 
> 
> Diff: https://reviews.apache.org/r/54987/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 55893: Fixed OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable.

[Guangya Liu]

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



I think that your patch need depend on /r/51027 , also in `Testing Done` 
section, can you please make the test repeat or use `--gtest_repeat=-1 
--gtest_break_on_failure` to make sure the test is not flaky?


src/tests/oversubscription_tests.cpp (line 507)


How about s/offer1/offer as here you only have one such variable.



src/tests/oversubscription_tests.cpp (line 543)


How about following?

```
// The latest resource estimate should match the total offered resources.
```

This can also make the comments match `resources2 and resources3`.


- Guangya Liu


On 一月 24, 2017, 9:52 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55893/
> ---
> 
> (Updated 一月 24, 2017, 9:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Jacob Janco.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is another test fix in the chain.
> 
> 
> Diffs
> -
> 
>   src/tests/oversubscription_tests.cpp 
> 22ae069ab71c82c6a6e2f5783b13ebd74a9ccf25 
> 
> Diff: https://reviews.apache.org/r/55893/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 55607: CMake: Added configuration of test scripts in the bin/ directory.

[Alex Clemmer]

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

(Updated Jan. 26, 2017, 9:20 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Changes
---

Address Joseph's questions.


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


Repository: mesos


Description
---

This resolves MESOS-6757.


Diffs (updated)
-

  cmake/MesosConfigure.cmake ce127e1e8c5c33ad8badef6fb3ac9f50ade9056f 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55607: CMake: Added configuration of test scripts in the bin/ directory.

[Alex Clemmer]

> On Jan. 25, 2017, 1:07 a.m., Joseph Wu wrote:
> > cmake/MesosConfigure.cmake, line 217
> > 
> >
> > How about moving the file instead?  And cleaning up the bin/tmp folder?

Unless I'm missing something, `COPY` is the preferred way to "move" a file, 
particularly if you want to set permissions. We can `REMOVE_RECURSE` to get rid 
of this `tmp/` folder though, and it should look basically identical to a move 
to a user.


> On Jan. 25, 2017, 1:07 a.m., Joseph Wu wrote:
> > cmake/MesosConfigure.cmake, line 212
> > 
> >
> > This part ( 
> > https://cmake.org/cmake/help/v3.0/command/configure_file.html ): ```
> > If the  file is modified the build system will re-run CMake to 
> > re-configure the file and generate the build system again.
> > ```
> > 
> > is a little unfortunate, but that's better than the automake, which 
> > doesn't always regenerate these template files when the underlying ones get 
> > changed.

It does suck, but I'm not sure what the alternatives are.


- Alex


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


On Jan. 17, 2017, 8:34 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55607/
> ---
> 
> (Updated Jan. 17, 2017, 8:34 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6757
> https://issues.apache.org/jira/browse/MESOS-6757
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This resolves MESOS-6757.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc 
> 
> Diff: https://reviews.apache.org/r/55607/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55893: Fixed OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable.

[Guangya Liu]

> On 一月 25, 2017, 9:55 a.m., Guangya Liu wrote:
> > @Yan, I posted some comments at https://reviews.apache.org/r/51027/ for 
> > this issue with some comments as:
> > 
> > ```
> > Jacob, regaring the test failure of 
> > OversubscriptionTest.RescindRevocableOfferWithIncreasedRevocable, I think 
> > that you can disable it as 
> > DISABLED_RescindRevocableOfferWithIncreasedRevocable in your patch, and I 
> > will fix this in /r/51621
> > 
> > I found that with current logic, it is difficult to handle this case, as we 
> > cannot make sure the complete order for allocator->updateSlave and 
> > allocator->recoverResources, if allocator->recoverResources finished first, 
> > then the offer will have 3 REV resources; if allocator->updateSlave 
> > finished first, then the offer will have 2 REV resources.
> > 
> > With /r/51621 , after we enable recoverResources allocate resources, we can 
> > always make sure we get 3 REV resources in the offer, so I propose that you 
> > disable the test case first and I will fix this after we enable allocate 
> > resources when recoverResoures.
> > ```
> > 
> > As I will have a patch for `allocate resources when recoverResoures`, so 
> > how about `disable` this test first and then I can fix this when my patch 
> > ready in https://reviews.apache.org/r/51621/ ?
> 
> Jiang Yan Xu wrote:
> Hey Guangya I took a look at your fix in /r/51621/ but had a question, 
> when `recoverResources()` also triggers allocations. it's true that we don't 
> need `Clock::advance` to trigger a periodic allocation anymore but the race 
> still exists, i.e., you don't know how many `Offers` events the scheduler is 
> going to get, right? With my fix you won't need to care about the number of 
> events since you really only care about the number/amount of offers (you 
> eventually get). So I think this fix will work with your patch as well?

@Yan, you are right. Your fix can also help `allocate when recoverResources`, 
my fix in /r/51621/ is still flaky for this. Thanks.


- Guangya


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


On 一月 24, 2017, 9:52 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55893/
> ---
> 
> (Updated 一月 24, 2017, 9:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Jacob Janco.
> 
> 
> Bugs: MESOS-6904
> https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is another test fix in the chain.
> 
> 
> Diffs
> -
> 
>   src/tests/oversubscription_tests.cpp 
> 22ae069ab71c82c6a6e2f5783b13ebd74a9ccf25 
> 
> Diff: https://reviews.apache.org/r/55893/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 55599: CMake: Added `GroupSource` function to automate IDE source grouping.

[Alex Clemmer]

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

(Updated Jan. 26, 2017, 9:12 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Changes
---

Address Joseph's questions.


Repository: mesos


Description
---

CMake allows users to declare groups of source files, which it uses to
(among other things) present source in a directory-like tree of files in
IDEs like XCode and Visual Studio.

Currently this is a manual process: we group the source in each folder
manually and declare it as a source group to CMake.

This commit will introduce a CMake macro that automates this process
away, providing control over many aspects, such as where the group tree
should be rooted, what the root should be named, and so on.

This macro indirectly partially addresses MESOS-3542, which will help us
to separate out Mesos builds into many libraries, rather than one
single, monolithic libmesos.


Diffs (updated)
-

  3rdparty/stout/cmake/GroupSource.cmake PRE-CREATION 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55600: CMake: Transitioned Stout to automatic source grouping.

[Alex Clemmer]

> On Jan. 25, 2017, 12:27 a.m., Joseph Wu wrote:
> > 3rdparty/stout/cmake/StoutConfigure.cmake, line 47
> > 
> >
> > Why not use `*.hpp` instead?

Generally, we also would like to include .h files generated by protobufs, 
either those that exist, or those that might exist in the future. In Stout, for 
example, there is `protobuf_tests.proto` in the `tests/` folder. The Stout 
headers don't have any .proto files that I know of right now, but I think it's 
better just to use this one pattern everywhere.


- Alex


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


On Jan. 17, 2017, 6:41 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55600/
> ---
> 
> (Updated Jan. 17, 2017, 6:41 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout currently manually groups its source code by folder in order to
> present the source nicely in IDEs like XCode and Visual Studio.
> 
> With the recently-introduced CMake function, `GROUP_SOURCE`, this
> process is automated away. This commit will remove these manual groups
> and replace it with a call to this function.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/cmake/StoutConfigure.cmake 
> bc27ac687bae4e1798eece562027ba33c6b32348 
>   3rdparty/stout/tests/CMakeLists.txt 
> a8a3ac772aa243c848a6fd8d7a0d45acfe1b98ae 
> 
> Diff: https://reviews.apache.org/r/55600/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55549: Windows: Added health checker to build.

[Alex Clemmer]

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

(Updated Jan. 26, 2017, 8:45 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Changes
---

Address Joseph's comments.


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


Repository: mesos


Description
---

This is a blocker for MESOS-6709, which requires both the health checker
and the TCP connector to build and work on Windows.


Diffs (updated)
-

  src/checks/CMakeLists.txt ce195e2820c70dd7ebec1f06a6382f58fc729af7 
  src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5 
  src/checks/tcp_connect.cpp ad1e932da53c8f6b0ae77dfa6b6bb3d642273af9 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55549: Windows: Added health checker to build.

[Alex Clemmer]

> On Jan. 24, 2017, 9:37 p.m., Joseph Wu wrote:
> > src/health-check/tcp_connect.cpp, lines 33-38
> > 
> >
> > Libprocess headers should go above stout headers.

Oh, did this change recently, or have I just always done this wrong?


> On Jan. 24, 2017, 9:37 p.m., Joseph Wu wrote:
> > src/health-check/tcp_connect.cpp, lines 148-150
> > 
> >
> > Should there be a `process::finalize` here?

Actually, now that I think about it, this should not take a dependency on 
libprocess at all. Let's use the Stout WSA functions instead.


- Alex


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


On Jan. 15, 2017, 9:35 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55549/
> ---
> 
> (Updated Jan. 15, 2017, 9:35 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6709
> https://issues.apache.org/jira/browse/MESOS-6709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a blocker for MESOS-6709, which requires both the health checker
> and the TCP connector to build and work on Windows.
> 
> 
> Diffs
> -
> 
>   src/health-check/CMakeLists.txt ce195e2820c70dd7ebec1f06a6382f58fc729af7 
>   src/health-check/health_checker.cpp 
> a8424b75927d15dc1b897faf0e47cf075c70ff26 
>   src/health-check/tcp_connect.cpp ad1e932da53c8f6b0ae77dfa6b6bb3d642273af9 
> 
> Diff: https://reviews.apache.org/r/55549/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 53517: Added test case for cgroup namespace isolator.

[haosdent huang]

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

(Updated Jan. 26, 2017, 8:19 a.m.)


Review request for mesos, Jason Lai, Jie Yu, James Peach, Qian Zhang, and Jiang 
Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added test case for cgroup namespace isolator.


Diffs (updated)
-

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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 53516: Moved `namespaces/pid` associated test cases to a separate file.

[haosdent huang]

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

(Updated Jan. 26, 2017, 8:18 a.m.)


Review request for mesos, Jason Lai, Jie Yu, James Peach, Qian Zhang, and Jiang 
Yan Xu.


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


Repository: mesos


Description
---

Moved `namespaces/pid` associated test cases to a separate file.


Diffs (updated)
-

  src/Makefile.am 2cc0ad7e3c843e2f9baffa7b59640172dcd7628d 
  src/tests/CMakeLists.txt eebbc06ce5d5123ac38d781f6fc1b58dddf3ac4f 
  src/tests/containerizer/isolator_tests.cpp 
355e15ff69ca6bdd94821f6566fd09a280d03b47 
  src/tests/containerizer/namespaces_isolator_tests.cpp PRE-CREATION 
  src/tests/slave_recovery_tests.cpp 9cf1d461cf5c6ac1f51ac963360a17e37f558505 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 53296: Added cgroup namespace support for unified container.

[haosdent huang]

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

(Updated Jan. 26, 2017, 8:18 a.m.)


Review request for mesos, Jason Lai, Jie Yu, James Peach, Qian Zhang, and Jiang 
Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added cgroup namespace support for unified container.


Diffs (updated)
-

  src/CMakeLists.txt 348eb2c314107409a5370fdd24b4735c57f6934c 
  src/Makefile.am 2cc0ad7e3c843e2f9baffa7b59640172dcd7628d 
  src/slave/containerizer/mesos/containerizer.cpp 
4f0a773676da45fa40ad1ad9cdfab2a19249247d 
  src/slave/containerizer/mesos/isolators/namespaces/cgroup.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/namespaces/cgroup.cpp PRE-CREATION 

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


Testing
---

Test case is: https://reviews.apache.org/r/53517/


Thanks,

haosdent huang

Re: Review Request 55955: Added validation tests to ensure environment variable value is set.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55954, 55955]

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

- Mesos Reviewbot


On Jan. 26, 2017, 2:36 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55955/
> ---
> 
> (Updated Jan. 26, 2017, 2:36 a.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Bugs: MESOS-6991
> https://issues.apache.org/jira/browse/MESOS-6991
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `value` field within `Environment::Variable` is being
> changed to `optional`, but for the time being we will
> enforce that it must be set for backward compatibility.
> This patch adds tests to ensure that environment variables
> with unset values are correctly rejected.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4 
>   src/tests/health_check_tests.cpp debbd3c09b7555145aaf3f62a24d795d1423a269 
>   src/tests/master_validation_tests.cpp 
> edb57407e08cdbd8fbf10a9e1493cab3b4979bb8 
>   src/tests/slave_validation_tests.cpp 
> 5de771114982751e7796f55dcacd4384c6989efb 
> 
> Diff: https://reviews.apache.org/r/55955/diff/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --gtest_filter="*Validation*"
> 
> 
> Thanks,
> 
> Greg Mann
> 
>