Re: Review Request 50081: Refactored docker/docker.cpp to use path::absolute().

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50081]

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

- Mesos ReviewBot


On July 15, 2016, 7:07 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50081/
> ---
> 
> (Updated July 15, 2016, 7:07 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored docker/docker.cpp to use path::absolute().
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 515842d381ca8a91ad481f66c7be057dff2f3f28 
> 
> Diff: https://reviews.apache.org/r/50081/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49913: Moved createFrameworkInfo() function definition to tests/mesos.hpp.

[Jay Guo]

> On July 12, 2016, 2:24 a.m., Jay Guo wrote:
> > src/tests/mesos.hpp, line 582
> > 
> >
> > why `role1` but not `role`?
> 
> Abhishek Dasgupta wrote:
> There are three reasons for this:
> 
> 1. Renaming to role instead of role1 needs more changes in the codebases.
> 2. In persistant_volume tests, it is testing against JSON for "role : 
> role1", here it looks a little qeer if used "role : role".
> 3. In future, if we need to add more roles, it would be rational to add 
> as "role2", "role3" .. So, this one is named as role1.
> 
> What do you think?

Reasonable, I'll drop the issue. :)


- Jay


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


On July 15, 2016, 11:08 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49913/
> ---
> 
> (Updated July 15, 2016, 11:08 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jay Guo, and Vinod Kone.
> 
> 
> Bugs: MESOS-5725
> https://issues.apache.org/jira/browse/MESOS-5725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There were multiple createFrameworkInfo() definition scattered
> in various testcases. In this patch, only one defintion of
> createFrameworkInfo() is provided in src/tests/mesos.hpp and
> other tests just used this definition.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp b4a6c46cd3e0e57b4f4d22d2103d3947c592e133 
>   src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 2a22f3b0da817e650a25e5e2c951adb7462718b4 
>   src/tests/reservation_endpoints_tests.cpp 
> 48c002d1dc371c285b9421ef5a2c57250d270fa8 
> 
> Diff: https://reviews.apache.org/r/49913/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Review Request 50103: Fixed the incomplete `TaskStatus` message of docker executor.

[haosdent huang]

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

Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


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


Repository: mesos


Description
---

When we enable health check for the tasks run in docker container,
their `TaskStatus` messages generated by docker executor may miss
`NetworkInfo` field and agent would fill the host ip as default value.
In this changes, we cache the `NetworkInfo` of the task and reuse it
when generate `TaskStatus` messages which state is `TASK_RUNNING`.


Diffs
-

  src/docker/executor.cpp 69511044e39bc05d7d6240264ec70b6e6f44edba 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 50064: Added setns and active user test binaries.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50064]

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

- Mesos ReviewBot


On July 15, 2016, 10:18 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50064/
> ---
> 
> (Updated July 15, 2016, 10:18 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added setns and active user test binaries.
> 
> 
> Diffs
> -
> 
>   src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 
>   src/tests/cmake/MesosTestsConfigure.cmake 
> caecce14ca884dcc09ae4ba7649a09f7ae7c1fdf 
>   src/tests/containerizer/CMakeLists.txt 
> 41e792a2c9ec588d4897d60d012e67c606bbe601 
> 
> Diff: https://reviews.apache.org/r/50064/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 50062: Updated makePortRanges for variable types.

[Guangya Liu]

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

(Updated 七月 16, 2016, 3:15 a.m.)


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


Repository: mesos


Description (updated)
---

1) Using `size_t` for counter.
2) Using `uint64_t` for range related values.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
ce5da6be490b6fce311286eb4018c91eef55067e 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Review Request 50062: Updated makePortRanges for variable types.

[Guangya Liu]

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

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


Repository: mesos


Description
---

1) Using `size_t` for counter.
2) Using `unit64_t` for range related values.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
ce5da6be490b6fce311286eb4018c91eef55067e 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 49223: Fixed Value parsing code to only accept the canonical formats.

[Klaus Ma]

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

(Updated July 16, 2016, 10:39 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Update v1/value.cpp & add more cases


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


Repository: mesos


Description (updated)
---

Fixed Value parsing code to only accept the canonical formats.


Diffs (updated)
-

  src/common/values.cpp 7b8a8ea00bd4bd1a82c33be4438c26b2731b1dd8 
  src/tests/values_tests.cpp cc17b675d5d3768685b44a1cea64264dcbca80ba 
  src/v1/values.cpp 3e0f739b69e680f9eb29ed36d4501c393758c861 

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


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

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

(Updated July 16, 2016, 2:14 a.m.)


Review request for mesos, Greg Mann and Vinod Kone.


Changes
---

rebased on latest master to hopefully fix the patch


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


Repository: mesos


Description
---

Propagate work_dir flag from local runs to agents/masters.


Diffs (updated)
-

  src/local/flags.hpp f0af0d2 
  src/local/local.cpp a543aef 
  src/tests/test_framework_test.sh 73b4ddc 

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


Testing
---

Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
regression test for this would be good, please advise.


Thanks,

Ammar Askar

Re: Review Request 50102: Ignored /etc/* mounts to host filesystems if host network is used.

[Gilbert Song]

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


Fix it, then Ship it!





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


Add `do` please?


- Gilbert Song


On July 15, 2016, 6:38 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50102/
> ---
> 
> (Updated July 15, 2016, 6:38 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-5806
> https://issues.apache.org/jira/browse/MESOS-5806
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignored /etc/* mounts to host filesystems if host network is used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 11b826e177cf4db6b80027fd221e84ab85c4efbc 
> 
> Diff: https://reviews.apache.org/r/50102/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 43475: Sped up MasterDetectorExpireSlaveZKSessionNewMaster by advance Clock.

[haosdent huang]

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

(Updated July 16, 2016, 2:11 a.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Changes
---

Rebase.


Summary (updated)
-

Sped up MasterDetectorExpireSlaveZKSessionNewMaster by advance Clock.


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


Repository: mesos


Description (updated)
---

Sped up MasterDetectorExpireSlaveZKSessionNewMaster by advance Clock.


Diffs (updated)
-

  src/tests/master_contender_detector_tests.cpp 
2970986830d0cfaedbf17f3b002c40ac5c28bd2a 

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


Testing
---

Test repeatly in CentOS 7.1

```
 sudo ./bin/mesos-tests.sh 
--gtest_filter="ZooKeeperMasterContenderDetectorTest.MasterDetectorExpireSlaveZKSessionNewMaster"
 --verbose --gtest_repeat=200 --gtest_break_on_failure
```

Before
```
[   OK ] 
ZooKeeperMasterContenderDetectorTest.MasterDetectorExpireSlaveZKSessionNewMaster
 (3359 ms)
```

After

```
[   OK ] 
ZooKeeperMasterContenderDetectorTest.MasterDetectorExpireSlaveZKSessionNewMaster
 (613 ms)
```


Thanks,

haosdent huang

Re: Review Request 43477: Sped up GroupTest.* test cases by advance clock.

[haosdent huang]

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

(Updated July 16, 2016, 2:11 a.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Changes
---

Rebase.


Summary (updated)
-

Sped up GroupTest.* test cases by advance clock.


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


Repository: mesos


Description (updated)
---

Sped up GroupTest.* test cases by advance clock.


Diffs (updated)
-

  src/tests/group_tests.cpp 193a158c4d07591584fc64b6baa41f2e90618732 

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


Testing
---

Test repeatly in CentOS 7.1

```
 sudo ./bin/mesos-tests.sh --gtest_filter="GroupTest.*" --verbose 
--gtest_repeat=200 --gtest_break_on_failure
```

Before

```
[   OK ] GroupTest.GroupJoinWithDisconnect (3352 ms)
[   OK ] GroupTest.GroupDataWithDisconnect (3350 ms)
[   OK ] GroupTest.GroupCancelWithDisconnect (2013 ms)
[   OK ] GroupTest.GroupPathWithRestrictivePerms (13368 ms)
[   OK ] GroupTest.RetryableErrors (26720 ms)
```

After 

```
[   OK ] GroupTest.GroupJoinWithDisconnect (405 ms)
[   OK ] GroupTest.GroupDataWithDisconnect (192 ms)
[   OK ] GroupTest.GroupCancelWithDisconnect (250 ms)
[   OK ] GroupTest.GroupPathWithRestrictivePerms (334 ms)
[   OK ] GroupTest.RetryableErrors (341 ms)
```


Thanks,

haosdent huang

Re: Review Request 43474: Sped up MasterDetectorExpireSlaveZKSession by advance Clock.

[haosdent huang]

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

(Updated July 16, 2016, 2:10 a.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Changes
---

Rebase.


Summary (updated)
-

Sped up MasterDetectorExpireSlaveZKSession by advance Clock.


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


Repository: mesos


Description (updated)
---

Sped up MasterDetectorExpireSlaveZKSession by advance Clock.


Diffs (updated)
-

  src/tests/master_contender_detector_tests.cpp 
2970986830d0cfaedbf17f3b002c40ac5c28bd2a 

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


Testing
---

Test repeatly in CentOS 7.1

```
 sudo ./bin/mesos-tests.sh 
--gtest_filter="ZooKeeperMasterContenderDetectorTest.MasterDetectorExpireSlaveZKSession"
 --verbose --gtest_repeat=200 --gtest_break_on_failure
```

Before

```
[   OK ] 
ZooKeeperMasterContenderDetectorTest.MasterDetectorExpireSlaveZKSession (3358 
ms)
```

After 

```
[   OK ] 
ZooKeeperMasterContenderDetectorTest.MasterDetectorExpireSlaveZKSession (448 ms)
```


Thanks,

haosdent huang

Re: Review Request 43471: Added the zookeeper patch for the slow add_auth calls.

[haosdent huang]

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

(Updated July 16, 2016, 2:09 a.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Changes
---

Rebase.


Summary (updated)
-

Added the zookeeper patch for the slow add_auth calls.


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


Repository: mesos


Description (updated)
---

Added the zookeeper patch for the slow add_auth calls.


Diffs (updated)
-

  3rdparty/zookeeper-06d3f3f.patch be2ceaf529895c92dcf53984d6d88c78fb1d74ec 
  3rdparty/zookeeper-3.4.8.patch 486df1ae96af3426835c9d47ff2e36dd47ccde3f 

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


Testing
---

Before

```
[   OK ] ZooKeeperMasterContenderDetectorTest.NonRetryableFrrors (10053 ms)
[   OK ] MasterZooKeeperTest.MasterInfoAddress (11282 ms)
[   OK ] ZooKeeperTest.Auth (6688 ms)
[   OK ] ZooKeeperTest.Create (6690 ms)
```

After

```
[   OK ] ZooKeeperMasterContenderDetectorTest.NonRetryableFrrors (321 ms)
[   OK ] MasterZooKeeperTest.MasterInfoAddress (447 ms)
[   OK ] ZooKeeperTest.Auth (233 ms)
[   OK ] ZooKeeperTest.Create (275 ms)
```


Thanks,

haosdent huang

Re: Review Request 43472: Sped up ZooKeeperTest.LeaderContender by advance Clock.

[haosdent huang]

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

(Updated July 16, 2016, 2:09 a.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Changes
---

Rebase.


Summary (updated)
-

Sped up ZooKeeperTest.LeaderContender by advance Clock.


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


Repository: mesos


Description (updated)
---

Sped up ZooKeeperTest.LeaderContender by advance Clock.


Diffs (updated)
-

  src/tests/zookeeper_tests.cpp 72279a3397af7fa15f6ecb3c7adb9d149439e8c8 

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


Testing
---

Test repeatly in CentOS 7.1

```
 sudo ./bin/mesos-tests.sh --gtest_filter="ZooKeeperTest.LeaderContender" 
--verbose --gtest_repeat=200 --gtest_break_on_failure
```

Before

```
[   OK ] ZooKeeperTest.LeaderContender (3385 ms)
```

After

```
[   OK ] ZooKeeperTest.LeaderContender (694 ms)
```


Thanks,

haosdent huang

Re: Review Request 43473: Sped up ContenderDetectorShutdownNetwork by advance Clock.

[haosdent huang]

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

(Updated July 16, 2016, 2:09 a.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Changes
---

Rebase.


Summary (updated)
-

Sped up ContenderDetectorShutdownNetwork by advance Clock.


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


Repository: mesos


Description (updated)
---

Sped up ContenderDetectorShutdownNetwork by advance Clock.


Diffs (updated)
-

  src/tests/master_contender_detector_tests.cpp 
2970986830d0cfaedbf17f3b002c40ac5c28bd2a 

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


Testing
---

Test repeatly in CentOS 7.1

```
 sudo ./bin/mesos-tests.sh 
--gtest_filter="ZooKeeperMasterContenderDetectorTest.ContenderDetectorShutdownNetwork
 " --verbose --gtest_repeat=200 --gtest_break_on_failure
```

Before

```
[   OK ] 
ZooKeeperMasterContenderDetectorTest.ContenderDetectorShutdownNetwork (3390 ms)
```

After

```
[   OK ] 
ZooKeeperMasterContenderDetectorTest.ContenderDetectorShutdownNetwork (546 ms)
```


Thanks,

haosdent huang

Re: Review Request 43471: Add the zookeeper patch for the allow add_auth calls.

[haosdent huang]

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

(Updated July 16, 2016, 2:02 a.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


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


Repository: mesos


Description
---

Currently zookeeper multi-threaded c client doesn't wake up the IO
thread after enqueue auth info message. So the auth info message isn't
flushed util poll timeout. We back port this patch from ZOOKEEPER-770
to wake up the IO thread in send_info_packet() just as send_ping()
does.


Diffs
-

  3rdparty/zookeeper-06d3f3f.patch 56480d5f42b65b5b75e40e5e44666f1db3cf2423 
  3rdparty/zookeeper-3.4.8.patch 486df1ae96af3426835c9d47ff2e36dd47ccde3f 

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


Testing
---

Before

```
[   OK ] ZooKeeperMasterContenderDetectorTest.NonRetryableFrrors (10053 ms)
[   OK ] MasterZooKeeperTest.MasterInfoAddress (11282 ms)
[   OK ] ZooKeeperTest.Auth (6688 ms)
[   OK ] ZooKeeperTest.Create (6690 ms)
```

After

```
[   OK ] ZooKeeperMasterContenderDetectorTest.NonRetryableFrrors (321 ms)
[   OK ] MasterZooKeeperTest.MasterInfoAddress (447 ms)
[   OK ] ZooKeeperTest.Auth (233 ms)
[   OK ] ZooKeeperTest.Create (275 ms)
```


Thanks,

haosdent huang

Review Request 50102: Ignored /etc/* mounts to host filesystems if host network is used.

[Jie Yu]

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

Review request for mesos, Avinash sridharan and Qian Zhang.


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


Repository: mesos


Description
---

Ignored /etc/* mounts to host filesystems if host network is used.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
11b826e177cf4db6b80027fd221e84ab85c4efbc 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 50088: Added mesos-protobuf target to build protobuf libraries.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [50088]

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

Error:
2016-07-16 01:28:48 URL:https://reviews.apache.org/r/50088/diff/raw/ 
[1644/1644] -> "50088.patch" [1]
error: patch failed: src/CMakeLists.txt:490
error: src/CMakeLists.txt: patch does not apply

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

- Mesos ReviewBot


On July 15, 2016, 9:54 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50088/
> ---
> 
> (Updated July 15, 2016, 9:54 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-5852
> https://issues.apache.org/jira/browse/MESOS-5852
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mesos-protobuf target to build protobuf libraries.
> 
> This patch builds libmesos-protobufs.1.1.0.dylib that compiles all protobuf 
> structures required by mesos.
> libmesos can depend on this source but not required.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
>   src/CMakeLists.txt 493b6dbd28945332fbda2d3172018e109e73235a 
> 
> Diff: https://reviews.apache.org/r/50088/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make mesos-protobufs
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 50081: Refactored docker/docker.cpp to use path::absolute().

[Timothy Chen]

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


Ship it!




Ship It!

- Timothy Chen


On July 15, 2016, 7:07 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50081/
> ---
> 
> (Updated July 15, 2016, 7:07 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored docker/docker.cpp to use path::absolute().
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 515842d381ca8a91ad481f66c7be057dff2f3f28 
> 
> Diff: https://reviews.apache.org/r/50081/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [50003, 50002]

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

Error:
2016-07-16 00:25:35 URL:https://reviews.apache.org/r/50003/diff/raw/ 
[4380/4380] -> "50003.patch" [1]
error: patch failed: src/tests/test_framework_test.sh:20
error: src/tests/test_framework_test.sh: patch does not apply

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

- Mesos ReviewBot


On July 15, 2016, 9:01 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 15, 2016, 9:01 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
>   src/tests/test_framework_test.sh 617ca52 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50080: Updated Windows build instructions.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50080]

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

- Mesos ReviewBot


On July 15, 2016, 7:29 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50080/
> ---
> 
> (Updated July 15, 2016, 7:29 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds notes about the type of Visual Studio installation expected (C++),
> the git checkout configuration, and the build path on Windows.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md 104816ed4be06ab6be96de9d69cf2b01d60199d0 
> 
> Diff: https://reviews.apache.org/r/50080/diff/
> 
> 
> Testing
> ---
> 
> Visually confirmed (via website preview) that the markdown is sane.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 49784: Increase framework numbers to allocator benchmarks.

[Guangya Liu]

> On 七月 12, 2016, 4:32 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3196
> > 
> >
> > Two issues for this patch:
> > 
> > 1) The new added framework number will introduce some cases that agent 
> > number is less than framework number and some frameworks may not able to 
> > get resources.
> > 2) This will slow down the unit test, it is better introduce this and 
> > fix https://issues.apache.org/jira/browse/MESOS-4558 together by 
> > introducing `batchsize` for benchmark test.
> 
> Alexander Rukletsov wrote:
> 1) I think this a good thing to test / benchmark.
> 2) Benchmark tests are not part of the default test suite, hence it seems 
> fine if this test takes more time to run.
> 
> Guangya Liu wrote:
> For 2), one minor comment is MESOS-4558 is tracking the time of benchmark 
> tests because want to enable them on ASF CI, so it is better to reduce the 
> time of the benchmark test.
> 
> Jiang Yan Xu wrote:
> I commented on MESOS-4558. For this patch I think this is fine because 
> speeding up benchmark tests (or just CI) isn't necessarily at odds with 
> increasing the cluster size. The motivation for the latter is that we do have 
> this many frameworks in our cluster and we'd like to test it.
> 
> Jiang Yan Xu wrote:
> About 1), I don't think in the tests we have the expectation that all 
> frameworks need to be allocated resources in each allocation round? Also this 
> scenario is very much real for us. :)

Thanks Jiang Yan, before the fix, I saw the max framework count is same as the 
minimum agent count, so my thinking is that we always want all of the 
frameworks get resources in the benchmark test. But after some performance test 
recently, I found that this is useful for the evaluating the case `how much 
time does the fully allocated agent contribute to the allocation cycle`, i.e. 
3000 frameworks + 1000 agents, 6000 frameworks + 1000 agents etc.

```
// The Hierarchical Allocator benchmark tests are parameterized
// by the number of slaves.
INSTANTIATE_TEST_CASE_P(
  SlaveAndFrameworkCount,
  HierarchicalAllocator_BENCHMARK_Test,
  ::testing::Combine(
::testing::Values(1000U, 5000U, 1U, 2U, 3U, 5U),
::testing::Values(1U, 50U, 100U, 200U, 500U, 1000U, 3000U, 6000U))
  );
```


- Guangya


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


On 七月 8, 2016, 4:02 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49784/
> ---
> 
> (Updated 七月 8, 2016, 4:02 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5780 and MESOS-5781
> https://issues.apache.org/jira/browse/MESOS-5780
> https://issues.apache.org/jira/browse/MESOS-5781
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Our clusters have very high numbers of frameworks,
>   and we would like to increase the allocator benchmark
>   parameters to reflect this.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 50064: Added setns and active user test binaries.

[Srinivas Brahmaroutu]

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

(Updated July 15, 2016, 10:18 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description (updated)
---

Added setns and active user test binaries.


Diffs (updated)
-

  src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 
  src/tests/cmake/MesosTestsConfigure.cmake 
caecce14ca884dcc09ae4ba7649a09f7ae7c1fdf 
  src/tests/containerizer/CMakeLists.txt 
41e792a2c9ec588d4897d60d012e67c606bbe601 

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


Testing
---

cmake .. && make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49232: Added appcManifest to ImageInfo and ProvisionInfo.

[Guangya Liu]

> On 七月 15, 2016, 1:42 p.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/provisioner/appc/store.cpp, line 199
> > 
> >
> > I recalled Gilbert asked why returning `Future` here but you dropped 
> > the comments without any comment, can you please help explain?
> 
> Gilbert Song wrote:
> It is because we have to do `spec::getManifest()` after we have the 
> ImageId ready.

I see, thanks Gilbert.


- Guangya


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


On 七月 14, 2016, 5:28 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49232/
> ---
> 
> (Updated 七月 14, 2016, 5:28 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added appcManifest to ImageInfo and ProvisionInfo.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e900d752b70165236a198f70c8cb24876a678e4b 
>   src/slave/containerizer/mesos/provisioner/appc/store.cpp 
> aaa0efe63e587b9e604082b52a3cb8c11545fbb9 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 48a05059969e068a0ee0d38b61be9e7104e3188d 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 6825742546a87c8148097e6e13a94592e3b6754e 
>   src/slave/containerizer/mesos/provisioner/store.hpp 
> 1d477ef13ddd24fd8badae0decaa4a2271ecc746 
> 
> Diff: https://reviews.apache.org/r/49232/diff/
> 
> 
> Testing
> ---
> 
> Make Check.
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 50088: Added mesos-protobuf target to build protobuf libraries.

[Joseph Wu]

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




src/CMakeLists.txt (line 415)


The protobuf source should be removed from here.



src/CMakeLists.txt (lines 503 - 504)


The `MESOS_TARGET` needs to depend on the new `MESOS_PROTOBUF_TARGET` too.


- Joseph Wu


On July 15, 2016, 2:54 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50088/
> ---
> 
> (Updated July 15, 2016, 2:54 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-5852
> https://issues.apache.org/jira/browse/MESOS-5852
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mesos-protobuf target to build protobuf libraries.
> 
> This patch builds libmesos-protobufs.1.1.0.dylib that compiles all protobuf 
> structures required by mesos.
> libmesos can depend on this source but not required.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
>   src/CMakeLists.txt 493b6dbd28945332fbda2d3172018e109e73235a 
> 
> Diff: https://reviews.apache.org/r/50088/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make mesos-protobufs
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 42441: Speeded up the `ExamplesTest.*` test cases.

[Benjamin Mahler]

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


Ship it!




Very clean patch! Thanks for writing a good summary and description. :)

- Benjamin Mahler


On July 14, 2016, 9:58 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42441/
> ---
> 
> (Updated July 14, 2016, 9:58 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jian Qiu.
> 
> 
> Bugs: MESOS-4155
> https://issues.apache.org/jira/browse/MESOS-4155
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This lowers the scheduler authentication timeout to speed up the
> `ExamplesTest.*` test cases. Because the master may drop the
> authentication message from the scheduler while it is recovering,
> lowering the authentication timeout could speed up the scheduler
> retries to connect the master.
> 
> 
> Diffs
> -
> 
>   src/tests/dynamic_reservation_framework_test.sh 
> 448dfee8704e39a552fbe2340de6fd1be20b5950 
>   src/tests/java_exception_test.sh fd02322096d09cee6cc22340f18681ad372867b5 
>   src/tests/java_framework_test.sh 8fccc699133c6db6130153c367ce01ba3931c1a2 
>   src/tests/no_executor_framework_test.sh 
> 4fa154ee52454fe3f56161abaa29e58793e627e3 
>   src/tests/persistent_volume_framework_test.sh 
> 074cdcbc4a738170e84887c1773cd7c118112d58 
>   src/tests/python_framework_test.sh dc3a50f4fe8d3340429996622e62f732941985a2 
>   src/tests/test_framework_test.sh 617ca5283bd626b535d0f8091bd1d622d175f6d8 
>   src/tests/test_http_framework_test.sh 
> 0e04ff689d008dea5d2e565b0c58096ecdd24b08 
> 
> Diff: https://reviews.apache.org/r/42441/diff/
> 
> 
> Testing
> ---
> 
> Repeat test in CentOS 7.1
> ```
> $ sudo ./bin/mesos-tests.sh --gtest_filter="ExamplesTest.*" 
> --gtest_repeat=200 --gtest_break_on_failure
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 50026: Made the scheduler authentication timeout configurable.

[Benjamin Mahler]

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


Ship it!




Beautiful, thanks for splitting them apart!

- Benjamin Mahler


On July 14, 2016, 9:58 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50026/
> ---
> 
> (Updated July 14, 2016, 9:58 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-4155
> https://issues.apache.org/jira/browse/MESOS-4155
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the scheduler authentication timeout configurable.
> 
> 
> Diffs
> -
> 
>   src/sched/constants.hpp 75973c97c20927cd18b531cbeab2bf6f69d5538c 
>   src/sched/flags.hpp 43b38d733e963aafc81d69d56569ff20ce8bffa6 
>   src/sched/sched.cpp b9315b53dfbe86500ad1643cc7dfd0f9d6f6a32a 
> 
> Diff: https://reviews.apache.org/r/50026/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 50081: Refactored docker/docker.cpp to use path::absolute().

[Guangya Liu]

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


Ship it!




Thanks Gilbert for the follow up patch.

- Guangya Liu


On 七月 15, 2016, 7:07 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50081/
> ---
> 
> (Updated 七月 15, 2016, 7:07 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored docker/docker.cpp to use path::absolute().
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 515842d381ca8a91ad481f66c7be057dff2f3f28 
> 
> Diff: https://reviews.apache.org/r/50081/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 50064: Added setns and active user test binaries.

[Srinivas Brahmaroutu]

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

(Updated July 15, 2016, 9:56 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Added setns and active user test binaries.


This adds build steps to build executables active-user-test-helper and 
setns-test-helper(linux only) along with mesos-containerizer-memory_test. The 
binaries help in running some tests with specific user and set namespace and 
memory for some container testing.


Diffs
-

  src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 
  src/tests/cmake/MesosTestsConfigure.cmake 
caecce14ca884dcc09ae4ba7649a09f7ae7c1fdf 
  src/tests/containerizer/CMakeLists.txt 
dab8cb07f09f5554123ede4ec8b45b60abf62eee 

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


Testing
---

cmake .. && make check


Thanks,

Srinivas Brahmaroutu

Review Request 50088: Added mesos-protobuf target to build protobuf libraries.

[Srinivas Brahmaroutu]

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

Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Added mesos-protobuf target to build protobuf libraries.

This patch builds libmesos-protobufs.1.1.0.dylib that compiles all protobuf 
structures required by mesos.
libmesos can depend on this source but not required.


Diffs
-

  cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
  src/CMakeLists.txt 493b6dbd28945332fbda2d3172018e109e73235a 

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


Testing
---

cmake .. && make mesos-protobufs


Thanks,

Srinivas Brahmaroutu

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

> On July 15, 2016, 5:41 p.m., Greg Mann wrote:
> > src/local/flags.hpp, line 33
> > 
> >
> > I was originally put off by the duplication of the `work_dir` flag 
> > here, since it makes `work_dir` the only normal Mesos master/agent flag 
> > that can be specified at the command line. Another option I see is to 
> > manually check the value of `MESOS_WORK_DIR` and then set it via 
> > `flags.work_dir = DEFAULT` if necessary. However, I think I prefer the 
> > method of using our flag objects to express the default value rather than 
> > manually checking an environment variable.
> > 
> > Anyway! What do you think about adding a comment here explaining why 
> > we're doing this for `work_dir` but not any other flag? i.e., we need to 
> > set `work_dir` early because it's a required flag and `load` will barf if 
> > it's not already set. Local mode is the one case in which we deem it 
> > acceptable to use a default location within `/tmp` for the `work_dir`.

>since it makes work_dir the only normal Mesos master/agent flag that can be 
>specified at the command line.

Sure but that's just for now, in the future this could be expanded to pass down 
any flags to the agents/master from `local`

>Anyway! What do you think about adding a comment here explaining why we're 
>doing this for work_dir but not any other flag? i.e., we need to set work_dir 
>early because it's a required flag and load will barf if it's not already set. 
>Local mode is the one case in which we deem it acceptable to use a default 
>location within /tmp for the work_dir.

Done


- Ammar


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


On July 15, 2016, 9:01 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 15, 2016, 9:01 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
>   src/tests/test_framework_test.sh 617ca52 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

> On July 15, 2016, 5:41 p.m., Greg Mann wrote:
> > Regarding the regression test: several of our tests use this "local mode" 
> > to test example frameworks (see 'src/tests/examples_tests.cpp'), but they 
> > all set the MESOS_WORK_DIR env var before running. I also noticed that we 
> > don't have any tests which run the 'mesos-local' binary; I created a 
> > separate ticket for that: https://issues.apache.org/jira/browse/MESOS-5850.
> > 
> > One easy thing you could do to cover the issue which led to these patches 
> > is to modify one of the existing example framework test scripts to _not_ 
> > set MESOS_WORK_DIR before running. This would just involve removing lines 
> > like 
> > [these](https://github.com/apache/mesos/blob/4764768fc781bb99294039cebf072748f190bfba/src/tests/test_framework_test.sh#L25-L26).

Done, as a side note, I don't think the `atexit` stuff is working because if I 
do `ls /tmp/mesos*` after running `make check`, I still see a bunch of folders.


- Ammar


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


On July 15, 2016, 9:01 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 15, 2016, 9:01 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
>   src/tests/test_framework_test.sh 617ca52 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Ammar Askar]

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

(Updated July 15, 2016, 9:01 p.m.)


Review request for mesos, Greg Mann and Vinod Kone.


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


Repository: mesos


Description
---

Propagate work_dir flag from local runs to agents/masters.


Diffs (updated)
-

  src/local/flags.hpp f0af0d2 
  src/local/local.cpp a543aef 
  src/tests/test_framework_test.sh 617ca52 

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


Testing
---

Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
regression test for this would be good, please advise.


Thanks,

Ammar Askar

Review Request 50064: Added setns and active user test binaries.

[Srinivas Brahmaroutu]

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

Review request for mesos.


Repository: mesos


Description
---

Added setns and active user test binaries.


This adds build steps to build executables active-user-test-helper and 
setns-test-helper(linux only) along with mesos-containerizer-memory_test. The 
binaries help in running some tests with specific user and set namespace and 
memory for some container testing.


Diffs
-

  src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 
  src/tests/cmake/MesosTestsConfigure.cmake 
caecce14ca884dcc09ae4ba7649a09f7ae7c1fdf 
  src/tests/containerizer/CMakeLists.txt 
dab8cb07f09f5554123ede4ec8b45b60abf62eee 

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


Testing
---

cmake .. && make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49939: Updated Agent::GET_METRICS call to return metrics grouped by types.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49936, 49937, 49938, 49939]

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

- Mesos ReviewBot


On July 15, 2016, 6:30 p.m., Tuan-Anh Hoang-Vu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49939/
> ---
> 
> (Updated July 15, 2016, 6:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5731
> https://issues.apache.org/jira/browse/MESOS-5731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Agent::GET_METRICS call to return metrics grouped by types.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto cfd117de81396bf79049b7642f1ccd1ff4fbb676 
>   include/mesos/v1/agent/agent.proto 213c428d424d8e4f0cc07bd86f1ed59b60df107c 
>   src/slave/http.cpp 21c7ebf7c23fd06bee7125c90576eb892b249b4d 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49939/diff/
> 
> 
> Testing
> ---
> 
> Modified AgentAPITest.GetMetrics to make sure we return metrics grouped by 
> types.
> 
> 
> 1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 2. Run agent: ./bin/mesos-slave.sh --master=127.0.0.1:5050 
> --work_dir=/var/lib/mesos
> 2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
> '{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5051/api/v1 | 
> python -m json.tool
> 
> 
> Thanks,
> 
> Tuan-Anh Hoang-Vu
> 
>

Re: Review Request 49990: Added cmake scripts to build slave component libraries.

[Joseph Wu]

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


Ship it!




LGTM!

Verified the build on OSX and Windows.

- Joseph Wu


On July 13, 2016, 7:32 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49990/
> ---
> 
> (Updated July 13, 2016, 7:32 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added cmake scripts to build slave component libraries.
> 
> 
> Diffs
> -
> 
>   src/slave/CMakeLists.txt d31440cb5e784d3e4f1236ac45001e80384f36f6 
>   src/slave/cmake/SlaveConfigure.cmake 
> ca4575653e00e8f868160976344ac1d57b5f7d27 
>   src/slave/qos_controllers/CMakeLists.txt PRE-CREATION 
>   src/slave/resource_estimators/CMakeLists.txt PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49990/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make (builds the dynamic libraries)
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49178: Configured single output binary folder.

[Joseph Wu]

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


Fix it, then Ship it!




As this is a small change, I'll make the changes we agreed upon before 
committing.


src/CMakeLists.txt (line 460)


Nit: not all caps + period at the end.



src/CMakeLists.txt (lines 461 - 463)


This would be nice to have on non-Windows builds too.  I'll add the 
`Debug`/`Release` flattening for Windows-only though.


- Joseph Wu


On June 23, 2016, 9:04 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49178/
> ---
> 
> (Updated June 23, 2016, 9:04 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Configured single output binary folder.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ecdeee75abfca944bc2ac1da4962d8d50e236d2d 
> 
> Diff: https://reviews.apache.org/r/49178/diff/
> 
> 
> Testing
> ---
> 
> Windows: build/run
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 50080: Updated Windows build instructions.

[Joseph Wu]

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

(Updated July 15, 2016, 12:29 p.m.)


Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, and 
Vinod Kone.


Changes
---

One more note about spaces in the build path.  Those are not allowed, because 
we don't escape everything :(
And you generally don't have spaces in Unix paths.


Repository: mesos


Description (updated)
---

Adds notes about the type of Visual Studio installation expected (C++),
the git checkout configuration, and the build path on Windows.


Diffs (updated)
-

  docs/getting-started.md 104816ed4be06ab6be96de9d69cf2b01d60199d0 

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


Testing
---

Visually confirmed (via website preview) that the markdown is sane.


Thanks,

Joseph Wu

Re: Review Request 50065: Enhancement for containers which have image and join host network.

[Jie Yu]

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


Fix it, then Ship it!




LGTM! Thanks for fixing it and being detail oriented in this patch! Really 
appreciated!

I'll fix the issue and commit for you. Thanks!

Please follow up with a test (container uses rootfs).


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


add a space after `rootDir`



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


I would:
```
3. The container joined the host network (both w/ or w/o container rootfs).
```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 624 - 631)


I would move this up and add a NOTE here:
```

if (containerNetworks.empty()) {
  // This is for the container which has an image and wants to join host
  // network, we will make sure it has access to host /etc/* files.
  if (containerConfig.has_rootfs()) {
...
  }
  
  // NOTE: No additional namespace needed. If container has a rootfs,
  // the filesystem/linux isolator will put the container in a new
  // mount namespace.
  return None();
} else {
  ...
  
  return launchInfo;
}
```



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


I'd be safe here to do:
```
if (infos[containerId]->containerNetworks.empty() &&
infos[containerId]->rootfs.isSome()) {
  ...
}
```


- Jie Yu


On July 15, 2016, 6:21 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50065/
> ---
> 
> (Updated July 15, 2016, 6:21 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-5806
> https://issues.apache.org/jira/browse/MESOS-5806
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For the containers which have image and join host network, we enhanced
> 'network/cni' isolator to make sure they have access to host /etc/hosts
> , /etc/hostname and /etc/resolv.conf files.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 09890cedf2e7a1846bd1cb250e117be1680a1b80 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 92b33111799cb4e1c8bc2051381e1254d701d95c 
> 
> Diff: https://reviews.apache.org/r/50065/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 1. Start Mesos master
> ```
> sudo ./bin/mesos-master.sh --work_dir=/opt/mesos
> ```
> 
> 2. Start Mesos agent
> ```
> sudo ./bin/mesos-slave.sh --master=192.168.122.171:5050 
> --containerizers=mesos --image_providers=appc,docker 
> --isolation=filesystem/linux,docker/runtime,network/cni,cgroups/cpu,cgroups/mem
>  --network_cni_config_dir=/opt/cni/net_configs 
> --network_cni_plugins_dir=/opt/cni/plugins --work_dir=/opt/mesos
> ```
> 
> 3. Launch a container which has image and joins host network
> ```
> sudo src/mesos-execute --master=192.168.122.171:5050 --name=test 
> --docker_image=library/busybox --command="sleep 120"
> ```
> 
> 4. Check if the container has access to host network files
> ```
> # 26927 is the PID of the container
> 
> $ sudo nsenter -t 26927 -m -n -u ls -la /etc/
> total 36
> drwxr-xr-x2 root root  4096 Jul 15 05:52 .
> drwxr-xr-x   13 root root  4096 Jul 15 05:52 ..
> -rw-rw-r--1 root root   304 Dec  5  2015 group
> -rw-r--r--1 root root 6 Aug 10  2015 hostname
> -rw-r--r--1 root root   272 Apr 18 08:37 hosts
> -rw-r--r--1 root root   118 Jun  8 16:29 localtime
> -rw-rw-r--1 root root   334 Dec  5  2015 passwd
> -rw-r--r--1 root root   176 Jul  9 14:44 resolv.conf
> -rw-rw-r--1 root root   243 Dec  5  2015 shadow
> 
> $ sudo nsenter -t 26927 -m -n -u mount 
> rootfs on / type rootfs (rw)
> /dev/mapper/u1404u1--vg-root on / type ext4 
> (rw,relatime,errors=remount-ro,data=ordered)
> tmpfs on /etc/resolv.conf type tmpfs 
> (rw,nosuid,noexec,relatime,size=1643388k,mode=755)
> /dev/mapper/u1404u1--vg-root on /etc/hostname type ext4 
> (rw,relatime,errors=remount-ro,data=ordered)
> /dev/mapper/u1404u1--vg-root on /etc/hosts type ext4 
> (rw,relatime,errors=remount-ro,data=ordered)
> /dev/mapper/u1404u1--vg-root on /mnt/mesos/sandbox type ext4 
> (rw,relatime,errors=remount-ro,data=ordered)
> proc on /proc type proc (rw,nosuid,nodev,noexec,relatime)
> proc on /proc/sys type proc (ro,relatime)

Re: Review Request 49955: Disabled the `--registry_strict` master flag.

[Benjamin Mahler]

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



Could we clarify the description a bit? I'm having a hard time convincing 
myself that this makes sense in the world we'd like to move to.

The strict flag was not intended as a mechanims for ensuring that partitioned 
agents are never allowed back in the registry, it was a mechanism for ensuring 
that removed agents are never allowed back in the registry. There are a few 
cases where we remove agents: (a) the agent unregisters itself (b) a new agent 
registers on the same host:port (c) we can't reach the agent (possibly a 
partition). It seems that from the description the intent is to no longer 
remove in case (c), and so it's not really clear to me what this implies for 
case (a) and case (b). The implication of this patch seems to be that we will 
allow agents that are removed in cases (a) and (b) to come back?

It would help me to see how each of (a) (b) and (c) will work in the design 
you're proposing.

- Benjamin Mahler


On July 12, 2016, 5:40 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49955/
> ---
> 
> (Updated July 12, 2016, 5:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benjamin Mahler, Joris Van 
> Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-5833
> https://issues.apache.org/jira/browse/MESOS-5833
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This flag was always marked as experimental. Moreover, we plan to change
> Mesos so that partitioned agents are always allowed to reregister with
> the master; in this world, the strict registry (which is a mechanism for
> ensuring that partitioned agents are *never* allowed to reregister with
> the master) will not be useful.
> 
> The code that implements the strict registry remains (for the time
> being), as do the test cases that depend on this behavior. However,
> `mesos-master` will refuse to start if the flag has been specified.
> 
> 
> Diffs
> -
> 
>   CHANGELOG fee129c6bdfc16d1ac038a23b4b1b097203a1502 
>   docs/configuration.md 526308a803307da48928f2cf663dfea5deb4b3a1 
>   docs/high-availability-framework-guide.md 
> ae5617b8b5a7f82499c4860130f03b2a8c669419 
>   docs/upgrades.md 431e6b3fab1a066e8f84e2a83ce961ddfb51f647 
>   src/local/local.cpp a543aef117fea62660d55435be4d66d30f8ee860 
>   src/master/flags.cpp ca3e80bf9467328892be89718e5e0a1a05264ab8 
>   src/master/main.cpp 9775b8a1e5fe51670789805557339bd0737a02b7 
> 
> Diff: https://reviews.apache.org/r/49955/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50080: Updated Windows build instructions.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50080]

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

- Mesos ReviewBot


On July 15, 2016, 5:55 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50080/
> ---
> 
> (Updated July 15, 2016, 5:55 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds a note the type of Visual Studio installation expected (C++)
> and a note about git checkouts on Windows.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md 104816ed4be06ab6be96de9d69cf2b01d60199d0 
> 
> Diff: https://reviews.apache.org/r/50080/diff/
> 
> 
> Testing
> ---
> 
> Visually confirmed (via website preview) that the markdown is sane.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 50081: Refactored docker/docker.cpp to use path::absolute().

[Gilbert Song]

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

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


Repository: mesos


Description
---

Refactored docker/docker.cpp to use path::absolute().


Diffs
-

  src/docker/docker.cpp 515842d381ca8a91ad481f66c7be057dff2f3f28 

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


Testing
---

make check

sudo ./bin/mesos-tests.sh


Thanks,

Gilbert Song

Re: Review Request 49232: Added appcManifest to ImageInfo and ProvisionInfo.

[Gilbert Song]

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




src/slave/containerizer/mesos/provisioner/appc/store.cpp (lines 198 - 199)


Just a style nit. Could we move this `->` down? Please find examples in the 
code base.


- Gilbert Song


On July 14, 2016, 10:28 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49232/
> ---
> 
> (Updated July 14, 2016, 10:28 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added appcManifest to ImageInfo and ProvisionInfo.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e900d752b70165236a198f70c8cb24876a678e4b 
>   src/slave/containerizer/mesos/provisioner/appc/store.cpp 
> aaa0efe63e587b9e604082b52a3cb8c11545fbb9 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 48a05059969e068a0ee0d38b61be9e7104e3188d 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 6825742546a87c8148097e6e13a94592e3b6754e 
>   src/slave/containerizer/mesos/provisioner/store.hpp 
> 1d477ef13ddd24fd8badae0decaa4a2271ecc746 
> 
> Diff: https://reviews.apache.org/r/49232/diff/
> 
> 
> Testing
> ---
> 
> Make Check.
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49232: Added appcManifest to ImageInfo and ProvisionInfo.

[Gilbert Song]

> On July 15, 2016, 6:42 a.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/provisioner/appc/store.cpp, line 199
> > 
> >
> > I recalled Gilbert asked why returning `Future` here but you dropped 
> > the comments without any comment, can you please help explain?

It is because we have to do `spec::getManifest()` after we have the ImageId 
ready.


> On July 15, 2016, 6:42 a.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/provisioner/appc/store.cpp, line 200
> > 
> >
> > move this before L213.

+1.


> On July 15, 2016, 6:42 a.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/provisioner/provisioner.cpp, line 403
> > 
> >
> > 4 spaces for the indent

Thanks Guangya.

@Srini, seems like you missed this one.


- Gilbert


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


On July 14, 2016, 10:28 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49232/
> ---
> 
> (Updated July 14, 2016, 10:28 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added appcManifest to ImageInfo and ProvisionInfo.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e900d752b70165236a198f70c8cb24876a678e4b 
>   src/slave/containerizer/mesos/provisioner/appc/store.cpp 
> aaa0efe63e587b9e604082b52a3cb8c11545fbb9 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 48a05059969e068a0ee0d38b61be9e7104e3188d 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 6825742546a87c8148097e6e13a94592e3b6754e 
>   src/slave/containerizer/mesos/provisioner/store.hpp 
> 1d477ef13ddd24fd8badae0decaa4a2271ecc746 
> 
> Diff: https://reviews.apache.org/r/49232/diff/
> 
> 
> Testing
> ---
> 
> Make Check.
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49961: Supported relative container path in docker containerizer.

[Gilbert Song]

> On July 13, 2016, 12:08 a.m., Guangya Liu wrote:
> >

Thansk Guangya:)


- Gilbert


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


On July 13, 2016, 12:26 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49961/
> ---
> 
> (Updated July 13, 2016, 12:26 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-5389
> https://issues.apache.org/jira/browse/MESOS-5389
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported relative container path in docker containerizer.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp e96b4b64e205946eefd1af87832fca1a45aa8ba4 
> 
> Diff: https://reviews.apache.org/r/49961/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 49939: Updated Agent::GET_METRICS call to return metrics grouped by types.

[Tuan-Anh Hoang-Vu]

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

(Updated July 15, 2016, 11:30 a.m.)


Review request for mesos, Benjamin Mahler and Vinod Kone.


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


Repository: mesos


Description
---

Updated Agent::GET_METRICS call to return metrics grouped by types.


Diffs (updated)
-

  include/mesos/agent/agent.proto cfd117de81396bf79049b7642f1ccd1ff4fbb676 
  include/mesos/v1/agent/agent.proto 213c428d424d8e4f0cc07bd86f1ed59b60df107c 
  src/slave/http.cpp 21c7ebf7c23fd06bee7125c90576eb892b249b4d 
  src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 

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


Testing
---

Modified AgentAPITest.GetMetrics to make sure we return metrics grouped by 
types.


1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
2. Run agent: ./bin/mesos-slave.sh --master=127.0.0.1:5050 
--work_dir=/var/lib/mesos
2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
'{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5051/api/v1 | 
python -m json.tool


Thanks,

Tuan-Anh Hoang-Vu

Re: Review Request 50080: Updated Windows build instructions.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 15, 2016, 5:55 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50080/
> ---
> 
> (Updated July 15, 2016, 5:55 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds a note the type of Visual Studio installation expected (C++)
> and a note about git checkouts on Windows.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md 104816ed4be06ab6be96de9d69cf2b01d60199d0 
> 
> Diff: https://reviews.apache.org/r/50080/diff/
> 
> 
> Testing
> ---
> 
> Visually confirmed (via website preview) that the markdown is sane.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

[Tuan-Anh Hoang-Vu]

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

(Updated July 15, 2016, 11:13 a.m.)


Review request for mesos, Benjamin Mahler and Vinod Kone.


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


Repository: mesos


Description
---

Updated Master::GET_METRICS call to return metrics grouped by types.


Diffs (updated)
-

  include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
  include/mesos/v1/master/master.proto c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
  src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
  src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 

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


Testing
---

Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
types.


1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
'{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
python -m json.tool


Thanks,

Tuan-Anh Hoang-Vu

Re: Review Request 50044: Updated Sorter::sort to return a vector rather than list.

[Benjamin Mahler]

> On July 15, 2016, 12:15 a.m., Guangya Liu wrote:
> >

Thanks!


> On July 15, 2016, 12:15 a.m., Guangya Liu wrote:
> > src/master/allocator/sorter/sorter.hpp, line 132
> > 
> >
> > s/list/vector

I'll adjust to:

```
  // Returns all of the clients in the order that they should
  // be allocated to, according to this Sorter's policy.
```


- Benjamin


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


On July 14, 2016, 7:35 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50044/
> ---
> 
> (Updated July 14, 2016, 7:35 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Klaus Ma, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We now prefer to use vector rather than list in general for
> efficiency reasons, unless we need to take advantage of the
> operations that are efficient on a linked-list.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e29feebd70277c79f7c3f6fb233e7a36501cf220 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 7df4dd641b21ea0705368861bf4679fed1ef078d 
>   src/master/allocator/sorter/sorter.hpp 
> f5f0b086cb95eb2ab70b3f67e5b20814925bf702 
>   src/tests/sorter_tests.cpp bdd4355bfcd7b1fa1c22983f8e0ee6f20906917a 
> 
> Diff: https://reviews.apache.org/r/50044/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> benchmarks: with 1000 clients, sort time is reduced by 72 us
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 50044: Updated Sorter::sort to return a vector rather than list.

[Benjamin Mahler]

> On July 14, 2016, 11:49 p.m., Klaus Ma wrote:
> > src/master/allocator/sorter/drf/sorter.hpp, line 22
> > 
> >
> > This's not necessary, `sorter/sorter.hpp` had included it.
> 
> Guangya Liu wrote:
> I think the reason here is we generally don't rely on transitive includes 
> as it's harder to maintain.
> 
> Klaus Ma wrote:
> We should keep small list of header; if anyone missed removing, it'll 
> impact compile performance.
> 
> Jiang Yan Xu wrote:
> We follow the rules here: 
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes 
> so in this case I think it's fine for drf/sorter.hpp to include vector again 
> even if sorter/sorter.hpp already includes it. However if we follow the guide 
> strictly, then drf/sorter.cpp doesn't need to include again because it's own 
> header includes it. :)

We generally don't rely on transitive includes, even in the case that the 
header clearly demonstrates intent (e.g. the interface function returns 
vector), because it's easier to maintain (quickly scan a file and add includes 
for all symbols present).

Klaus, note that this is a redundant include, so either way the compilation 
units with sorter.hpp are going to have one or many vector includes, but not 
zero. Zero -> one definitely adds a significant compile-time performance 
impact, but here we're talking about N -> M where N >= 1, M > N. I don't think 
we found that redundant includes had a significant impact on compile time 
performance, but if you have found that to be the case, please show us!


- Benjamin


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


On July 14, 2016, 7:35 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50044/
> ---
> 
> (Updated July 14, 2016, 7:35 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Klaus Ma, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We now prefer to use vector rather than list in general for
> efficiency reasons, unless we need to take advantage of the
> operations that are efficient on a linked-list.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e29feebd70277c79f7c3f6fb233e7a36501cf220 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 7df4dd641b21ea0705368861bf4679fed1ef078d 
>   src/master/allocator/sorter/sorter.hpp 
> f5f0b086cb95eb2ab70b3f67e5b20814925bf702 
>   src/tests/sorter_tests.cpp bdd4355bfcd7b1fa1c22983f8e0ee6f20906917a 
> 
> Diff: https://reviews.apache.org/r/50044/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> benchmarks: with 1000 clients, sort time is reduced by 72 us
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 50080: Updated Windows build instructions.

[Joseph Wu]

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

Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, and 
Vinod Kone.


Repository: mesos


Description
---

Adds a note the type of Visual Studio installation expected (C++)
and a note about git checkouts on Windows.


Diffs
-

  docs/getting-started.md 104816ed4be06ab6be96de9d69cf2b01d60199d0 

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


Testing
---

Visually confirmed (via website preview) that the markdown is sane.


Thanks,

Joseph Wu

Re: Review Request 50044: Updated Sorter::sort to return a vector rather than list.

[Benjamin Mahler]

> On July 15, 2016, 3:35 p.m., Jiang Yan Xu wrote:
> > Modulo Gaungya's comments.
> 
> Jiang Yan Xu wrote:
> In the testing done section: 72 us reduction from how much time 
> originally? :)

The timing was from a no-op sort. Took 155us for 1000 clients with a list, 83us 
or so with a vector. I didn't really want to go into much detail since I just 
wanted to show that this doesn't make things worse, because this code dates 
from before we switched to prefer vector over list.


- Benjamin


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


On July 14, 2016, 7:35 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50044/
> ---
> 
> (Updated July 14, 2016, 7:35 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Klaus Ma, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We now prefer to use vector rather than list in general for
> efficiency reasons, unless we need to take advantage of the
> operations that are efficient on a linked-list.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e29feebd70277c79f7c3f6fb233e7a36501cf220 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 7df4dd641b21ea0705368861bf4679fed1ef078d 
>   src/master/allocator/sorter/sorter.hpp 
> f5f0b086cb95eb2ab70b3f67e5b20814925bf702 
>   src/tests/sorter_tests.cpp bdd4355bfcd7b1fa1c22983f8e0ee6f20906917a 
> 
> Diff: https://reviews.apache.org/r/50044/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> benchmarks: with 1000 clients, sort time is reduced by 72 us
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[Greg Mann]

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



Regarding the regression test: several of our tests use this "local mode" to 
test example frameworks (see 'src/tests/examples_tests.cpp'), but they all set 
the MESOS_WORK_DIR env var before running. I also noticed that we don't have 
any tests which run the 'mesos-local' binary; I created a separate ticket for 
that: https://issues.apache.org/jira/browse/MESOS-5850.

One easy thing you could do to cover the issue which led to these patches is to 
modify one of the existing example framework test scripts to _not_ set 
MESOS_WORK_DIR before running. This would just involve removing lines like 
[these](https://github.com/apache/mesos/blob/4764768fc781bb99294039cebf072748f190bfba/src/tests/test_framework_test.sh#L25-L26).


src/local/flags.hpp (line 33)


I was originally put off by the duplication of the `work_dir` flag here, 
since it makes `work_dir` the only normal Mesos master/agent flag that can be 
specified at the command line. Another option I see is to manually check the 
value of `MESOS_WORK_DIR` and then set it via `flags.work_dir = DEFAULT` if 
necessary. However, I think I prefer the method of using our flag objects to 
express the default value rather than manually checking an environment variable.

Anyway! What do you think about adding a comment here explaining why we're 
doing this for `work_dir` but not any other flag? i.e., we need to set 
`work_dir` early because it's a required flag and `load` will barf if it's not 
already set. Local mode is the one case in which we deem it acceptable to use a 
default location within `/tmp` for the `work_dir`.


- Greg Mann


On July 14, 2016, 10:11 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 14, 2016, 10:11 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp a543aef 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 48313: Consistency in persistent volumes between master and agent on failure.

[Jiang Yan Xu]

> On July 13, 2016, 4:04 a.m., Neil Conway wrote:
> > BTW, one thought: rather than writing out a new checkpoint and then 
> > deleting the target checkpoint file, what about renaming target -> current 
> > checkpoint? Rename is typically atomic (within a single filesystem), which 
> > is nice, and it would avoid the need to separately delete the target 
> > checkpoint afterward.
> 
> Anindya Sinha wrote:
> SGTM. Updating the changes.
> 
> Anindya Sinha wrote:
> @neilc, @xujyan: I discarded https://reviews.apache.org/r/48314 and 
> https://reviews.apache.org/r/48315, so can this be submitted now?

Yeah, I am doing a final pass right now.


- Jiang Yan


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


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

Re: Review Request 48313: Consistency in persistent volumes between master and agent on failure.

[Anindya Sinha]

> On July 13, 2016, 11:04 a.m., Neil Conway wrote:
> > BTW, one thought: rather than writing out a new checkpoint and then 
> > deleting the target checkpoint file, what about renaming target -> current 
> > checkpoint? Rename is typically atomic (within a single filesystem), which 
> > is nice, and it would avoid the need to separately delete the target 
> > checkpoint afterward.
> 
> Anindya Sinha wrote:
> SGTM. Updating the changes.

@neilc, @xujyan: I discarded https://reviews.apache.org/r/48314 and 
https://reviews.apache.org/r/48315, so can this be submitted now?


- Anindya


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


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

Re: Review Request 49602: Fixed log message to avoid spanning multiple lines.

[Anand Mazumdar]

> On July 15, 2016, 4:26 p.m., Greg Mann wrote:
> > Neil I agree - not useful logging information :-) Shall we just remove it 
> > instead?

+1 to killing the log message.


- Anand


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


On July 4, 2016, 6:05 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49602/
> ---
> 
> (Updated July 4, 2016, 6:05 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed log message to avoid spanning multiple lines.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> Diff: https://reviews.apache.org/r/49602/diff/
> 
> 
> Testing
> ---
> 
> Previous output:
> 
> ```
> I0704 19:52:23.530931 1970868224 resources.cpp:572] Parsing resources as JSON 
> failed: cpus:4;mem:512;disk:0
> Trying semicolon-delimited string format instead
> I0704 19:52:23.530920 528384 process.cpp:2676] Resuming 
> (1)@192.168.0.104:61317 at 2016-07-04 17:52:23.530142976+00:00
> ```
> 
> New output:
> 
> ```
> I0704 19:58:00.678043 1970868224 resources.cpp:572] Parsing resources as JSON 
> failed: 'cpus:4;mem:512;disk:0'; trying semicolon-delimited string format 
> instead
> I0704 19:58:00.678032 2138112 process.cpp:2676] Resuming 
> (1)@192.168.0.104:61374 at 2016-07-04 17:58:00.675815168+00:00
> ```
> 
> Note that I'm not convinced this is a super useful thing to be logging, even 
> at `VLOG(1)`, so I'd also be fine with just removing the log statement 
> altogether.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49604: Fixed incorrect clock time in log messages.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On July 4, 2016, 6:21 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49604/
> ---
> 
> (Updated July 4, 2016, 6:21 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previous code included a pointer value in a log message, rather
> than the clock time that the pointer points at.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/clock.cpp 3fd616e91c9b8c2ac2d14a49714d340ba5a3a5da 
> 
> Diff: https://reviews.apache.org/r/49604/diff/
> 
> 
> Testing
> ---
> 
> Previous log output:
> 
> ```
> I0704 20:15:47.270100 1970868224 clock.cpp:329] Clock paused at 0x7ff72842d310
> ```
> 
> Revised log output:
> 
> ```
> I0704 20:17:59.394853 1970868224 clock.cpp:329] Clock paused at 2016-07-04 
> 18:17:59.394846976+00:00
> ```
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50002: Allow all flags load methods to specify a prefix.

[Ammar Askar]

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

(Updated July 15, 2016, 4:45 p.m.)


Review request for mesos, Greg Mann and Vinod Kone.


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


Repository: mesos


Description
---

Allow all flags load methods to specify a prefix.

This also refactors the prefix logic into one place, so that's nice.
Required for the actual fix for passing work_dir in local.


Diffs (updated)
-

  3rdparty/stout/include/stout/flags/flags.hpp dd93627 
  3rdparty/stout/tests/flags_tests.cpp 77f3a6a 

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


Testing
---

Added additional tests to flags_tests.cpp to ensure that prefix works on the 
other methods.

`make check` passes


Thanks,

Ammar Askar

Re: Review Request 49602: Fixed log message to avoid spanning multiple lines.

[Greg Mann]

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



Neil I agree - not useful logging information :-) Shall we just remove it 
instead?

- Greg Mann


On July 4, 2016, 6:05 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49602/
> ---
> 
> (Updated July 4, 2016, 6:05 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed log message to avoid spanning multiple lines.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> Diff: https://reviews.apache.org/r/49602/diff/
> 
> 
> Testing
> ---
> 
> Previous output:
> 
> ```
> I0704 19:52:23.530931 1970868224 resources.cpp:572] Parsing resources as JSON 
> failed: cpus:4;mem:512;disk:0
> Trying semicolon-delimited string format instead
> I0704 19:52:23.530920 528384 process.cpp:2676] Resuming 
> (1)@192.168.0.104:61317 at 2016-07-04 17:52:23.530142976+00:00
> ```
> 
> New output:
> 
> ```
> I0704 19:58:00.678043 1970868224 resources.cpp:572] Parsing resources as JSON 
> failed: 'cpus:4;mem:512;disk:0'; trying semicolon-delimited string format 
> instead
> I0704 19:58:00.678032 2138112 process.cpp:2676] Resuming 
> (1)@192.168.0.104:61374 at 2016-07-04 17:58:00.675815168+00:00
> ```
> 
> Note that I'm not convinced this is a super useful thing to be logging, even 
> at `VLOG(1)`, so I'd also be fine with just removing the log statement 
> altogether.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50002: Allow all flags load methods to specify a prefix.

[Greg Mann]

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



Looks good - just some style stuff below.


3rdparty/stout/include/stout/flags/flags.hpp (line 86)


s/Load/Loads/

Since we inherit from the Google style guide, function comments should be 
descriptive rather than imperative: 
https://google.github.io/styleguide/cppguide.html#Function_Comments



3rdparty/stout/include/stout/flags/flags.hpp (line 92)


s/prefix/`prefix`/

Suggestion:
s/first/also/



3rdparty/stout/include/stout/flags/flags.hpp (line 94)


Suggestion:

s/exists in the/exists in both the/



3rdparty/stout/include/stout/flags/flags.hpp (line 103)


Missing a backtick



3rdparty/stout/include/stout/flags/flags.hpp (line 922)


s/in to/into/



3rdparty/stout/tests/flags_tests.cpp (line 533)


I think this comment could be omitted.


- Greg Mann


On July 14, 2016, 10:10 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50002/
> ---
> 
> (Updated July 14, 2016, 10:10 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow all flags load methods to specify a prefix.
> 
> This also refactors the prefix logic into one place, so that's nice.
> Required for the actual fix for passing work_dir in local.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/flags/flags.hpp dd93627 
>   3rdparty/stout/tests/flags_tests.cpp 77f3a6a 
> 
> Diff: https://reviews.apache.org/r/50002/diff/
> 
> 
> Testing
> ---
> 
> Added additional tests to flags_tests.cpp to ensure that prefix works on the 
> other methods.
> 
> `make check` passes
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 49784: Increase framework numbers to allocator benchmarks.

[Jiang Yan Xu]

> On July 11, 2016, 9:32 p.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3196
> > 
> >
> > Two issues for this patch:
> > 
> > 1) The new added framework number will introduce some cases that agent 
> > number is less than framework number and some frameworks may not able to 
> > get resources.
> > 2) This will slow down the unit test, it is better introduce this and 
> > fix https://issues.apache.org/jira/browse/MESOS-4558 together by 
> > introducing `batchsize` for benchmark test.
> 
> Alexander Rukletsov wrote:
> 1) I think this a good thing to test / benchmark.
> 2) Benchmark tests are not part of the default test suite, hence it seems 
> fine if this test takes more time to run.
> 
> Guangya Liu wrote:
> For 2), one minor comment is MESOS-4558 is tracking the time of benchmark 
> tests because want to enable them on ASF CI, so it is better to reduce the 
> time of the benchmark test.
> 
> Jiang Yan Xu wrote:
> I commented on MESOS-4558. For this patch I think this is fine because 
> speeding up benchmark tests (or just CI) isn't necessarily at odds with 
> increasing the cluster size. The motivation for the latter is that we do have 
> this many frameworks in our cluster and we'd like to test it.

About 1), I don't think in the tests we have the expectation that all 
frameworks need to be allocated resources in each allocation round? Also this 
scenario is very much real for us. :)


- Jiang Yan


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


On July 7, 2016, 9:02 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49784/
> ---
> 
> (Updated July 7, 2016, 9:02 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5780 and MESOS-5781
> https://issues.apache.org/jira/browse/MESOS-5780
> https://issues.apache.org/jira/browse/MESOS-5781
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Our clusters have very high numbers of frameworks,
>   and we would like to increase the allocator benchmark
>   parameters to reflect this.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 50044: Updated Sorter::sort to return a vector rather than list.

[Jiang Yan Xu]

> On July 15, 2016, 8:35 a.m., Jiang Yan Xu wrote:
> > Modulo Gaungya's comments.

In the testing done section: 72 us reduction from how much time originally? :)


- Jiang Yan


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


On July 14, 2016, 12:35 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50044/
> ---
> 
> (Updated July 14, 2016, 12:35 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Klaus Ma, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We now prefer to use vector rather than list in general for
> efficiency reasons, unless we need to take advantage of the
> operations that are efficient on a linked-list.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e29feebd70277c79f7c3f6fb233e7a36501cf220 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 7df4dd641b21ea0705368861bf4679fed1ef078d 
>   src/master/allocator/sorter/sorter.hpp 
> f5f0b086cb95eb2ab70b3f67e5b20814925bf702 
>   src/tests/sorter_tests.cpp bdd4355bfcd7b1fa1c22983f8e0ee6f20906917a 
> 
> Diff: https://reviews.apache.org/r/50044/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> benchmarks: with 1000 clients, sort time is reduced by 72 us
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 50044: Updated Sorter::sort to return a vector rather than list.

[Jiang Yan Xu]

> On July 14, 2016, 4:49 p.m., Klaus Ma wrote:
> > src/master/allocator/sorter/drf/sorter.hpp, line 22
> > 
> >
> > This's not necessary, `sorter/sorter.hpp` had included it.
> 
> Guangya Liu wrote:
> I think the reason here is we generally don't rely on transitive includes 
> as it's harder to maintain.
> 
> Klaus Ma wrote:
> We should keep small list of header; if anyone missed removing, it'll 
> impact compile performance.

We follow the rules here: 
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes 
so in this case I think it's fine for drf/sorter.hpp to include vector again 
even if sorter/sorter.hpp already includes it. However if we follow the guide 
strictly, then drf/sorter.cpp doesn't need to include again because it's own 
header includes it. :)


- Jiang Yan


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


On July 14, 2016, 12:35 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50044/
> ---
> 
> (Updated July 14, 2016, 12:35 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Klaus Ma, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We now prefer to use vector rather than list in general for
> efficiency reasons, unless we need to take advantage of the
> operations that are efficient on a linked-list.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e29feebd70277c79f7c3f6fb233e7a36501cf220 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 7df4dd641b21ea0705368861bf4679fed1ef078d 
>   src/master/allocator/sorter/sorter.hpp 
> f5f0b086cb95eb2ab70b3f67e5b20814925bf702 
>   src/tests/sorter_tests.cpp bdd4355bfcd7b1fa1c22983f8e0ee6f20906917a 
> 
> Diff: https://reviews.apache.org/r/50044/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> benchmarks: with 1000 clients, sort time is reduced by 72 us
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 49914: Improved the speed of 'MasterAPITest.UnreserveResources'.

[Abhishek Dasgupta]

> On July 13, 2016, 10:09 a.m., Neil Conway wrote:
> > src/tests/api_tests.cpp, line 95
> > 
> >
> > Rather than changing this for all tests implicitly, I'd prefer creating 
> > a `master::Flags` and changing the allocation interval explicitly in the 
> > tests that benefit from it.
> 
> Abhishek Dasgupta wrote:
> Ok.. I was working over this. Do you mean to say to write a 
> createMasterFlags() in tests/mesos.hpp as inline function and use that 
> instead of current virtual function? It needs a long streak of small changes 
> in many of the current tests.

Posted https://reviews.apache.org/r/50072/

Is it that what you asked for?


- Abhishek


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


On July 15, 2016, 3:33 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49914/
> ---
> 
> (Updated July 15, 2016, 3:33 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-5732
> https://issues.apache.org/jira/browse/MESOS-5732
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In this patch, speed of  'MasterAPITest.UnreserveResources'
> is increased by removing the unnecessary check for unreserved
> resources which is already verified in
> 'MasterAPITest.ReserveResources' and thus by removing the
> delay for waiting for the resource offers to be declined.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp b4a6c46cd3e0e57b4f4d22d2103d3947c592e133 
> 
> Diff: https://reviews.apache.org/r/49914/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 50044: Updated Sorter::sort to return a vector rather than list.

[Jiang Yan Xu]

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


Ship it!




Modulo Gaungya's comments.

- Jiang Yan Xu


On July 14, 2016, 12:35 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50044/
> ---
> 
> (Updated July 14, 2016, 12:35 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Klaus Ma, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We now prefer to use vector rather than list in general for
> efficiency reasons, unless we need to take advantage of the
> operations that are efficient on a linked-list.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> e29feebd70277c79f7c3f6fb233e7a36501cf220 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 7df4dd641b21ea0705368861bf4679fed1ef078d 
>   src/master/allocator/sorter/sorter.hpp 
> f5f0b086cb95eb2ab70b3f67e5b20814925bf702 
>   src/tests/sorter_tests.cpp bdd4355bfcd7b1fa1c22983f8e0ee6f20906917a 
> 
> Diff: https://reviews.apache.org/r/50044/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> benchmarks: with 1000 clients, sort time is reduced by 72 us
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 50072: Moved CreateMasterFlags() function definintion to 'tests/mesos.hpp'.

[Abhishek Dasgupta]

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

(Updated July 15, 2016, 3:33 p.m.)


Review request for mesos, Anand Mazumdar and Neil Conway.


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


Repository: mesos


Description
---

Moved CreateMasterFlags() function definintion to 'tests/mesos.hpp'.


Diffs
-

  src/tests/api_tests.cpp b4a6c46cd3e0e57b4f4d22d2103d3947c592e133 
  src/tests/dynamic_weights_tests.cpp 6aa0102d3821c1b9d09672c706d6d8850f3729c3 
  src/tests/master_allocator_tests.cpp 32e3a4638273fa1c792c3c9425e0b2ff8f6bed03 
  src/tests/master_authorization_tests.cpp 
e43b264b9f67d4cd965aee143cc42a1034ac9952 
  src/tests/master_quota_tests.cpp 639f4c4146c61c0713e2945fea4fd6ffe1f3e726 
  src/tests/master_tests.cpp 252b8f9f2740256aaf54f24efe961d49fce53932 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/mesos.cpp d073d79c5797ecb021f0294ab6586a000f3ca600 
  src/tests/oversubscription_tests.cpp 18da40e92a2717e9729c300799a4790bba379906 
  src/tests/persistent_volume_endpoints_tests.cpp 
2a22f3b0da817e650a25e5e2c951adb7462718b4 
  src/tests/rate_limiting_tests.cpp 61674155bebea493ec60b277b72fdcf8f366e26b 
  src/tests/registrar_tests.cpp ba7c66ff83f09d68fb5324a3f85d2875068a35a6 
  src/tests/reservation_endpoints_tests.cpp 
48c002d1dc371c285b9421ef5a2c57250d270fa8 
  src/tests/slave_recovery_tests.cpp 470fb26a2985f912b2b91d647cd7a27b8748c2a5 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 49914: Improved the speed of 'MasterAPITest.UnreserveResources'.

[Abhishek Dasgupta]

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

(Updated July 15, 2016, 3:33 p.m.)


Review request for mesos, Anand Mazumdar, Neil Conway, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

In this patch, speed of  'MasterAPITest.UnreserveResources'
is increased by removing the unnecessary check for unreserved
resources which is already verified in
'MasterAPITest.ReserveResources' and thus by removing the
delay for waiting for the resource offers to be declined.


Diffs (updated)
-

  src/tests/api_tests.cpp b4a6c46cd3e0e57b4f4d22d2103d3947c592e133 

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


Testing
---

On Ubuntu 16.04:
sudo make -j4 check


Thanks,

Abhishek Dasgupta

Re: Review Request 50078: Cleaned the arguments passed to health checker in docker executor.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50078]

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

- Mesos ReviewBot


On July 15, 2016, 2:56 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50078/
> ---
> 
> (Updated July 15, 2016, 2:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-5848
> https://issues.apache.org/jira/browse/MESOS-5848
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker executor wraps the health check command into `docker exec`
> and leverages mesos-health-check binary to actually perform the
> check. However prior to this patch there were unnecessary arguments
> passed to the binary, including `path` / `argv[0]` mismatch, log
> message was misleading as well.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 69511044e39bc05d7d6240264ec70b6e6f44edba 
> 
> Diff: https://reviews.apache.org/r/50078/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 50078: Cleaned the arguments passed to health checker in docker executor.

[Alexander Rukletsov]

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

(Updated July 15, 2016, 2:52 p.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Till Toenshoff.


Changes
---

Benjamin's comments.


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


Repository: mesos


Description
---

Docker executor wraps the health check command into `docker exec`
and leverages mesos-health-check binary to actually perform the
check. However prior to this patch there were unnecessary arguments
passed to the binary, including `path` / `argv[0]` mismatch, log
message was misleading as well.


Diffs (updated)
-

  src/docker/executor.cpp 69511044e39bc05d7d6240264ec70b6e6f44edba 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 50078: Cleaned the arguments passed to health checker in docker executor.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On July 15, 2016, 4:56 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50078/
> ---
> 
> (Updated July 15, 2016, 4:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-5848
> https://issues.apache.org/jira/browse/MESOS-5848
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker executor wraps the health check command into `docker exec`
> and leverages mesos-health-check binary to actually perform the
> check. However prior to this patch there were unnecessary arguments
> passed to the binary, including `path` / `argv[0]` mismatch, log
> message was misleading as well.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 69511044e39bc05d7d6240264ec70b6e6f44edba 
> 
> Diff: https://reviews.apache.org/r/50078/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 50078: Cleaned the arguments passed to health checker in docker executor.

[Alexander Rukletsov]

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

(Updated July 15, 2016, 2:56 p.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Till Toenshoff.


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


Repository: mesos


Description
---

Docker executor wraps the health check command into `docker exec`
and leverages mesos-health-check binary to actually perform the
check. However prior to this patch there were unnecessary arguments
passed to the binary, including `path` / `argv[0]` mismatch, log
message was misleading as well.


Diffs (updated)
-

  src/docker/executor.cpp 69511044e39bc05d7d6240264ec70b6e6f44edba 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 50078: Cleaned the arguments passed to health checker in docker executor.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On July 15, 2016, 2:13 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50078/
> ---
> 
> (Updated July 15, 2016, 2:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-5848
> https://issues.apache.org/jira/browse/MESOS-5848
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker executor wraps the health check command into `docker exec`
> and leverages mesos-health-check binary to actually perform the
> check. However prior to this patch there were unnecessary arguments
> passed to the binary, including `path` / `argv[0]` mismatch, log
> message was misleading as well.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 69511044e39bc05d7d6240264ec70b6e6f44edba 
> 
> Diff: https://reviews.apache.org/r/50078/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 50078: Cleaned the arguments passed to health checker in docker executor.

[Benjamin Bannier]

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




src/docker/executor.cpp (line 496)


Maybe that's just me, but I have difficulty quickly seeing the difference 
between `argvHealthCommand` and `argvHealthChecker`.

What about e.g., just `commandArgv` and `processArgv` (or even 
`s/Argv/Arguments/`)?



src/docker/executor.cpp (line 526)


Minor style nit: Since you know the number of entries you could just 
initialize the vector with an initializer list.


- Benjamin Bannier


On July 15, 2016, 4:13 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50078/
> ---
> 
> (Updated July 15, 2016, 4:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-5848
> https://issues.apache.org/jira/browse/MESOS-5848
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker executor wraps the health check command into `docker exec`
> and leverages mesos-health-check binary to actually perform the
> check. However prior to this patch there were unnecessary arguments
> passed to the binary, including `path` / `argv[0]` mismatch, log
> message was misleading as well.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 69511044e39bc05d7d6240264ec70b6e6f44edba 
> 
> Diff: https://reviews.apache.org/r/50078/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 50078: Cleaned the arguments passed to health checker in docker executor.

[Alexander Rukletsov]

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

Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Till Toenshoff.


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


Repository: mesos


Description
---

Docker executor wraps the health check command into `docker exec`
and leverages mesos-health-check binary to actually perform the
check. However prior to this patch there were unnecessary arguments
passed to the binary, including `path` / `argv[0]` mismatch, log
message was misleading as well.


Diffs
-

  src/docker/executor.cpp 69511044e39bc05d7d6240264ec70b6e6f44edba 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 50017: WIP: Validated the resources when parsing it.

[Guangya Liu]

> On 七月 14, 2016, 5:56 p.m., Benjamin Mahler wrote:
> > include/mesos/resources.hpp, lines 135-147
> > 
> >
> > This suggests a change of semantics to `Resources`.
> > 
> > Currently: `Resources` always refers to a valid collection resources. 
> > This function would never return Some(Error) unless we change the semantics.
> > 
> > If we want to remove the unnecessary validation, we can do this by 
> > making '`operator += (const Resources& that)`' avoid validating '`that`' 
> > since it's already valid. Currently it just calls '`operator += (const 
> > Resource& r)`' which is unaware of '`r`' already being valid since it comes 
> > from a '`Resources`'.
> > 
> > However, we have to keep the validation in '`operator += (const 
> > Resource& r)`', since '`r`' is just a protobuf coming from an arbitrary 
> > caller, it may be invalid and we need to validate.
> > 
> > Make sense?
> 
> Guangya Liu wrote:
> Hi Ben, does there are any example in mesos code for `Resource` object 
> come from protobuf? I found that we are always using `Resource::parse` to get 
> a `Resource` or `Resources` object and I have added some validation logic in 
> `Resource::parse` in when getting `Resource` or `Resources` object.

Ben, got your point. We can set `Resource` object as following:

```javascript 
Resource scalar;
scalar.set_name("cpus");
scalar.set_type(Value::SCALAR);
scalar.mutable_scalar()->set_value(1.234);
```

So if we igore the validation for `'operator += (const Resource& r)'`, there 
might be some erros if the resource was not constructed correctly. This is not 
a good way to create `Resource` object but we cannot guard the usage for this 
now.

I found that the API `'operator += (const Resource& r)'` was not called 
directly too frequently, but we are using this API `'operator += (const 
Resources& that)'` for the most of the time, so seems it is good to keep the 
validation for `'operator += (const Resource& r)'` as it was not called 
directly too much. 

The problem is that `'operator += (const Resources& that)'` is calling 
`'operator += (const Resource& r)'`, I did not found a good way to keep 
validation for `'operator += (const Resource& r)'` but ignore the validation 
for `'operator += (const Resources& that)'`, still checking how we can make 
this work, any comments/suggestions for this?


- Guangya


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


On 七月 14, 2016, 3:59 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50017/
> ---
> 
> (Updated 七月 14, 2016, 3:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The "validation" API was called in a huge number of times, but this
> was only needed when parsing resources and we do not need to do
> other validation when doing resources operations, such as add etc.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> Diff: https://reviews.apache.org/r/50017/diff/
> 
> 
> Testing
> ---
> 
> The `qcachegrind` result here: 
> https://docs.google.com/document/d/1oilen04e8trIOgYbj-jxAd7esTRDaySll8y2BQ6p5nU/edit?usp=sharing
> 
> After the fix, the performance of `adding` resources increased 30+% when 
> adding 5 agents to the cluster.
> 
> Before fix:
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30
> Using 5 agents and 1 clients
> Added 1 clients in 47us
> **Added 5 agents in 1.312497secs**
> Sorted 1 clients in 43us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30 (1321 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31
> Using 5 agents and 50 clients
> Added 50 clients in 948us
> **Added 5 agents in 1.325987secs**
> Sorted 50 clients in 1165us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31 (1340 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/32
> Using 5 agents and 100 clients
> Added 100 clients in 1697us
> **Added 5 agents in 1.409478secs**
> Sorted 100 clients in 2876us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/32 (1432 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/33
> Using 5 agents and 200 clients
> Added 200 clients in 4553us
> **Added 5 agents 

Re: Review Request 49232: Added appcManifest to ImageInfo and ProvisionInfo.

[Guangya Liu]

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




src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 199)


I recalled Gilbert asked why returning `Future` here but you dropped the 
comments without any comment, can you please help explain?



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 200)


move this before L213.



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 403)


4 spaces for the indent


- Guangya Liu


On 七月 14, 2016, 5:28 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49232/
> ---
> 
> (Updated 七月 14, 2016, 5:28 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added appcManifest to ImageInfo and ProvisionInfo.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e900d752b70165236a198f70c8cb24876a678e4b 
>   src/slave/containerizer/mesos/provisioner/appc/store.cpp 
> aaa0efe63e587b9e604082b52a3cb8c11545fbb9 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 48a05059969e068a0ee0d38b61be9e7104e3188d 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 6825742546a87c8148097e6e13a94592e3b6754e 
>   src/slave/containerizer/mesos/provisioner/store.hpp 
> 1d477ef13ddd24fd8badae0decaa4a2271ecc746 
> 
> Diff: https://reviews.apache.org/r/49232/diff/
> 
> 
> Testing
> ---
> 
> Make Check.
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49913: Moved createFrameworkInfo() function definition to tests/mesos.hpp.

[Abhishek Dasgupta]

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

(Updated July 15, 2016, 11:08 a.m.)


Review request for mesos, Anand Mazumdar, Jay Guo, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

There were multiple createFrameworkInfo() definition scattered
in various testcases. In this patch, only one defintion of
createFrameworkInfo() is provided in src/tests/mesos.hpp and
other tests just used this definition.


Diffs
-

  src/tests/api_tests.cpp b4a6c46cd3e0e57b4f4d22d2103d3947c592e133 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/persistent_volume_endpoints_tests.cpp 
2a22f3b0da817e650a25e5e2c951adb7462718b4 
  src/tests/reservation_endpoints_tests.cpp 
48c002d1dc371c285b9421ef5a2c57250d270fa8 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 49913: Moved createFrameworkInfo() function definition to tests/mesos.hpp.

[Abhishek Dasgupta]

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

(Updated July 15, 2016, 11:07 a.m.)


Review request for mesos, Anand Mazumdar, Jay Guo, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Moved createFrameworkInfo() function definition to tests/mesos.hpp.


Diffs (updated)
-

  src/tests/api_tests.cpp b4a6c46cd3e0e57b4f4d22d2103d3947c592e133 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/persistent_volume_endpoints_tests.cpp 
2a22f3b0da817e650a25e5e2c951adb7462718b4 
  src/tests/reservation_endpoints_tests.cpp 
48c002d1dc371c285b9421ef5a2c57250d270fa8 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 49913: Moved createFrameworkInfo() function definition to tests/mesos.hpp.

[Abhishek Dasgupta]

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

(Updated July 15, 2016, 10:47 a.m.)


Review request for mesos, Anand Mazumdar, Jay Guo, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

There were multiple createFrameworkInfo() definition scattered
in various testcases. In this patch, only one defintion of
createFrameworkInfo() is provided in src/tests/mesos.hpp and
other tests just used this definition.


Diffs (updated)
-

  src/tests/api_tests.cpp b4a6c46cd3e0e57b4f4d22d2103d3947c592e133 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/persistent_volume_endpoints_tests.cpp 
2a22f3b0da817e650a25e5e2c951adb7462718b4 
  src/tests/reservation_endpoints_tests.cpp 
48c002d1dc371c285b9421ef5a2c57250d270fa8 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 50065: Enhancement for containers which have image and join host network.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50065]

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

- Mesos ReviewBot


On July 15, 2016, 6:21 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50065/
> ---
> 
> (Updated July 15, 2016, 6:21 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-5806
> https://issues.apache.org/jira/browse/MESOS-5806
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For the containers which have image and join host network, we enhanced
> 'network/cni' isolator to make sure they have access to host /etc/hosts
> , /etc/hostname and /etc/resolv.conf files.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 09890cedf2e7a1846bd1cb250e117be1680a1b80 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 92b33111799cb4e1c8bc2051381e1254d701d95c 
> 
> Diff: https://reviews.apache.org/r/50065/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 1. Start Mesos master
> ```
> sudo ./bin/mesos-master.sh --work_dir=/opt/mesos
> ```
> 
> 2. Start Mesos agent
> ```
> sudo ./bin/mesos-slave.sh --master=192.168.122.171:5050 
> --containerizers=mesos --image_providers=appc,docker 
> --isolation=filesystem/linux,docker/runtime,network/cni,cgroups/cpu,cgroups/mem
>  --network_cni_config_dir=/opt/cni/net_configs 
> --network_cni_plugins_dir=/opt/cni/plugins --work_dir=/opt/mesos
> ```
> 
> 3. Launch a container which has image and joins host network
> ```
> sudo src/mesos-execute --master=192.168.122.171:5050 --name=test 
> --docker_image=library/busybox --command="sleep 120"
> ```
> 
> 4. Check if the container has access to host network files
> ```
> # 26927 is the PID of the container
> 
> $ sudo nsenter -t 26927 -m -n -u ls -la /etc/
> total 36
> drwxr-xr-x2 root root  4096 Jul 15 05:52 .
> drwxr-xr-x   13 root root  4096 Jul 15 05:52 ..
> -rw-rw-r--1 root root   304 Dec  5  2015 group
> -rw-r--r--1 root root 6 Aug 10  2015 hostname
> -rw-r--r--1 root root   272 Apr 18 08:37 hosts
> -rw-r--r--1 root root   118 Jun  8 16:29 localtime
> -rw-rw-r--1 root root   334 Dec  5  2015 passwd
> -rw-r--r--1 root root   176 Jul  9 14:44 resolv.conf
> -rw-rw-r--1 root root   243 Dec  5  2015 shadow
> 
> $ sudo nsenter -t 26927 -m -n -u mount 
> rootfs on / type rootfs (rw)
> /dev/mapper/u1404u1--vg-root on / type ext4 
> (rw,relatime,errors=remount-ro,data=ordered)
> tmpfs on /etc/resolv.conf type tmpfs 
> (rw,nosuid,noexec,relatime,size=1643388k,mode=755)
> /dev/mapper/u1404u1--vg-root on /etc/hostname type ext4 
> (rw,relatime,errors=remount-ro,data=ordered)
> /dev/mapper/u1404u1--vg-root on /etc/hosts type ext4 
> (rw,relatime,errors=remount-ro,data=ordered)
> /dev/mapper/u1404u1--vg-root on /mnt/mesos/sandbox type ext4 
> (rw,relatime,errors=remount-ro,data=ordered)
> proc on /proc type proc (rw,nosuid,nodev,noexec,relatime)
> proc on /proc/sys type proc (ro,relatime)
> sysfs on /sys type sysfs (ro,nosuid,nodev,noexec,relatime)
> tmpfs on /dev type tmpfs (rw,nosuid,mode=755)
> devpts on /dev/pts type devpts 
> (rw,nosuid,noexec,relatime,mode=600,ptmxmode=666)
> tmpfs on /dev/shm type tmpfs (rw,nosuid,nodev)
> ```
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 50017: WIP: Validated the resources when parsing it.

[Guangya Liu]

> On 七月 14, 2016, 5:56 p.m., Benjamin Mahler wrote:
> > include/mesos/resources.hpp, lines 135-147
> > 
> >
> > This suggests a change of semantics to `Resources`.
> > 
> > Currently: `Resources` always refers to a valid collection resources. 
> > This function would never return Some(Error) unless we change the semantics.
> > 
> > If we want to remove the unnecessary validation, we can do this by 
> > making '`operator += (const Resources& that)`' avoid validating '`that`' 
> > since it's already valid. Currently it just calls '`operator += (const 
> > Resource& r)`' which is unaware of '`r`' already being valid since it comes 
> > from a '`Resources`'.
> > 
> > However, we have to keep the validation in '`operator += (const 
> > Resource& r)`', since '`r`' is just a protobuf coming from an arbitrary 
> > caller, it may be invalid and we need to validate.
> > 
> > Make sense?

Hi Ben, does there are any example in mesos code for `Resource` object come 
from protobuf? I found that we are always using `Resource::parse` to get a 
`Resource` or `Resources` object and I have added some validation logic in 
`Resource::parse` in when getting `Resource` or `Resources` object.


- Guangya


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


On 七月 14, 2016, 3:59 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50017/
> ---
> 
> (Updated 七月 14, 2016, 3:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The "validation" API was called in a huge number of times, but this
> was only needed when parsing resources and we do not need to do
> other validation when doing resources operations, such as add etc.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> Diff: https://reviews.apache.org/r/50017/diff/
> 
> 
> Testing
> ---
> 
> The `qcachegrind` result here: 
> https://docs.google.com/document/d/1oilen04e8trIOgYbj-jxAd7esTRDaySll8y2BQ6p5nU/edit?usp=sharing
> 
> After the fix, the performance of `adding` resources increased 30+% when 
> adding 5 agents to the cluster.
> 
> Before fix:
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30
> Using 5 agents and 1 clients
> Added 1 clients in 47us
> **Added 5 agents in 1.312497secs**
> Sorted 1 clients in 43us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30 (1321 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31
> Using 5 agents and 50 clients
> Added 50 clients in 948us
> **Added 5 agents in 1.325987secs**
> Sorted 50 clients in 1165us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31 (1340 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/32
> Using 5 agents and 100 clients
> Added 100 clients in 1697us
> **Added 5 agents in 1.409478secs**
> Sorted 100 clients in 2876us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/32 (1432 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/33
> Using 5 agents and 200 clients
> Added 200 clients in 4553us
> **Added 5 agents in 1.371473secs**
> Sorted 200 clients in 5371us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/33 (1412 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/34
> Using 5 agents and 500 clients
> Added 500 clients in 8836us
> **Added 5 agents in 1.304245secs**
> Sorted 500 clients in 14697us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/34 (1387 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35
> Using 5 agents and 1000 clients
> Added 1000 clients in 19508us
> **Added 5 agents in 1.270555secs**
> Sorted 1000 clients in 32575us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 (1433 ms)
> 
> 
> After the fix:
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30
> Using 5 agents and 1 clients
> Added 1 clients in 42us
> **Added 5 agents in 891266us**
> Sorted 1 clients in 59us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/30 (902 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/31
> Using 5 agents and 50 clients
> Added 50 clients in 933us
> 

Re: Review Request 49855: Enabled cgroups unified isolator in isolation.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50038, 49814, 49817, 49819, 49820, 49821, 49823, 49824, 
49825, 49827, 49828, 49849, 49850, 49851, 49852, 45573, 49853, 49854, 49855]

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

- Mesos ReviewBot


On July 15, 2016, 3:33 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49855/
> ---
> 
> (Updated July 15, 2016, 3:33 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5652
> https://issues.apache.org/jira/browse/MESOS-5652
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled cgroups unified isolator in isolation.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e900d752b70165236a198f70c8cb24876a678e4b 
> 
> Diff: https://reviews.apache.org/r/49855/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49843: Added benchmark test for sorter.

[Guangya Liu]

> On 七月 14, 2016, 7:14 p.m., Benjamin Mahler wrote:
> > src/tests/sorter_tests.cpp, lines 504-542
> > 
> >
> > It looks like we can simplify these two functions down to the following 
> > single function?
> > 
> > ```
> > // Returns a "ports" resource with the number of ranges
> > // specified as: [1-2, 4-5, 7-8, 10-11, ...]
> > static Resource makePortRanges(size_t numRanges)
> > {
> >   ::mesos::Value::Ranges ranges;
> > 
> >   for (size_t i = 0; i < numRanges; ++i) {
> > Value::Range* range = ranges.add_range();
> > range->set_begin((3 * i) + 1);
> > range->set_end(range->begin() + 1);
> >   }
> > 
> >   Value value;
> >   value.set_type(Value::RANGES);
> >   value.mutable_ranges()->CopyFrom(ranges);
> > 
> >   Resource resource;
> >   resource.set_role("*");
> >   resource.set_name("ports");
> >   resource.set_type(Value::RANGES);
> >   resource.mutable_ranges()->CopyFrom(value.ranges());
> > 
> >   return resource;
> > }
> > ```
> > 
> > Since we only care about the number of ranges, we don't need to specify 
> > the bounds, right?
> 
> Guangya Liu wrote:
> Yes, my orignial thinking was keep this same as 
> https://github.com/apache/mesos/blob/master/src/tests/hierarchical_allocator_tests.cpp#L85-L122
>  in case we may need specify bounds in future, but since this is only for 
> test and we do not need to care for bounds, so seems no need to have 
> `makeRange` here. I was thinking we may also need to update 
> https://github.com/apache/mesos/blob/master/src/tests/hierarchical_allocator_tests.cpp#L85-L122
>  as well to use a single function to create port ranges.

Hi Ben, there is one issue for current function: We are now using port range as 
`[1-2, 4-5, 7-8, 10-11, ...]`, but the slave created using port `[31000-32000]` 
in here 
https://github.com/apache/mesos/blob/master/src/tests/sorter_tests.cpp#L565-L566
 . 

Basically, clients cannot get port resources which are not in `[31000-32000]` 
but here we are using port range `[1-2, 4-5, 7-8, 10-11, ...]`, this will not 
impact the `sorter` result as the port resources are excluded from sort 
https://github.com/apache/mesos/blob/master/src/master/allocator/sorter/drf/sorter.cpp#L288
 .

But I think we still need to fix this to make sure the logic tecnically right 
here, there are two options: 1) Keep the orignial code and have two functions 
by using `makeRange` to define the bounds. 2) Hard code the resources in slave 
and make the port range starting from 1. I think 1) is better as this can make 
the port range controllable via some parameters, and for 2), we need to make 
sure the allocated port range always fall into the agent resources port range 
when setting the agent resources.

What do you think?


- Guangya


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


On 七月 12, 2016, 9:09 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49843/
> ---
> 
> (Updated 七月 12, 2016, 9:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Bugs: MESOS-5701
> https://issues.apache.org/jira/browse/MESOS-5701
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added benchmark test for sorter.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp 20e42419934e81b97676569876cd63ee0a550da3 
> 
> Diff: https://reviews.apache.org/r/49843/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
>  
>  ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/*"
> [==] Running 36 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 36 tests from AgentAndClientCount/Sorter_BENCHMARK_Test
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/0
> Using 1000 agents and 1 clients
> Added 1 clients in 1047us
> Added 1000 agents in 30147us
> Sorted 1 clients in 87us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/0 (33 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1
> Using 1000 agents and 50 clients
> Added 50 clients in 884us
> Added 1000 agents in 30129us
> Sorted 50 clients in 1284us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1 (37 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/2
> Using 1000 agents and 100 clients
> Added 100 clients in 1676us
> Added 1000 agents in 25157us
> Sorted 100 clients in 3814us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/2 (41 

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

[Abhishek Dasgupta]

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




src/master/http.cpp (lines 1943 - 1945)


I just learned few days back that we don't use switch this way. We don't 
use `default` . Instead, we address UNKNOWN explicitly and use UNREACHABLE(). 
You may see :
Future Master::Http::listFiles(
const mesos::master::Call& call,
const Option& principal,
ContentType contentType) const
{
}
in the same file.


- Abhishek Dasgupta


On July 14, 2016, 9:35 p.m., Tuan-Anh Hoang-Vu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49938/
> ---
> 
> (Updated July 14, 2016, 9:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5731
> https://issues.apache.org/jira/browse/MESOS-5731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Master::GET_METRICS call to return metrics grouped by types.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49938/diff/
> 
> 
> Testing
> ---
> 
> Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
> types.
> 
> 
> 1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
> '{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
> python -m json.tool
> 
> 
> Thanks,
> 
> Tuan-Anh Hoang-Vu
> 
>

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

[Abhishek Dasgupta]

> On July 15, 2016, 6:55 a.m., Abhishek Dasgupta wrote:
> > src/master/http.cpp, lines 1917-1928
> > 
> >
> > You may do it this way:
> >  // TODO(tuananh): I don't know why I cannot use foreachpair instead.
> > foreachpair (const MetricType& metricType, const auto& metrics, 
> > metricsMap) {
> >   foreachpair (const string& key, double value, metrics) {
> > switch (metricType) {
> >   case MetricType::COUNTER:
> >   {
> > Metric metric;
> > metric.set_name(key);
> > metric.set_value(value);
> > _getMetrics->add_counters()->CopyFrom(metric);
> > break;
> >   }

Please, notice that I changed blocks inside switch cases a little. Can you do 
likewise for other blocks in this switch case if you address this comment of 
mine?


- Abhishek


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


On July 14, 2016, 9:35 p.m., Tuan-Anh Hoang-Vu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49938/
> ---
> 
> (Updated July 14, 2016, 9:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5731
> https://issues.apache.org/jira/browse/MESOS-5731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Master::GET_METRICS call to return metrics grouped by types.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49938/diff/
> 
> 
> Testing
> ---
> 
> Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
> types.
> 
> 
> 1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
> '{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
> python -m json.tool
> 
> 
> Thanks,
> 
> Tuan-Anh Hoang-Vu
> 
>

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

[Abhishek Dasgupta]

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




src/master/http.cpp (lines 1917 - 1928)


You may do it this way:
 // TODO(tuananh): I don't know why I cannot use foreachpair instead.
foreachpair (const MetricType& metricType, const auto& metrics, 
metricsMap) {
  foreachpair (const string& key, double value, metrics) {
switch (metricType) {
  case MetricType::COUNTER:
  {
Metric metric;
metric.set_name(key);
metric.set_value(value);
_getMetrics->add_counters()->CopyFrom(metric);
break;
  }


- Abhishek Dasgupta


On July 14, 2016, 9:35 p.m., Tuan-Anh Hoang-Vu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49938/
> ---
> 
> (Updated July 14, 2016, 9:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5731
> https://issues.apache.org/jira/browse/MESOS-5731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Master::GET_METRICS call to return metrics grouped by types.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49938/diff/
> 
> 
> Testing
> ---
> 
> Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
> types.
> 
> 
> 1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
> '{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
> python -m json.tool
> 
> 
> Thanks,
> 
> Tuan-Anh Hoang-Vu
> 
>

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

[Abhishek Dasgupta]

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




src/tests/api_tests.cpp (line 335)


Can you be specific here? eg., // Verifies that the response for `COUNTER` 
type metrics is not empty. Likewise, in the following ASSERT_LE tests.


- Abhishek Dasgupta


On July 14, 2016, 9:35 p.m., Tuan-Anh Hoang-Vu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49938/
> ---
> 
> (Updated July 14, 2016, 9:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5731
> https://issues.apache.org/jira/browse/MESOS-5731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Master::GET_METRICS call to return metrics grouped by types.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49938/diff/
> 
> 
> Testing
> ---
> 
> Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
> types.
> 
> 
> 1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
> '{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
> python -m json.tool
> 
> 
> Thanks,
> 
> Tuan-Anh Hoang-Vu
> 
>

Re: Review Request 49943: Made vector reserve some spaces for allocator benchmark test.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On July 14, 2016, 8:14 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49943/
> ---
> 
> (Updated July 14, 2016, 8:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave to
> call vector reserve to reserve soem spaces before using, this can
> help improve the performance of the test.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 153c9b4cf4819e976910c5a7ad9602028e2d22eb 
> 
> Diff: https://reviews.apache.org/r/49943/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/*"
> [==] Running 36 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 36 tests from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0
> Using 1000 agents and 1 frameworks
> Added 1 frameworks in 102us
> Added 1000 agents in 1.110443secs
> Updated 1000 agents in 583296us
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0
>  (1993 ms)
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/1
> Using 1000 agents and 50 frameworks
> Added 50 frameworks in 1826us
> Added 1000 agents in 2.577197secs
> Updated 1000 agents in 2.412386secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/1
>  (5265 ms)
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/2
> Using 1000 agents and 100 frameworks
> Added 100 frameworks in 4260us
> Added 1000 agents in 4.275021secs
> Updated 1000 agents in 3.902358secs
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/2
>  (8494 ms)
> ...
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 50065: Enhancement for containers which have image and join host network.

[Qian Zhang]

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

Review request for mesos, Avinash sridharan and Jie Yu.


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


Repository: mesos


Description
---

For the containers which have image and join host network, we enhanced
'network/cni' isolator to make sure they have access to host /etc/hosts
, /etc/hostname and /etc/resolv.conf files.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
09890cedf2e7a1846bd1cb250e117be1680a1b80 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
92b33111799cb4e1c8bc2051381e1254d701d95c 

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


Testing
---

make check

1. Start Mesos master
```
sudo ./bin/mesos-master.sh --work_dir=/opt/mesos
```

2. Start Mesos agent
```
sudo ./bin/mesos-slave.sh --master=192.168.122.171:5050 --containerizers=mesos 
--image_providers=appc,docker 
--isolation=filesystem/linux,docker/runtime,network/cni,cgroups/cpu,cgroups/mem 
--network_cni_config_dir=/opt/cni/net_configs 
--network_cni_plugins_dir=/opt/cni/plugins --work_dir=/opt/mesos
```

3. Launch a container which has image and joins host network
```
sudo src/mesos-execute --master=192.168.122.171:5050 --name=test 
--docker_image=library/busybox --command="sleep 120"
```

4. Check if the container has access to host network files
```
# 26927 is the PID of the container

$ sudo nsenter -t 26927 -m -n -u ls -la /etc/
total 36
drwxr-xr-x2 root root  4096 Jul 15 05:52 .
drwxr-xr-x   13 root root  4096 Jul 15 05:52 ..
-rw-rw-r--1 root root   304 Dec  5  2015 group
-rw-r--r--1 root root 6 Aug 10  2015 hostname
-rw-r--r--1 root root   272 Apr 18 08:37 hosts
-rw-r--r--1 root root   118 Jun  8 16:29 localtime
-rw-rw-r--1 root root   334 Dec  5  2015 passwd
-rw-r--r--1 root root   176 Jul  9 14:44 resolv.conf
-rw-rw-r--1 root root   243 Dec  5  2015 shadow

$ sudo nsenter -t 26927 -m -n -u mount 
rootfs on / type rootfs (rw)
/dev/mapper/u1404u1--vg-root on / type ext4 
(rw,relatime,errors=remount-ro,data=ordered)
tmpfs on /etc/resolv.conf type tmpfs 
(rw,nosuid,noexec,relatime,size=1643388k,mode=755)
/dev/mapper/u1404u1--vg-root on /etc/hostname type ext4 
(rw,relatime,errors=remount-ro,data=ordered)
/dev/mapper/u1404u1--vg-root on /etc/hosts type ext4 
(rw,relatime,errors=remount-ro,data=ordered)
/dev/mapper/u1404u1--vg-root on /mnt/mesos/sandbox type ext4 
(rw,relatime,errors=remount-ro,data=ordered)
proc on /proc type proc (rw,nosuid,nodev,noexec,relatime)
proc on /proc/sys type proc (ro,relatime)
sysfs on /sys type sysfs (ro,nosuid,nodev,noexec,relatime)
tmpfs on /dev type tmpfs (rw,nosuid,mode=755)
devpts on /dev/pts type devpts (rw,nosuid,noexec,relatime,mode=600,ptmxmode=666)
tmpfs on /dev/shm type tmpfs (rw,nosuid,nodev)
```


Thanks,

Qian Zhang