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 20, 2016, 5:10 a.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 

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.

[Greg Mann]

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


Fix it, then Ship it!





src/local/flags.hpp (lines 33 - 34)


s/work_dir/`work_dir`/


- Greg Mann


On July 18, 2016, 10:51 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated July 18, 2016, 10:51 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 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/#review142901
---


Ship it!




Ship It!

- Greg Mann


On July 15, 2016, 4:45 p.m., Ammar Askar wrote:
> 
> ---
> 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
> -
> 
>   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 50017: Do not validate resource when add/subtract `Resources` object.

[Guangya Liu]

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

(Updated 七月 20, 2016, 3:22 a.m.)


Review request for mesos, Benjamin Mahler and Klaus Ma.


Summary (updated)
-

Do not validate resource when add/subtract `Resources` object.


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


Repository: mesos


Description (updated)
---

Do not validate resource when add/subtract `Resources` object.


Diffs (updated)
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
  src/common/resources.cpp b1bd2784aefdebf91892638b40b215373b998574 
  src/v1/resources.cpp 6d4ec75fbb7edab013563142aaf13d5302cc50af 

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


Testing (updated)
---

The `qcachegrind` result here: 
https://docs.google.com/document/d/1oilen04e8trIOgYbj-jxAd7esTRDaySll8y2BQ6p5nU/edit?usp=sharing

Based on the test, the performance of resources `+=` and `-=` will be improved 
by 10x for sorter test, and the performance for port range `+=` was improved by 
5x and port range `-=` was improved 1000x.

Sorter Benchmark test before fix:
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35
Using 5 agents and 1000 clients
Added 1000 clients in 23305us
Added 5 agents in 1.174069secs
Added allocations for 5 agents in 40.562802secs
Full sort of 1000 clients took 38193us
No-op sort of 1000 clients took 382us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 (43032 ms)
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test (43032 ms 
total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (43054 ms total)
[  PASSED  ] 1 test.
```

Sorter Benchmark test after fix:
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35
Using 5 agents and 1000 clients
Added 1000 clients in 25846us
Added 5 agents in 1.092462secs
Added allocations for 5 agents in 4.397859secs
Full sort of 1000 clients took 35051us
No-op sort of 1000 clients took 551us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 (6897 ms)
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test (6897 ms 
total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (6920 ms total)
[  PASSED  ] 1 test.
```

Ports resources benchmark test before fix:
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
Took 12.478841secs to perform 1000 'total += r' operations on ports(*):[1-2, 
4-5, 7-8, 10-11, 13-14, 16-17, 1...
Took 8.512399secs to perform 1000 'total -= r' operations on ports(*):[1-2, 
4-5, 7-8, 10-11, 13-14, 16-17, 1...
Took 11.296542secs to perform 1000 'total = total + r' operations on 
ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
Took 8.517692secs to perform 1000 'total = total - r' operations on 
ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (40808 ms)
[--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test (40808 ms 
total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (40832 ms total)
[  PASSED  ] 1 test.
```

Ports resources benchmark test after fix:
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
Took 2.827012secs to perform 1000 'total += r' operations on ports(*):[1-2, 
4-5, 7-8, 10-11, 13-14, 16-17, 1...
Took 8841us to perform 1000 'total -= r' operations on ports(*):[1-2, 4-5, 7-8, 
10-11, 13-14, 16-17, 1...
Took 3.313112secs to perform 1000 'total = total + r' operations on 
ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
Took 12415us to perform 1000 'total = total - r' operations on ports(*):[1-2, 
4-5, 7-8, 10-11, 13-14, 16-17, 1...
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (6164 ms)
[--] 1 test from ResourcesOperators/Resources_BENCHMARK_Test (6164 ms 
total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (6187 ms total)
[  PASSED  ] 1 test.
```


Thanks,

Guangya Liu

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Klaus Ma]

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




include/mesos/resources.hpp (line 84)


I think this's dangous, the developer need to check this converting and 
decide which `operator` is used. If miss used, it's hard to debuging.



include/mesos/resources.hpp (line 124)


Seems `Option` is not necessary.



include/mesos/resources.hpp (line 479)


Why not `list`? It may avoid un-necessary resizing.



src/common/resources.cpp (lines 308 - 316)


I think we can check name,type, role firstly, then check SharedInfo.



src/common/resources.cpp (line 882)


one `isShared()` is enough. `isShared` is checked in `subtractable`.


- Klaus Ma


On July 20, 2016, 6:51 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 20, 2016, 6:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-4892
> https://issues.apache.org/jira/browse/MESOS-4892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A new class Resource_ is added that allows 'Resources' to group
> identical shared resource objects together into a single 'Resource_'
> object and tracked by its shared count. Non-shared resource objects
> are not grouped.
> 
> For resource addition and subtraction, the shared count is adjusted for
> shared resources as follows:
> a) Addition: If shared resource is absent from original, then the
>resource is added initialized with a consumer count of 1. Otherwise,
>the share count for the shared resource is incremented.
> b) Subtraction: If shared resource's share count is already 1, then
>the shared resource is removed from the original. Otherwise, its
>consumer count is decremented.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   src/common/resources.cpp b1bd2784aefdebf91892638b40b215373b998574 
>   src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
>   src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
>   src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 
> 
> Diff: https://reviews.apache.org/r/45959/diff/
> 
> 
> Testing
> ---
> 
> New tests added to demonstrate arithmetic operations for shared resources 
> with consumer counts.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 50208: Fixed a file descriptor leak bug while reading file.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On July 20, 2016, 1:45 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50208/
> ---
> 
> (Updated July 20, 2016, 1:45 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: mesos-5867
> https://issues.apache.org/jira/browse/mesos-5867
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There are two early returns without closing the file
> when using read_file operator API. The two returns are:
> 
> - when `length=0`
> - when finishes reading file
> 
> 
> Diffs
> -
> 
>   src/files/files.cpp 8ab8f8aad9cfe1e536b29e8994fcd8599627e590 
> 
> Diff: https://reviews.apache.org/r/50208/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 50200: Made the agent fetch files as the task user.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50199, 50200]

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 19, 2016, 11:10 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50200/
> ---
> 
> (Updated July 19, 2016, 11:10 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: mesos-5845
> https://issues.apache.org/jira/browse/mesos-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To ensure that a task cannot fetch root-protected
> files from the local filesystem when running as a
> non-root user, this patch changes the fetcher to
> fetch files as the task user.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
>   src/tests/fetcher_tests.cpp d38ce6e750dc828ef5af4a27fac76327cc4cb56c 
> 
> Diff: https://reviews.apache.org/r/50200/diff/
> 
> 
> Testing
> ---
> 
> A new test was added to the fetcher tests: 
> `FetcherTest.ROOT_RootProtectedFileURI`.
> 
> `sudo make check` was used to test on both OSX and CentOS 7.
> 
> Note that two of the fetcher tests fail for me when run as root on OSX. I saw 
> the following on my OSX 10.10.5 system:
> ```
> [  FAILED  ] FetcherCacheTest.LocalUncachedExtract
> [  FAILED  ] FetcherCacheHttpTest.HttpMixed
> ```
> 
> These failures are already tracked here: 
> https://issues.apache.org/jira/browse/MESOS-4890
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50184: Fixed length argument bug when reading file.

[zhou xing]

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

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


Review request for mesos and Anand Mazumdar.


Changes
---

indent modifications


Bugs: mesos-5867
https://issues.apache.org/jira/browse/mesos-5867


Repository: mesos


Description
---

If length provided by client is bigger than
(size - offset), the data returned by read
file API should only contain the data actually
read from the file. Now, a length long string
will be returned which may contain blank characters.


Diffs (updated)
-

  src/files/files.cpp 8ab8f8aad9cfe1e536b29e8994fcd8599627e590 
  src/tests/api_tests.cpp d20b999f3a0d833d67c21fa267413b94277909dd 

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


Testing
---

make
make check

run mesos-master with --log_dir or --external_log_file specified. 
Open a browser and goto 127.0.0.1:5050, click 'LOG' link on the home page.
The log of master shall be shown and updated correctly.


Thanks,

zhou xing

Re: Review Request 50184: Fixed length argument bug when reading file.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On July 20, 2016, 1:45 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50184/
> ---
> 
> (Updated July 20, 2016, 1:45 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: mesos-5867
> https://issues.apache.org/jira/browse/mesos-5867
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If length provided by client is bigger than
> (size - offset), the data returned by read
> file API should only contain the data actually
> read from the file. Now, a length long string
> will be returned which may contain blank characters.
> 
> 
> Diffs
> -
> 
>   src/files/files.cpp 8ab8f8aad9cfe1e536b29e8994fcd8599627e590 
>   src/tests/api_tests.cpp d20b999f3a0d833d67c21fa267413b94277909dd 
> 
> Diff: https://reviews.apache.org/r/50184/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> run mesos-master with --log_dir or --external_log_file specified. 
> Open a browser and goto 127.0.0.1:5050, click 'LOG' link on the home page.
> The log of master shall be shown and updated correctly.
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 50208: Fixed a file descriptor leak bug while reading file.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On July 20, 2016, 1:45 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50208/
> ---
> 
> (Updated July 20, 2016, 1:45 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: mesos-5867
> https://issues.apache.org/jira/browse/mesos-5867
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There are two early returns without closing the file
> when using read_file operator API. The two returns are:
> 
> - when `length=0`
> - when finishes reading file
> 
> 
> Diffs
> -
> 
>   src/files/files.cpp 8ab8f8aad9cfe1e536b29e8994fcd8599627e590 
> 
> Diff: https://reviews.apache.org/r/50208/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 50184: Fixed length argument bug when reading file.

[zhou xing]

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

(Updated July 20, 2016, 1:45 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

update changes and associate ticket number


Bugs: mesos-5867
https://issues.apache.org/jira/browse/mesos-5867


Repository: mesos


Description
---

If length provided by client is bigger than
(size - offset), the data returned by read
file API should only contain the data actually
read from the file. Now, a length long string
will be returned which may contain blank characters.


Diffs (updated)
-

  src/files/files.cpp 8ab8f8aad9cfe1e536b29e8994fcd8599627e590 
  src/tests/api_tests.cpp d20b999f3a0d833d67c21fa267413b94277909dd 

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


Testing
---

make
make check

run mesos-master with --log_dir or --external_log_file specified. 
Open a browser and goto 127.0.0.1:5050, click 'LOG' link on the home page.
The log of master shall be shown and updated correctly.


Thanks,

zhou xing

Review Request 50208: Fixed a file descriptor leak bug while reading file.

[zhou xing]

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

Review request for mesos and Anand Mazumdar.


Bugs: mesos-5867
https://issues.apache.org/jira/browse/mesos-5867


Repository: mesos


Description
---

There are two early returns without closing the file
when using read_file operator API. The two returns are:

- when `length=0`
- when finishes reading file


Diffs
-

  src/files/files.cpp 8ab8f8aad9cfe1e536b29e8994fcd8599627e590 

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


Testing
---

make
make check


Thanks,

zhou xing

Re: Review Request 46626: Added example framework for testing disk quota enforcement.

[Artem Harutyunyan]

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

(Updated July 19, 2016, 6:23 p.m.)


Review request for Joseph Wu.


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


Repository: mesos


Description
---

Added example framework for testing disk quota enforcement.


Diffs (updated)
-

  src/Makefile.am cc83525a4455bbb0e654d346921d66ed2436411d 
  src/examples/disk_full_framework.cpp PRE-CREATION 

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


Testing
---


Thanks,

Artem Harutyunyan

Re: Review Request 50213: Made build* fields optional in VersionInfo protobuf.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On July 20, 2016, 12:49 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50213/
> ---
> 
> (Updated July 20, 2016, 12:49 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-5479 and MESOS-5480
> https://issues.apache.org/jira/browse/MESOS-5479
> https://issues.apache.org/jira/browse/MESOS-5480
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will enable us to use the VersionInfo for other components
> that do not necessarily have a build_user etc.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3f9964177c193bf8ef0455d735e0b601a6288fe3 
>   include/mesos/v1/mesos.proto cbc1d0109719f85ba27c57e10e0c6d3e01683d45 
> 
> Diff: https://reviews.apache.org/r/50213/diff/
> 
> 
> Testing
> ---
> 
> make check GTEST_FILTER="*MasterApiTest*"
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 50212: Removed GET_STATE_SUMMARY from v1 operator API.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On July 20, 2016, 12:48 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50212/
> ---
> 
> (Updated July 20, 2016, 12:48 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-5490
> https://issues.apache.org/jira/browse/MESOS-5490
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is not clear if we would need this in v1 API.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 0293e4956afb5968f84b516e3d4b5f6f858ef7a3 
>   src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
> 
> Diff: https://reviews.apache.org/r/50212/diff/
> 
> 
> Testing
> ---
> 
> make check GTEST_FILTER="*MasterApiTest*"
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 50211: Renamed GET_LEADING_MASTER to GET_MASTER in v1 operator API.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On July 20, 2016, 12:47 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50211/
> ---
> 
> (Updated July 20, 2016, 12:47 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-5497
> https://issues.apache.org/jira/browse/MESOS-5497
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed GET_LEADING_MASTER to GET_MASTER in v1 operator API.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 0293e4956afb5968f84b516e3d4b5f6f858ef7a3 
>   src/master/master.hpp bade8af69f567341667b9907368207189d0dab0e 
>   src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
>   src/tests/api_tests.cpp d20b999f3a0d833d67c21fa267413b94277909dd 
> 
> Diff: https://reviews.apache.org/r/50211/diff/
> 
> 
> Testing
> ---
> 
> make check GTEST_FILTER="*MasterApiTest*"
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 50213: Made build* fields optional in VersionInfo protobuf.

[Vinod Kone]

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

Review request for mesos and Anand Mazumdar.


Bugs: MESOS-5479 and MESOS-5480
https://issues.apache.org/jira/browse/MESOS-5479
https://issues.apache.org/jira/browse/MESOS-5480


Repository: mesos


Description
---

This will enable us to use the VersionInfo for other components
that do not necessarily have a build_user etc.


Diffs
-

  include/mesos/mesos.proto 3f9964177c193bf8ef0455d735e0b601a6288fe3 
  include/mesos/v1/mesos.proto cbc1d0109719f85ba27c57e10e0c6d3e01683d45 

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


Testing
---

make check GTEST_FILTER="*MasterApiTest*"


Thanks,

Vinod Kone

Review Request 50212: Removed GET_STATE_SUMMARY from v1 operator API.

[Vinod Kone]

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

Review request for mesos and Anand Mazumdar.


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


Repository: mesos


Description
---

It is not clear if we would need this in v1 API.


Diffs
-

  include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
  include/mesos/v1/master/master.proto c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
  src/master/http.cpp 0293e4956afb5968f84b516e3d4b5f6f858ef7a3 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 

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


Testing
---

make check GTEST_FILTER="*MasterApiTest*"


Thanks,

Vinod Kone

Review Request 50211: Renamed GET_LEADING_MASTER to GET_MASTER in v1 operator API.

[Vinod Kone]

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

Review request for mesos and Anand Mazumdar.


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


Repository: mesos


Description
---

Renamed GET_LEADING_MASTER to GET_MASTER in v1 operator API.


Diffs
-

  include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
  include/mesos/v1/master/master.proto c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
  src/master/http.cpp 0293e4956afb5968f84b516e3d4b5f6f858ef7a3 
  src/master/master.hpp bade8af69f567341667b9907368207189d0dab0e 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
  src/tests/api_tests.cpp d20b999f3a0d833d67c21fa267413b94277909dd 

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


Testing
---

make check GTEST_FILTER="*MasterApiTest*"


Thanks,

Vinod Kone

Re: Review Request 50181: [WIP] Fixed the flaky test case `MasterAPITest.GetTasks`.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 19, 2016, 7:01 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50181/
> ---
> 
> (Updated July 19, 2016, 7:01 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jay Guo, and Vinod Kone.
> 
> 
> Bugs: MESOS-5860
> https://issues.apache.org/jira/browse/MESOS-5860
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes to wait for every `StatusUpdateAcknowledgementMessage`
> exactly in this test case to make sure they don't conflict each other.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp b4bee760cbc5cc90b488554f58daf23b7482d915 
> 
> Diff: https://reviews.apache.org/r/50181/diff/
> 
> 
> Testing
> ---
> 
> This is working in progress because I have not yet reproduce it.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

[Anindya Sinha]

> On July 19, 2016, 9:45 p.m., Jiang Yan Xu wrote:
> > Commented on the resources benchmarks. Also let's pull it out then we can 
> > hopefully commit the arithmetic operations for shared resources patch first.
> > 
> > On the separate review could you post the numbers from cout instead of the 
> > total test time?
> > 
> > Overall I think this benchmark is sufficient for a first cut while we are 
> > thinking about more sophisticated tests that evaluate the performance of 
> > shared persistent volumes fairly.
> > 
> > In the new review perhaps add klaus1982 as the review as they are writing 
> > benchmarks for regular DiskInfo/Persistent Volumes (I pinged @klaus1982 on 
> > #allocator channel on slack)
> > 
> > If they haven't done so, we can add a benchmark for regular persistent 
> > volumes: add up (e.g.,) 5000 distinct persistent volumes together and see 
> > the performance. But then, what we can achieve is to compare the 
> > performance of regular persistent volumes with and without the new patch: 
> > to verify that the new patch doesn't lead to degradation of regular 
> > persistent volume arithmetic.

Moves resources benchmark to https://reviews.apache.org/r/50205/.


> On July 19, 2016, 9:45 p.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, lines 2592-2603
> > 
> >
> > Can we use `createPersistentVolume()` to create the `disk` directly?

Creating the shared persistent volume via `createDiskResource()`.


- Anindya


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


On July 19, 2016, 10:53 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49571/
> ---
> 
> (Updated July 19, 2016, 10:53 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5771
> https://issues.apache.org/jira/browse/MESOS-5771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allocations test has the following configurations:
> (1) REGULAR: Offers from every slave have regular resources.
> (2) SHARED: Offers from every slave include a shared resource.
> (3) REGULAR: Offers from every alternate slave contain only regular
> resources; and offers from every other alternate slave contains
> a shared resource.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3ddce7ab19613831527f010524b8454fecfb9927 
> 
> Diff: https://reviews.apache.org/r/49571/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> Allocations benchmark test results
> ==
> Support of shared resources has a small impact on runtime performance in 
> allocations. Also, there is no visible impact in performance when shared 
> resources are added in the tests.
> 
> With the patch (and no shared resources)
> 
> round 0 allocate took 3.19704secs to make 200 offers
> round 50 allocate took 3.240605secs to make 200 offers
> round 100 allocate took 3.227024secs to make 200 offers
> round 150 allocate took 3.225281secs to make 200 offers
> round 199 allocate took 3.26036secs to make 200 offers
> 
> With the patch (and shared resources on all agents)
> ---
> round 0 allocate took 3.279115secs to make 200 offers
> round 50 allocate took 3.273396secs to make 200 offers
> round 100 allocate took 3.278509secs to make 200 offers
> round 150 allocate took 3.275959secs to make 200 offers
> round 199 allocate took 3.278151secs to make 200 offers
> 
> With the patch (and shared resources on alternate agents)
> -
> round 0 allocate took 3.251739secs to make 200 offers
> round 50 allocate took 3.263777secs to make 200 offers
> round 100 allocate took 3.263079secs to make 200 offers
> round 150 allocate took 3.263114secs to make 200 offers
> round 199 allocate took 3.236228secs to make 200 offers
> 
> Based on HEAD, with all regular resources (no shared resources in HEAD 
> supported)
> -
> round 0 allocate took 2.925681secs to make 200 offers
> round 50 allocate took 2.922036secs to make 200 offers
> round 100 allocate took 2.909337secs to make 200 offers
> round 150 allocate took 2.914093secs to make 200 offers
> round 199 allocate took 2.923762secs to make 200 offers
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 50199: Refactored fetcher cache directory creation.

[Greg Mann]

> On July 19, 2016, 8:51 p.m., Joerg Schad wrote:
> > src/launcher/fetcher.cpp, line 381
> > 
> >
> > Does it make sense to add a check here to document and check that 
> > precondition?

Yep! :)


> On July 19, 2016, 8:51 p.m., Joerg Schad wrote:
> > src/launcher/fetcher.cpp, line 437
> > 
> >
> > Not an issue: just making sure I understood this correctly :-): if 
> > there is no user we do not need to create directory i.e., a global/non 
> > user-specific cache dir is created upfront?

While I do think this was a correct assumption (that the directory will already 
exist if no task user is specified), the `FetcherInfo` passed to the fetcher 
binary should always contain a cache directory if the cache is going to be used 
(see 
https://github.com/apache/mesos/blob/dc4a4ae9b264e66ff891c6b00181d8d501514642/src/launcher/fetcher.cpp#L368-L370),
 so I think it makes more sense to follow the original behavior found here, 
where we call `os::mkdir` whether or not a task user has been supplied.


- Greg


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


On July 19, 2016, 11:10 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 19, 2016, 11:10 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
> chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50200: Made the agent fetch files as the task user.

[Greg Mann]

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

(Updated July 19, 2016, 11:10 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Changes
---

Addressed comment.


Bugs: mesos-5845
https://issues.apache.org/jira/browse/mesos-5845


Repository: mesos


Description
---

To ensure that a task cannot fetch root-protected
files from the local filesystem when running as a
non-root user, this patch changes the fetcher to
fetch files as the task user.


Diffs (updated)
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
  src/tests/fetcher_tests.cpp d38ce6e750dc828ef5af4a27fac76327cc4cb56c 

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


Testing
---

A new test was added to the fetcher tests: 
`FetcherTest.ROOT_RootProtectedFileURI`.

`sudo make check` was used to test on both OSX and CentOS 7.

Note that two of the fetcher tests fail for me when run as root on OSX. I saw 
the following on my OSX 10.10.5 system:
```
[  FAILED  ] FetcherCacheTest.LocalUncachedExtract
[  FAILED  ] FetcherCacheHttpTest.HttpMixed
```

These failures are already tracked here: 
https://issues.apache.org/jira/browse/MESOS-4890


Thanks,

Greg Mann

Re: Review Request 50199: Refactored fetcher cache directory creation.

[Greg Mann]

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

(Updated July 19, 2016, 11:10 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

The fetcher's launcher creates the fetcher cache
directory for each user immediately before an artifact
is fetched. In order to allow this directory to be
created by a different user than the user doing the
fetching, this patch factors out this directory
creation and places it before all fetches occur.


Diffs (updated)
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 

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


Testing
---

`sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
chain.


Thanks,

Greg Mann

Re: Review Request 50072: Speed up reservation test by setting allocation interval in master flags.

[Vinod Kone]

> On July 19, 2016, 10:46 p.m., Vinod Kone wrote:
> > src/tests/api_tests.cpp, lines 95-97
> > 
> >
> > I'll clean up the coment while committing.
> 
> Anand Mazumdar wrote:
> Why do we want to change this for all the tests? As pointed out by Neil 
> here: https://reviews.apache.org/r/49914/#comment207529 we should be inlining 
> and changing this for only the tests that _need_ it.

oh missed that comment. thought it was affecting all tests in this file.


- Vinod


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


On July 19, 2016, 8:47 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50072/
> ---
> 
> (Updated July 19, 2016, 8:47 a.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
> ---
> 
> ReserveResource test (and UnreserveResource test) in v1 operator
> API is very slow and taking more than 1000 ms. If we set allocation
> interval in master flag to 50ms and use that flag to start up a
> master to use in the test, we can reduce the duration time for
> that test as low as 120 ms. So, in this patch we made that change.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp b4bee76 
> 
> Diff: https://reviews.apache.org/r/50072/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 45960: Added interfaces to handle and track shareable resources.

[Anindya Sinha]

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

(Updated July 19, 2016, 10:58 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added interfaces to handle and track shareable resources.


Diffs
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
  src/common/resources.cpp b1bd2784aefdebf91892638b40b215373b998574 
  src/v1/resources.cpp 6d4ec75fbb7edab013563142aaf13d5302cc50af 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45960: Added interfaces to handle and track shareable resources.

[Anindya Sinha]

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

(Updated July 19, 2016, 10:57 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added interfaces to handle and track shareable resources.


Diffs
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
  src/common/resources.cpp b1bd2784aefdebf91892638b40b215373b998574 
  src/v1/resources.cpp 6d4ec75fbb7edab013563142aaf13d5302cc50af 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

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

[Vinod Kone]

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



LGTM, module benm's comments.


src/master/flags.cpp (line 86)


// TODO(neilc): This flag is deprecated in 1.0 and will be removed 6 months 
later.



src/master/main.cpp (line 369)


Instead of doing the validation in two places (master and local), just do 
this in the validation lambda of the `Flags::add` method.


- Vinod Kone


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 49571: Added a benchmark test for allocations.

[Anindya Sinha]

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

(Updated July 19, 2016, 10:53 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Moved resources benchmark test to https://reviews.apache.org/r/50205/.


Summary (updated)
-

Added a benchmark test for allocations.


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


Repository: mesos


Description (updated)
---

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


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
3ddce7ab19613831527f010524b8454fecfb9927 

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


Testing (updated)
---

All tests passed.

Allocations benchmark test results
==
Support of shared resources has a small impact on runtime performance in 
allocations. Also, there is no visible impact in performance when shared 
resources are added in the tests.

With the patch (and no shared resources)

round 0 allocate took 3.19704secs to make 200 offers
round 50 allocate took 3.240605secs to make 200 offers
round 100 allocate took 3.227024secs to make 200 offers
round 150 allocate took 3.225281secs to make 200 offers
round 199 allocate took 3.26036secs to make 200 offers

With the patch (and shared resources on all agents)
---
round 0 allocate took 3.279115secs to make 200 offers
round 50 allocate took 3.273396secs to make 200 offers
round 100 allocate took 3.278509secs to make 200 offers
round 150 allocate took 3.275959secs to make 200 offers
round 199 allocate took 3.278151secs to make 200 offers

With the patch (and shared resources on alternate agents)
-
round 0 allocate took 3.251739secs to make 200 offers
round 50 allocate took 3.263777secs to make 200 offers
round 100 allocate took 3.263079secs to make 200 offers
round 150 allocate took 3.263114secs to make 200 offers
round 199 allocate took 3.236228secs to make 200 offers

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported)
-
round 0 allocate took 2.925681secs to make 200 offers
round 50 allocate took 2.922036secs to make 200 offers
round 100 allocate took 2.909337secs to make 200 offers
round 150 allocate took 2.914093secs to make 200 offers
round 199 allocate took 2.923762secs to make 200 offers


Thanks,

Anindya Sinha

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

[Anindya Sinha]

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

(Updated July 19, 2016, 10:53 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  include/mesos/mesos.proto 3f9964177c193bf8ef0455d735e0b601a6288fe3 
  include/mesos/v1/mesos.proto cbc1d0109719f85ba27c57e10e0c6d3e01683d45 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/master/allocator/mesos/hierarchical.hpp 
b72ba16277a3210e4d309b616d185a10e2029a66 
  src/master/allocator/mesos/hierarchical.cpp 
7d4064535a20b93950f5a95eef1ad3f0d37d305b 
  src/tests/hierarchical_allocator_tests.cpp 
3ddce7ab19613831527f010524b8454fecfb9927 

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


Testing
---

Tests updated with new capability.
Tests successful.


Thanks,

Anindya Sinha

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

[Anindya Sinha]

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

(Updated July 19, 2016, 10:53 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added documentation for shareable resources.


Diffs (updated)
-

  docs/home.md 5e6225ce9fd5a3fab817ec333fc5d45fb4488956 
  docs/shared-resources.md PRE-CREATION 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45960: Added interfaces to handle and track shareable resources.

[Anindya Sinha]

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

(Updated July 19, 2016, 10:52 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added interfaces to handle and track shareable resources.


Diffs (updated)
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
  src/common/resources.cpp b1bd2784aefdebf91892638b40b215373b998574 
  src/v1/resources.cpp 6d4ec75fbb7edab013563142aaf13d5302cc50af 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

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

[Anindya Sinha]

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

(Updated July 19, 2016, 10:53 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Add unit tests for sharing of resources.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
3ddce7ab19613831527f010524b8454fecfb9927 
  src/tests/master_validation_tests.cpp 
9eb82a569f7c48caa96d4d54a93b199ccac74385 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/persistent_volume_tests.cpp 
a6f97c4bb5fb29d610c01255036095e2b30c44c5 
  src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 

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


Testing
---

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


Thanks,

Anindya Sinha

Re: Review Request 45962: Added a persistent volume test framework for shared volumes.

[Anindya Sinha]

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

(Updated July 19, 2016, 10:53 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added a persistent volume test framework for shared volumes.


Diffs (updated)
-

  src/Makefile.am cc83525a4455bbb0e654d346921d66ed2436411d 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/tests/examples_tests.cpp ac513ce9aa3c8f366fe81ba937e3dc0d51a26940 
  src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 48616: Add v1 changes for shared resources.

[Anindya Sinha]

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

(Updated July 19, 2016, 10:52 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Add v1 changes for shared resources.


Diffs (updated)
-

  include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
  src/v1/resources.cpp 6d4ec75fbb7edab013563142aaf13d5302cc50af 

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


Testing
---


Thanks,

Anindya Sinha

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

[Anindya Sinha]

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

(Updated July 19, 2016, 10:52 p.m.)


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


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  src/common/resources.cpp b1bd2784aefdebf91892638b40b215373b998574 
  src/common/resources_utils.cpp 8e881a09500b0966b577e514077460f724868a8d 
  src/master/allocator/mesos/hierarchical.hpp 
b72ba16277a3210e4d309b616d185a10e2029a66 
  src/master/allocator/mesos/hierarchical.cpp 
7d4064535a20b93950f5a95eef1ad3f0d37d305b 
  src/master/allocator/sorter/drf/sorter.hpp 
bc6bfb2d5d3b32d55be055a0514861b4e7d889bb 
  src/master/allocator/sorter/drf/sorter.cpp 
ac85b327fc33d34246788e6a8c8bf5a486c61434 
  src/master/http.cpp 0293e4956afb5968f84b516e3d4b5f6f858ef7a3 
  src/master/master.hpp bade8af69f567341667b9907368207189d0dab0e 
  src/master/master.cpp 370fd8712062dc75bb81824cb99ccc7920acbf78 
  src/master/quota_handler.cpp bf6a613a7bb3c62fd77d1ffde3170749d6c21fa2 
  src/master/validation.hpp 43d876b00511d4ef652c2685d4950c994b5eef31 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
  src/master/weights_handler.cpp 9a901e71ba2fbf2ca1c02f658a72d44cfaa5ec62 
  src/tests/master_validation_tests.cpp 
9eb82a569f7c48caa96d4d54a93b199ccac74385 
  src/tests/sorter_tests.cpp b0e5ef8a55bbcca3553a221bd5691a9c801a04f7 
  src/v1/resources.cpp 6d4ec75fbb7edab013563142aaf13d5302cc50af 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

[Anindya Sinha]

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

(Updated July 19, 2016, 10:53 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Allow the task to specify the persistent volume access to be read-only
or read-write. Note that the persistent volume is always created as
read-write.
If the task is the first consumer of the shared persistent volume, then
set the ownership of the persistent volume to match that of the task.
Otherwise, allow the task to be executed only if the ownership of the
persistent volume matches that of the task.
Added an option to run the test in mixed (default) mode or shared-only
mode. In mixed mode, multiple shards alternate between shared and
unshared persistent volumes for the tasks. In shared-only mode, all
shards use shared persistent volumes for the tasks.


Diffs (updated)
-

  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/master/validation.hpp 43d876b00511d4ef652c2685d4950c994b5eef31 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
0afe9272900cfa4b39887eb259070a9a2df2ab93 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
db3ed8f69de8b52633194b252b0e5aba38ec69c0 
  src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
794b6e5990db5f8eb21a6535872f284ca02e0553 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
0809e8ec35232fbdafee171a7a960cbdec272134 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Review Request 50205: Enhanced benchmark test for resources to include shared resources.

[Anindya Sinha]

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

Review request for mesos, Klaus Ma and Jiang Yan Xu.


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


Repository: mesos


Description
---

Enhanced benchmark test for resources to include shared resources.


Diffs
-

  src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 

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


Testing
---

Tests passed. Results for resources benchmark is as follows:

Minimal impact seen in Resources arithmetic with the Resources refactor changes 
to incorporate shared resources.

With shared resources patch (note that 4th test below is for shared resources 
for scalars)

[--] 4 tests from ResourcesOperators/Resources_BENCHMARK_Test
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0 (964 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1 (17099 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (9222 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/3
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/3 (1049 ms)
[--] 4 tests from ResourcesOperators/Resources_BENCHMARK_Test (28334 ms 
total)

HEAD

[--] 3 tests from ResourcesOperators/Resources_BENCHMARK_Test
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0 (866 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1 (17563 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (9218 ms)
[--] 3 tests from ResourcesOperators/Resources_BENCHMARK_Test (27647 ms 
total)

Output from stdout is:

[==] Running 4 tests from 1 test case.
[--] Global test environment set-up.
[--] 4 tests from ResourcesOperators/Resources_BENCHMARK_Test
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0
Took 113429us to perform 5 'total += r' operations on cpus(*):1; gpus(*):1; 
mem(*):128; disk(*):256
Took 154104us to perform 5 'total -= r' operations on cpus(*):1; gpus(*):1; 
mem(*):128; disk(*):256
Took 327479us to perform 5 'total = total + r' operations on cpus(*):1; 
gpus(*):1; mem(*):128; disk(*):256
Took 365752us to perform 5 'total = total - r' operations on cpus(*):1; 
gpus(*):1; mem(*):128; disk(*):256
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0 (964 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1
Took 4.105799secs to perform 10 'total += r' operations on cpus(0, principal_0, 
{key_0: value_0}):1; gpus(...
Took 4.325702secs to perform 10 'total -= r' operations on cpus(0, principal_0, 
{key_0: value_0}):1; gpus(...
Took 4.188394secs to perform 10 'total = total + r' operations on cpus(0, 
principal_0, {key_0: value_0}):1; gpus(...
Took 4.428872secs to perform 10 'total = total - r' operations on cpus(0, 
principal_0, {key_0: value_0}):1; gpus(...
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1 (17099 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
Took 2.515553secs to perform 1000 'total += r' operations on ports(*):[1-2, 
4-5, 7-8, 10-11, 13-14, 16-17, 1...
Took 2.038598secs to perform 1000 'total -= r' operations on ports(*):[1-2, 
4-5, 7-8, 10-11, 13-14, 16-17, 1...
Took 2.62789secs to perform 1000 'total = total + r' operations on 
ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
Took 2.03868secs to perform 1000 'total = total - r' operations on 
ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1...
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (9222 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/3
Took 102441us to perform 5 'total += r' operations on cpus(*):1; 
mem(*):128; disk(test)[persistentId:...
Took 141011us to perform 5 'total -= r' operations on cpus(*):1; 
mem(*):128; disk(test)[persistentId:...
Took 383887us to perform 5 'total = total + r' operations on cpus(*):1; 
mem(*):128; disk(test)[persistentId:...
Took 421132us to perform 5 'total = total - r' operations on cpus(*):1; 
mem(*):128; disk(test)[persistentId:...
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/3 (1049 ms)
[--] 4 tests from ResourcesOperators/Resources_BENCHMARK_Test (28334 ms 
total)

[--] Global test environment tear-down
[==] 

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

> On July 19, 2016, 5:03 p.m., Jiang Yan Xu wrote:
> > It would be helpful to add a test just for contains. i.e., to test how 
> > `contains` works with nonshared resources and how it works with simple 
> > shared resources (could be constructed from createDiskResources with no 
> > arithmetic operations).
> > 
> > Also as a followup add a benchmark for resource operations. (Let's check 
> > with the IBM folks on their progress in adding DiskInfo and Reservation
> 
> Jiang Yan Xu wrote:
> Finishing my sentense above: "Diskinfo and ReservationInfo" to resources 
> benchmarks)"

Added a test `SharedResourcesTest.Contains` to verify `contains()` semantics 
with regular and shared resources.
Regarding benchmark for resources, that has already been added to 
https://reviews.apache.org/r/49571/.


> On July 19, 2016, 5:03 p.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, line 2615
> > 
> >
> > How about also throwing in a check for `count`:
> > 
> > ```
> > EXPECT_TRUE(diff.count(disk));
> > ```

I think you mean check for `contains(disk)`. Added.


> On July 19, 2016, 5:03 p.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, line 2647
> > 
> >
> > Seems like this should fail?
> > 
> > This should be true?
> > ```
> > EXPECT_EQ(r2 - r1 + r1, r2);
> > ```

```
Resources r1 = Resources::parse("cpus:2;mem:10").get() + disk + disk + disk + 
disk;
Resources r2 = Resources::parse("cpus:2;mem:10").get() + disk + disk + disk;
```

So, `EXPECT_EQ(r2 - r1 + r1, r1)` works as follows:
1) Since the `r2.count(disk) = 3` and `r1.count(disk) = 4`, so `r2 - r1` is an 
empty `Resources` object (since unshared resources are same).
2) So, when we add `r1` to that empty `Resources` object, we should get `r1`.


- Anindya


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


On July 18, 2016, 2:29 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 18, 2016, 2:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-4892
> https://issues.apache.org/jira/browse/MESOS-4892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A new class Resource_ is added that allows 'Resources' to group
> identical shared resource objects together into a single 'Resource_'
> object and tracked by its shared count. Non-shared resource objects
> are not grouped.
> 
> For resource addition and subtraction, the shared count is adjusted for
> shared resources as follows:
> a) Addition: If shared resource is absent from original, then the
>resource is added initialized with a consumer count of 1. Otherwise,
>the share count for the shared resource is incremented.
> b) Subtraction: If shared resource's share count is already 1, then
>the shared resource is removed from the original. Otherwise, its
>consumer count is decremented.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
>   src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
>   src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 
> 
> Diff: https://reviews.apache.org/r/45959/diff/
> 
> 
> Testing
> ---
> 
> New tests added to demonstrate arithmetic operations for shared resources 
> with consumer counts.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Anindya Sinha]

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

(Updated July 19, 2016, 10:51 p.m.)


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


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

A new class Resource_ is added that allows 'Resources' to group
identical shared resource objects together into a single 'Resource_'
object and tracked by its shared count. Non-shared resource objects
are not grouped.

For resource addition and subtraction, the shared count is adjusted for
shared resources as follows:
a) Addition: If shared resource is absent from original, then the
   resource is added initialized with a consumer count of 1. Otherwise,
   the share count for the shared resource is incremented.
b) Subtraction: If shared resource's share count is already 1, then
   the shared resource is removed from the original. Otherwise, its
   consumer count is decremented.

Note that v1 changes for shared resources are in the next commit.


Diffs (updated)
-

  include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
  src/common/resources.cpp b1bd2784aefdebf91892638b40b215373b998574 
  src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
  src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
  src/tests/resources_tests.cpp 4111e080b84079e100b731c9a56861b204f17388 

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


Testing
---

New tests added to demonstrate arithmetic operations for shared resources with 
consumer counts.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 50072: Speed up reservation test by setting allocation interval in master flags.

[Anand Mazumdar]

> On July 19, 2016, 10:46 p.m., Vinod Kone wrote:
> > src/tests/api_tests.cpp, lines 95-97
> > 
> >
> > I'll clean up the coment while committing.

Why do we want to change this for all the tests? As pointed out by Neil here: 
https://reviews.apache.org/r/49914/#comment207529 we should be inlining and 
changing this for only the tests that _need_ it.


- Anand


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


On July 19, 2016, 8:47 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50072/
> ---
> 
> (Updated July 19, 2016, 8:47 a.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
> ---
> 
> ReserveResource test (and UnreserveResource test) in v1 operator
> API is very slow and taking more than 1000 ms. If we set allocation
> interval in master flag to 50ms and use that flag to start up a
> master to use in the test, we can reduce the duration time for
> that test as low as 120 ms. So, in this patch we made that change.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp b4bee76 
> 
> Diff: https://reviews.apache.org/r/50072/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 50072: Speed up reservation test by setting allocation interval in master flags.

[Vinod Kone]

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


Ship it!





src/tests/api_tests.cpp (lines 95 - 97)


I'll clean up the coment while committing.


- Vinod Kone


On July 19, 2016, 8:47 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50072/
> ---
> 
> (Updated July 19, 2016, 8:47 a.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
> ---
> 
> ReserveResource test (and UnreserveResource test) in v1 operator
> API is very slow and taking more than 1000 ms. If we set allocation
> interval in master flag to 50ms and use that flag to start up a
> master to use in the test, we can reduce the duration time for
> that test as low as 120 ms. So, in this patch we made that change.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp b4bee76 
> 
> Diff: https://reviews.apache.org/r/50072/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49892: Removed unnecessary await from test.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 19, 2016, 7:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49892/
> ---
> 
> (Updated July 19, 2016, 7:48 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unnecessary await from test.
> 
> 
> Diffs
> -
> 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 3bb64995c076a8f6a09d4bacde4335c6fb8ade78 
> 
> Diff: https://reviews.apache.org/r/49892/diff/
> 
> 
> Testing
> ---
> 
> make check --gtest_repeat=100
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

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

[Jiang Yan Xu]

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



Commented on the resources benchmarks. Also let's pull it out then we can 
hopefully commit the arithmetic operations for shared resources patch first.

On the separate review could you post the numbers from cout instead of the 
total test time?

Overall I think this benchmark is sufficient for a first cut while we are 
thinking about more sophisticated tests that evaluate the performance of shared 
persistent volumes fairly.

In the new review perhaps add klaus1982 as the review as they are writing 
benchmarks for regular DiskInfo/Persistent Volumes (I pinged @klaus1982 on 
#allocator channel on slack)

If they haven't done so, we can add a benchmark for regular persistent volumes: 
add up (e.g.,) 5000 distinct persistent volumes together and see the 
performance. But then, what we can achieve is to compare the performance of 
regular persistent volumes with and without the new patch: to verify that the 
new patch doesn't lead to degradation of regular persistent volume arithmetic.


src/tests/resources_tests.cpp (lines 2592 - 2603)


Can we use `createPersistentVolume()` to create the `disk` directly?


- Jiang Yan Xu


On July 18, 2016, 7:31 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49571/
> ---
> 
> (Updated July 18, 2016, 7:31 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5771
> https://issues.apache.org/jira/browse/MESOS-5771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allocations test has the following configurations:
> (1) REGULAR: Offers from every slave have regular resources.
> (2) SHARED: Offers from every slave include a shared resource.
> (3) REGULAR: Offers from every alternate slave contain only regular
> resources; and offers from every other alternate slave contains
> a shared resource.
> 
> Added a case for testing arithmetic operations on shared resources.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> ce5da6be490b6fce311286eb4018c91eef55067e 
>   src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 
> 
> Diff: https://reviews.apache.org/r/49571/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> Resource arithmetic benchmark test results
> ==
> Minimal impact seen in Resources arithmetic with the Resources refactor 
> changes to incorporate shared resources.
> 
> With shared resources patch (note that 4th test below is for shared resources 
> for scalars)
> --
> 
> [--] 4 tests from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0 (980 ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1 (17128 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (9217 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/3
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/3 (1100 
> ms)
> [--] 4 tests from ResourcesOperators/Resources_BENCHMARK_Test (28425 
> ms total)
> 
> HEAD
> 
> 
> [--] 3 tests from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/0 (866 ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/1 (17563 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (9218 
> ms)
> [--] 3 tests from ResourcesOperators/Resources_BENCHMARK_Test (27647 
> ms total)
> 
> Allocations benchmark test results
> ==
> Support of shared resources has a small impact on runtime performance in 
> allocations. Also, there is no visible impact in performance when shared 
> resources are added in the tests.
> 
> With the patch (and no shared resources)
> 
> round 0 allocate took 3.19704secs to make 200 offers
> round 50 allocate took 3.240605secs to make 200 offers
> round 100 allocate took 

Re: Review Request 49870: Added test executables required to run tests.

[Srinivas Brahmaroutu]

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

(Updated July 19, 2016, 9:43 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 test executables required to run tests.


Diffs (updated)
-

  src/examples/CMakeLists.txt PRE-CREATION 
  src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 

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


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49863: Added Test Modules that are loaded by mesos tests.

[Srinivas Brahmaroutu]

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

(Updated July 19, 2016, 9:36 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 Test Modules that are loaded by mesos tests.


Diffs (updated)
-

  CMakeLists.txt ee8f565586b958929bc4e95b09827244d6d4155f 
  cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
  src/CMakeLists.txt bde76f7840afe55f20d7551b3f7e5fe522f7f326 
  src/examples/CMakeLists.txt PRE-CREATION 
  src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 

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


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49688: Added cmake build variables for mesos tests.

[Srinivas Brahmaroutu]

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

(Updated July 19, 2016, 9:35 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 cmake build variables for mesos tests.


Diffs (updated)
-

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

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


Testing
---

cmake ..
cmake check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49688: Added cmake build variables for mesos tests.

[Srinivas Brahmaroutu]

> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > cmake/MesosConfigure.cmake, lines 96-97
> > 
> >
> > Hmm, this seems like it should break the build. We're defining these 
> > scripts in `MesosConfigure.cmake` before we define `MESOS_TARGET`, yet 
> > `CONTAINERIZER_TEST_LIBS` lists `MESOS_TARGET` as a dependency, no? So we 
> > should fail to emit flags that link to libmesos.
> > 
> > Am I missing something obvious?

Hmm, not sure, the build worked fine even with clean builds.


> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > src/tests/CMakeLists.txt, line 21
> > 
> >
> > It looks like at least some of the following tests are missing, and I 
> > don't see them grep'ing the codebase. Rough guess: we should be including 
> > these in the relevant test subdirectories? If this is the rationale, can we 
> > please add them all now? If this is not the rationale, can we please leave 
> > a comment explaining?
> > 
> > 
> > ```
> >   tests/common/command_utils_tests.cpp  \
> >   tests/common/http_tests.cpp   \
> >   tests/common/recordio_tests.cpp   \
> >   tests/containerizer/composing_containerizer_tests.cpp \
> >   tests/containerizer/docker_containerizer_tests.cpp\
> >   tests/containerizer/docker_spec_tests.cpp \
> >   tests/containerizer/docker_tests.cpp  \
> >   tests/containerizer/external_containerizer_test.cpp   \
> >   tests/containerizer/isolator_tests.cpp\
> >   tests/containerizer/launcher.cpp  \
> >   tests/containerizer/memory_test_helper.cpp\
> >   tests/containerizer/mesos_containerizer_tests.cpp \
> >   tests/containerizer/provisioner_appc_tests.cpp\
> >   tests/containerizer/provisioner_backend_tests.cpp \
> >   tests/containerizer/provisioner_docker_tests.cpp
> > ```
> > 
> > Also, though not a test, the following file also seems to be not listed 
> > here:
> > 
> > ```
> > slave/qos_controllers/load.cpp
> > ```

I am a bit undecided to see if all the code under test be pulled into one 
CMakeLists script. I think common files should make their way here but 
containerizer also has a main routine. I need clarification how we handle 
multiple 'main' routines in a project? 
slave/qos_controllers should end up in a library right?


> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > src/tests/CMakeLists.txt, line 35
> > 
> >
> > I don't think we have a `HAS_JAVA` option, do we? Does it make sense to 
> > add an `option` in the root `CMakeLists.txt`, in the same way we have for 
> > the `VERBOSE`, `REBUNDLED`, and `ENABLE_LIBEVENT` options?
> > 
> > (If you don't know, an `option` exposes a binary choice to the CMake 
> > command line, so you can do something like `cmake .. -DENABLE_LIBEVENT=1`, 
> > which would compile Mesos with libevent.)

I think, conditional builds for Java and Python should be enabled at some point 
in time, are you planning to do so?


> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > src/tests/CMakeLists.txt, line 43
> > 
> >
> > Double space here intentional?

Not intentional. I though each 'set' should be separated for readability. Fixed 
it.


> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > src/tests/CMakeLists.txt, line 71
> > 
> >
> > Can we please use the Mesos style TODO format? Specifically:
> > 
> > * add semicolons after the `TODO(xxx):`
> > * capitalize the first letter of the message
> > * end message with a period
> > * indent to line up with the rest of the block (i.e., put 2 spaces 
> > between the start of the line and the `#` character)
> > 
> > In this case, it should look something like:
> > 
> > ```
> > TODO(srbrahma): Needs leveldb to compile.
> > ```
> > 
> > Also would be good to have a bug # that corresponds to the issue.
> 
> Alex Clemmer wrote:
> Actually, it looks like all of these `TODO`s are not quite correct -- we 
> do support leveldb build, and we don't need anything else except to build the 
> qos controller that I mentioned earlier. These can all be safely uncommented 
> as soon as you add that to the build.
> 
> Please do keep in mind the comment about the `TODO` formatting, though.
> 
> Alex Clemmer wrote:
> That said, I do believe you will have to add `state/leveldb.cpp` to 

Re: Review Request 49617: Add benchmark for failover of many frameworks.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49617]

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 8, 2016, 3:11 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49617/
> ---
> 
> (Updated July 8, 2016, 3:11 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5780
> https://issues.apache.org/jira/browse/MESOS-5780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This benchmark measures latency to stability of
>   the allocator following disconnection and
>   reconnection of all frameworks.
> - In this scenario, frameworks are offered resources
>   and suppressed in batches.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49617/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

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

[Srinivas Brahmaroutu]

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

(Updated July 19, 2016, 9:12 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 (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
1c9e17b23d7f2ee49e00b80053e4de1797d73c73 
  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 49348: Added implementation to Appc Runtime Isolator.

[Srinivas Brahmaroutu]

> On July 7, 2016, 6:38 a.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/isolators/appc/runtime.cpp, line 265
> > 
> >
> > Seems you are losing the logic to handle logic in row 2 when there are 
> > arguments?
> 
> Gilbert Song wrote:
> The logic seems fine to me here. That case is covered.
> 
> Guangya Liu wrote:
> Seems this will only cover line 1 but not line 2, comments?
> 
> Gilbert Song wrote:
> We dont need to do anything special to line 2. Could you verify? I can be 
> wrong.

@gyliu I fixed the comment. When command is absent, we first set exec[0] as the 
command and then we replace arguments with exec[1]... only when they are not 
present. 
I do not want to append the args after exec[1]... or vice versa. More common 
use case where first argument to a executable is the ip address then command 
exec[1] set in image as -host=localhost which user likely to replace with 
proper ip.


> On July 7, 2016, 6:38 a.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/isolators/appc/runtime.cpp, lines 215-219
> > 
> >
> > So for the case of sh=0,value=0,argv=1,Exec=1, what about the value of 
> > `Exec[1]...` etc? Should not it be `./Exec[0] Exec[1] ... argv`
> 
> Gilbert Song wrote:
> Thanks Guangya. Let's keep this open. 
> 
> It depends on whether or not users have the ability to overwrite.
> 
> Guangya Liu wrote:
> Yes, but I thin at least we need to clarify the behaviro in comments and 
> the document if there are multiple exec.
> 
> Gilbert Song wrote:
> Guangya, I think it is the right semantic to have the users able to 
> overwrite. Thanks for deep dive.
> 
> @Srini, could you add comments on why we do it this way?

I have added more information into the comments above the table.


- Srinivas


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


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

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

[Srinivas Brahmaroutu]

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

(Updated July 19, 2016, 9:11 p.m.)


Review request for mesos.


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


Repository: mesos


Description
---

Added implementation to Appc Runtime Isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
19c68e8a25d9ceedc5dfd562e287d6b6a56a9d3a 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 50199: Refactored fetcher cache directory creation.

[Gilbert Song]

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




src/launcher/fetcher.cpp (line 433)


This var may not be necessary.



src/launcher/fetcher.cpp (lines 442 - 448)


Could we move this logic inside `if (item.action() != 
FetcherInfo::Item::BYPASS_CACHE) {}`?


- Gilbert Song


On July 19, 2016, 1:54 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 19, 2016, 1:54 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
> chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50200: Made the agent fetch files as the task user.

[Gilbert Song]

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




src/launcher/fetcher.cpp (lines 519 - 525)


An open discussion:

After looking at os::chown(), we should not only change the file owner, but 
also the groups. So only get and set the uid looks insufficient to me. We 
should consider to do the same to the gid and the supplimentary groups (please 
look at /mesos/launch.cpp for examples).


- Gilbert Song


On July 19, 2016, 1:53 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50200/
> ---
> 
> (Updated July 19, 2016, 1:53 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: mesos-5845
> https://issues.apache.org/jira/browse/mesos-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To ensure that a task cannot fetch root-protected
> files from the local filesystem when running as a
> non-root user, this patch changes the fetcher to
> fetch files as the task user.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
>   src/tests/fetcher_tests.cpp d38ce6e750dc828ef5af4a27fac76327cc4cb56c 
> 
> Diff: https://reviews.apache.org/r/50200/diff/
> 
> 
> Testing
> ---
> 
> A new test was added to the fetcher tests: 
> `FetcherTest.ROOT_RootProtectedFileURI`.
> 
> `sudo make check` was used to test on both OSX and CentOS 7.
> 
> Note that two of the fetcher tests fail for me when run as root on OSX. I saw 
> the following on my OSX 10.10.5 system:
> ```
> [  FAILED  ] FetcherCacheTest.LocalUncachedExtract
> [  FAILED  ] FetcherCacheHttpTest.HttpMixed
> ```
> 
> These failures are already tracked here: 
> https://issues.apache.org/jira/browse/MESOS-4890
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50199: Refactored fetcher cache directory creation.

[Greg Mann]

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

(Updated July 19, 2016, 8:54 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


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


Repository: mesos


Description
---

The fetcher's launcher creates the fetcher cache
directory for each user immediately before an artifact
is fetched. In order to allow this directory to be
created by a different user than the user doing the
fetching, this patch factors out this directory
creation and places it before all fetches occur.


Diffs
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 

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


Testing (updated)
---

`sudo make check` on both OSX and CentOS 7 was done at the end of this patch 
chain.


Thanks,

Greg Mann

Re: Review Request 50200: Made the agent fetch files as the task user.

[Greg Mann]

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

(Updated July 19, 2016, 8:53 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Bugs: mesos-5845
https://issues.apache.org/jira/browse/mesos-5845


Repository: mesos


Description
---

To ensure that a task cannot fetch root-protected
files from the local filesystem when running as a
non-root user, this patch changes the fetcher to
fetch files as the task user.


Diffs
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
  src/tests/fetcher_tests.cpp d38ce6e750dc828ef5af4a27fac76327cc4cb56c 

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


Testing (updated)
---

A new test was added to the fetcher tests: 
`FetcherTest.ROOT_RootProtectedFileURI`.

`sudo make check` was used to test on both OSX and CentOS 7.

Note that two of the fetcher tests fail for me when run as root on OSX. I saw 
the following on my OSX 10.10.5 system:
```
[  FAILED  ] FetcherCacheTest.LocalUncachedExtract
[  FAILED  ] FetcherCacheHttpTest.HttpMixed
```

These failures are already tracked here: 
https://issues.apache.org/jira/browse/MESOS-4890


Thanks,

Greg Mann

Re: Review Request 50200: Made the agent fetch files as the task user.

[Greg Mann]

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

(Updated July 19, 2016, 8:53 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Bugs: mesos-5845
https://issues.apache.org/jira/browse/mesos-5845


Repository: mesos


Description
---

To ensure that a task cannot fetch root-protected
files from the local filesystem when running as a
non-root user, this patch changes the fetcher to
fetch files as the task user.


Diffs
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
  src/tests/fetcher_tests.cpp d38ce6e750dc828ef5af4a27fac76327cc4cb56c 

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


Testing (updated)
---

A new test was added to the fetcher tests: 
`FetcherTest.ROOT_RootProtectedFileURI`.

`sudo make check` was used to test on CentOS 7.

Note that two of the fetcher tests fail for me when run as root on OSX. I saw 
the following on my OSX 10.10.5 system:
```
[  FAILED  ] FetcherCacheTest.LocalUncachedExtract
[  FAILED  ] FetcherCacheHttpTest.HttpMixed
```

These failures are already tracked here: 
https://issues.apache.org/jira/browse/MESOS-4890


Thanks,

Greg Mann

Re: Review Request 50199: Refactored fetcher cache directory creation.

[Joerg Schad]

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




src/launcher/fetcher.cpp (line 381)


Does it make sense to add a check here to document and check that 
precondition?



src/launcher/fetcher.cpp (line 431)


Not an issue: just making sure I understood this correctly :-): if there is 
no user we do not need to create directory i.e., a global/non user-specific 
cache dir is created upfront?



src/launcher/fetcher.cpp (line 448)


Should we check whether chown was sucessful?



src/launcher/fetcher.cpp (line 508)


Is there a reason to do this outside `createCacheDirectory`? We are already 
passing fetcherInfo...



src/launcher/fetcher.cpp (line 512)


Should we check whether an error was returned?


- Joerg Schad


On July 19, 2016, 8:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50199/
> ---
> 
> (Updated July 19, 2016, 8:07 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.
> 
> 
> Bugs: MESOS-5845
> https://issues.apache.org/jira/browse/MESOS-5845
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fetcher's launcher creates the fetcher cache
> directory for each user immediately before an artifact
> is fetched. In order to allow this directory to be
> created by a different user than the user doing the
> fetching, this patch factors out this directory
> creation and places it before all fetches occur.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
> 
> Diff: https://reviews.apache.org/r/50199/diff/
> 
> 
> Testing
> ---
> 
> `make check` on CentOS 7 was done at the end of this patch chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 50200: Made the agent fetch files as the task user.

[Greg Mann]

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

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


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Bugs: mesos-5845
https://issues.apache.org/jira/browse/mesos-5845


Repository: mesos


Description
---

To ensure that a task cannot fetch root-protected
files from the local filesystem when running as a
non-root user, this patch changes the fetcher to
fetch files as the task user.


Diffs
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
  src/tests/fetcher_tests.cpp d38ce6e750dc828ef5af4a27fac76327cc4cb56c 

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


Testing (updated)
---

A new test was added to the fetcher tests: 
`FetcherTest.ROOT_RootProtectedFileURI`.

`sudo make check` was used to test on CentOS 7.


Thanks,

Greg Mann

Review Request 50200: Made the agent fetch files as the task user.

[Greg Mann]

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

Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


Bugs: mesos-5845
https://issues.apache.org/jira/browse/mesos-5845


Repository: mesos


Description
---

To ensure that a task cannot fetch root-protected
files from the local filesystem when running as a
non-root user, this patch changes the fetcher to
fetch files as the task user.


Diffs
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 
  src/tests/fetcher_tests.cpp d38ce6e750dc828ef5af4a27fac76327cc4cb56c 

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


Testing
---

A new test was added to the fetcher tests: 
`FetcherTest.ROOT_RootProtectedFileURI`.

`sudo make check` was used to test on both OSX and CentOS 7.


Thanks,

Greg Mann

Review Request 50199: Refactored fetcher cache directory creation.

[Greg Mann]

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

Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


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


Repository: mesos


Description
---

The fetcher's launcher creates the fetcher cache
directory for each user immediately before an artifact
is fetched. In order to allow this directory to be
created by a different user than the user doing the
fetching, this patch factors out this directory
creation and places it before all fetches occur.


Diffs
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 

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


Testing
---

`make check` on OSX and CentOS 7 was done at the end of this patch chain.


Thanks,

Greg Mann

Re: Review Request 50199: Refactored fetcher cache directory creation.

[Greg Mann]

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

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


Review request for mesos, Gilbert Song, Jie Yu, and Joerg Schad.


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


Repository: mesos


Description
---

The fetcher's launcher creates the fetcher cache
directory for each user immediately before an artifact
is fetched. In order to allow this directory to be
created by a different user than the user doing the
fetching, this patch factors out this directory
creation and places it before all fetches occur.


Diffs
-

  src/launcher/fetcher.cpp 0539b0182bd4a7178f1031ab4fee8fc79eda 

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


Testing (updated)
---

`make check` on CentOS 7 was done at the end of this patch chain.


Thanks,

Greg Mann

Re: Review Request 49892: Removed unnecessary await from test.

[Joerg Schad]

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

(Updated July 19, 2016, 7:48 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Removed unnecessary await from test.


Diffs (updated)
-

  src/tests/master_slave_reconciliation_tests.cpp 
3bb64995c076a8f6a09d4bacde4335c6fb8ade78 

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


Testing
---

make check --gtest_repeat=100


Thanks,

Joerg Schad

Re: Review Request 46626: Added example framework for testing disk quota enforcement.

[Artem Harutyunyan]

> On July 18, 2016, 2:28 p.m., Joseph Wu wrote:
> > src/Makefile.am, line 1898
> > 
> >
> > Can you also add this to `examples_tests.cpp`?

Will do in a followup review.


- Artem


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


On July 19, 2016, 12:15 p.m., Artem Harutyunyan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46626/
> ---
> 
> (Updated July 19, 2016, 12:15 p.m.)
> 
> 
> Review request for Joseph Wu.
> 
> 
> Bugs: MESOS-5855
> https://issues.apache.org/jira/browse/MESOS-5855
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added example framework for testing disk quota enforcement.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am cc83525a4455bbb0e654d346921d66ed2436411d 
>   src/examples/disk_full_framework.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46626/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Artem Harutyunyan
> 
>

Re: Review Request 46626: Added example framework for testing disk quota enforcement.

[Artem Harutyunyan]

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

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


Review request for Joseph Wu.


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


Repository: mesos


Description
---

Added example framework for testing disk quota enforcement.


Diffs (updated)
-

  src/Makefile.am cc83525a4455bbb0e654d346921d66ed2436411d 
  src/examples/disk_full_framework.cpp PRE-CREATION 

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


Testing
---


Thanks,

Artem Harutyunyan

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

[Srinivas Brahmaroutu]

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

(Updated July 19, 2016, 6:30 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 (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
1c9e17b23d7f2ee49e00b80053e4de1797d73c73 
  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 49348: Added implementation to Appc Runtime Isolator.

[Srinivas Brahmaroutu]

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

(Updated July 19, 2016, 6:31 p.m.)


Review request for mesos.


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


Repository: mesos


Description
---

Added implementation to Appc Runtime Isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
19c68e8a25d9ceedc5dfd562e287d6b6a56a9d3a 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 50187: Removed the `os::sleep` from `Clock::settle`.

[Benjamin Mahler]

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


Ship it!




Thanks!

- Benjamin Mahler


On July 19, 2016, 10:08 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50187/
> ---
> 
> (Updated July 19, 2016, 10:08 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-3760
> https://issues.apache.org/jira/browse/MESOS-3760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Clock::settle` is used to wait until all pending libprocess events
> have been handled. Test cases should typically use it only when there is
> no other way to achieve the proper synchronization between two events.
> 
> Previously, `Clock::settle` also contained an `os::sleep` call to
> workaround broken test cases that assumed `settle` provided stronger
> guarantees than described above (e.g., some test cases assumed that
> doing `http::get` followed by a `Clock::settle` ensured that the remote
> side of the HTTP connection will have seen the request). Currently,
> there are relatively few such test cases, so it is better to fix those
> test cases (or add a `sleep` call to them) and remove the `sleep` from
> `Clock::settle`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 9661386afd4fddd1877d55941fa403afc9230280 
>   3rdparty/libprocess/src/tests/metrics_tests.cpp 
> 5a82f4f49aecd03d12687de629516be5b7895036 
> 
> Diff: https://reviews.apache.org/r/50187/diff/
> 
> 
> Testing
> ---
> 
> `make check` with lots of iterations.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50172: Made `ports` resource configurable in sorter benchmark test.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50172]

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 19, 2016, 3:59 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50172/
> ---
> 
> (Updated July 19, 2016, 3:59 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added three new functions to sorter benchmark test to
> generate a `ports` resource which falls into a specifed ports
> range bounds.
> 
> 1) makePorts: Creates a "ports(*)" resource for the given ranges.
> 2) fragment: Fragments the given range bounds into a number of
>subranges.
> 3) makeRange: Returns the range bounds.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp b0e5ef8a55bbcca3553a221bd5691a9c801a04f7 
> 
> Diff: https://reviews.apache.org/r/50172/diff/
> 
> 
> Testing
> ---
> 
> ```
> ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1"
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1
> Using 1000 agents and 50 clients
> Added 50 clients in 872us
> Added 1000 agents in 26457us
> Added allocations for 1000 agents in 79697us
> Full sort of 50 clients took 1321us
> No-op sort of 50 clients took 28us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1 (115 ms)
> [--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test (115 ms 
> total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (133 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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

[Gilbert Song]

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




src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 247)


This is a bug. You may get segfault if `!command.has_value() === true`.

Let's dont check `command.value().empty()` here. If it is an empty string, 
errors will be returned by `execlp`.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 255)


Same here. If we check on `!command.value().empty()` here. We are leaking 
some cases in logic. Please remove it.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 263 - 289)


We can optimize the logic here:

```
if (app.exec_size() > 0) {
  command.set_value(app.exec(0));
  
  command.clear_arguments();
  command.add_arguments(app.exec(0));
  
  if (!containerConfig.has_task_info()) {
command.mutable_ARGUMENTS()->MergeFrom(
containerConfig.executor_info().command().arguments());
  } else {
command.mutable_arguments()->MergeFrom(
containerConfig.task_info().command().arguments());
  }
  
  if (command.arguments_size() == 1) {
for (int i = 1; i < app.exec_size(); i++) {
  command.add_arguments(app.exec(i));
}
  }
} else {
  return Error("No executable is found");
}


- Gilbert Song


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

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

> On July 19, 2016, 10:03 a.m., Jiang Yan Xu wrote:
> > It would be helpful to add a test just for contains. i.e., to test how 
> > `contains` works with nonshared resources and how it works with simple 
> > shared resources (could be constructed from createDiskResources with no 
> > arithmetic operations).
> > 
> > Also as a followup add a benchmark for resource operations. (Let's check 
> > with the IBM folks on their progress in adding DiskInfo and Reservation

Finishing my sentense above: "Diskinfo and ReservationInfo" to resources 
benchmarks)"


- Jiang Yan


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


On July 18, 2016, 7:29 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 18, 2016, 7:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-4892
> https://issues.apache.org/jira/browse/MESOS-4892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A new class Resource_ is added that allows 'Resources' to group
> identical shared resource objects together into a single 'Resource_'
> object and tracked by its shared count. Non-shared resource objects
> are not grouped.
> 
> For resource addition and subtraction, the shared count is adjusted for
> shared resources as follows:
> a) Addition: If shared resource is absent from original, then the
>resource is added initialized with a consumer count of 1. Otherwise,
>the share count for the shared resource is incremented.
> b) Subtraction: If shared resource's share count is already 1, then
>the shared resource is removed from the original. Otherwise, its
>consumer count is decremented.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
>   src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
>   src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 
> 
> Diff: https://reviews.apache.org/r/45959/diff/
> 
> 
> Testing
> ---
> 
> New tests added to demonstrate arithmetic operations for shared resources 
> with consumer counts.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

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



It would be helpful to add a test just for contains. i.e., to test how 
`contains` works with nonshared resources and how it works with simple shared 
resources (could be constructed from createDiskResources with no arithmetic 
operations).

Also as a followup add a benchmark for resource operations. (Let's check with 
the IBM folks on their progress in adding DiskInfo and Reservation


src/tests/resources_tests.cpp (lines 2602 - 2603)


Why adding the two `Resources::parse()` results here?

Can we directly

```
Resources r1 = Resources::parse("cpus:40;mem:4096").get() + disk + disk;
```

? This would fit in one line (if it doesn't, use two space indentation).



src/tests/resources_tests.cpp (line 2607)


s/diff1/diff/ (due to the rename suggestion below).



src/tests/resources_tests.cpp (line 2615)


How about also throwing in a check for `count`:

```
EXPECT_TRUE(diff.count(disk));
```



src/tests/resources_tests.cpp (line 2617)


s/diff2/r/ as it's technically not a diff.



src/tests/resources_tests.cpp (lines 2623 - 2626)


For completeness let's verify the (pre-)condition before we take the diff.

i.e., add the following above this block.

```
EXPECT_EQ(2, r1.count(disk));
EXPECT_TRUE(r1.contains(disk));
EXPECT_EQ(1, r2.count(disk));
EXPECT_TRUE(r2.contains(disk));
```



src/tests/resources_tests.cpp (lines 2634 - 2637)


2 space indentation.



src/tests/resources_tests.cpp (line 2647)


Seems like this should fail?

This should be true?
```
EXPECT_EQ(r2 - r1 + r1, r2);
```



src/tests/resources_tests.cpp (lines 2650 - 2660)


These feel redundant as ScalarSubtractionShared has covered this case. 
Remove them?



src/tests/resources_tests.cpp (line 2670)


s/ScalarSharedNonEqualSharedOperations/ScalarNonEqualSharedOperations/



src/tests/resources_tests.cpp (lines 2679 - 2700)


Can we directly create r1, r2, r3 and r4 from disk1, disk2 and disk3, i.e., 
`Resources r2 = disk1 + disk2;` so each test block is independent so the test 
is easier to follow?


- Jiang Yan Xu


On July 18, 2016, 7:29 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated July 18, 2016, 7:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-4892
> https://issues.apache.org/jira/browse/MESOS-4892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A new class Resource_ is added that allows 'Resources' to group
> identical shared resource objects together into a single 'Resource_'
> object and tracked by its shared count. Non-shared resource objects
> are not grouped.
> 
> For resource addition and subtraction, the shared count is adjusted for
> shared resources as follows:
> a) Addition: If shared resource is absent from original, then the
>resource is added initialized with a consumer count of 1. Otherwise,
>the share count for the shared resource is incremented.
> b) Subtraction: If shared resource's share count is already 1, then
>the shared resource is removed from the original. Otherwise, its
>consumer count is decremented.
> 
> Note that v1 changes for shared resources are in the next commit.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/master/validation.cpp 50ba372d1301d16d1738a3e4f4882b51f9ce06cd 
>   src/tests/mesos.hpp e4eccfc3810bed3649a3ab80e252849470de4c72 
>   src/tests/resources_tests.cpp 40d290ac540d26373c5fb7c2a93d27d1aa61d722 
> 
> Diff: https://reviews.apache.org/r/45959/diff/
> 
> 
> Testing
> ---
> 
> New tests added to demonstrate arithmetic operations for shared resources 
> with consumer counts.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 49617: Add benchmark for failover of many frameworks.

[Jiang Yan Xu]

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




src/tests/hierarchical_allocator_tests.cpp (line 3724)


Ditto. Unnecessary `i < frameworkCount`.


- Jiang Yan Xu


On July 7, 2016, 8:11 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49617/
> ---
> 
> (Updated July 7, 2016, 8:11 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5780
> https://issues.apache.org/jira/browse/MESOS-5780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This benchmark measures latency to stability of
>   the allocator following disconnection and
>   reconnection of all frameworks.
> - In this scenario, frameworks are offered resources
>   and suppressed in batches.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49617/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 49617: Add benchmark for failover of many frameworks.

[Jiang Yan Xu]

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




src/tests/hierarchical_allocator_tests.cpp (line 3690)


A blank line above.



src/tests/hierarchical_allocator_tests.cpp (line 3706)


A blank line above.



src/tests/hierarchical_allocator_tests.cpp (lines 3709 - 3710)


Empty lines above and below.



src/tests/hierarchical_allocator_tests.cpp (line 3717)


The std::min above already makes sure batchEnd is at most frameworkCount so 
we don't need to worry about i < frameworkCount here.



src/tests/hierarchical_allocator_tests.cpp (lines 3721 - 3723)


Add to the end: `, thus a separate loop.` so it's more clear?


- Jiang Yan Xu


On July 7, 2016, 8:11 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49617/
> ---
> 
> (Updated July 7, 2016, 8:11 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5780
> https://issues.apache.org/jira/browse/MESOS-5780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This benchmark measures latency to stability of
>   the allocator following disconnection and
>   reconnection of all frameworks.
> - In this scenario, frameworks are offered resources
>   and suppressed in batches.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49617/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 50172: Made `ports` resource configurable in sorter benchmark test.

[Guangya Liu]

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

(Updated 七月 19, 2016, 3:59 p.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description (updated)
---

This patch added three new functions to sorter benchmark test to
generate a `ports` resource which falls into a specifed ports
range bounds.

1) makePorts: Creates a "ports(*)" resource for the given ranges.
2) fragment: Fragments the given range bounds into a number of
   subranges.
3) makeRange: Returns the range bounds.


Diffs (updated)
-

  src/tests/sorter_tests.cpp b0e5ef8a55bbcca3553a221bd5691a9c801a04f7 

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


Testing
---

```
./bin/mesos-tests.sh --benchmark 
--gtest_filter="AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1"
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1
Using 1000 agents and 50 clients
Added 50 clients in 872us
Added 1000 agents in 26457us
Added allocations for 1000 agents in 79697us
Full sort of 50 clients took 1321us
No-op sort of 50 clients took 28us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1 (115 ms)
[--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test (115 ms 
total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (133 ms total)
[  PASSED  ] 1 test.
```


Thanks,

Guangya Liu

Re: Review Request 50187: Removed the `os::sleep` from `Clock::settle`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50187]

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 19, 2016, 10:08 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50187/
> ---
> 
> (Updated July 19, 2016, 10:08 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-3760
> https://issues.apache.org/jira/browse/MESOS-3760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Clock::settle` is used to wait until all pending libprocess events
> have been handled. Test cases should typically use it only when there is
> no other way to achieve the proper synchronization between two events.
> 
> Previously, `Clock::settle` also contained an `os::sleep` call to
> workaround broken test cases that assumed `settle` provided stronger
> guarantees than described above (e.g., some test cases assumed that
> doing `http::get` followed by a `Clock::settle` ensured that the remote
> side of the HTTP connection will have seen the request). Currently,
> there are relatively few such test cases, so it is better to fix those
> test cases (or add a `sleep` call to them) and remove the `sleep` from
> `Clock::settle`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 9661386afd4fddd1877d55941fa403afc9230280 
>   3rdparty/libprocess/src/tests/metrics_tests.cpp 
> 5a82f4f49aecd03d12687de629516be5b7895036 
> 
> Diff: https://reviews.apache.org/r/50187/diff/
> 
> 
> Testing
> ---
> 
> `make check` with lots of iterations.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50186: Avoid a GMock warning in a reservation test case.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50186]

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 19, 2016, 9:55 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50186/
> ---
> 
> (Updated July 19, 2016, 9:55 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Depending on timing, the master might rescind an offer, which causes a
> GMock warning. It is better to wait for the offer and then wait for it
> to be rescinded.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_tests.cpp 26bd762db657d5c83cf4b7575ecbbbc26e95d917 
> 
> Diff: https://reviews.apache.org/r/50186/diff/
> 
> 
> Testing
> ---
> 
> `./src/mesos-tests 
> --gtest_filter="ReservationTest.CompatibleCheckpointedResourcesWithPersistentVolumes"
>  --verbose --gtest_repeat=400`
> 
> Without the patch, emits a GMock warning on ~1% of runs. With the patch, does 
> not emit the warning. Note that the warning seems to occur more often if you 
> remove the `os::sleep` from `Clock::settle`, which we want to do anyway.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50184: Fixed length argument bug when reading file.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50184]

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 19, 2016, 7:51 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50184/
> ---
> 
> (Updated July 19, 2016, 7:51 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If length provided by client is bigger than
> (size - offset), the data returned by read
> file API should only contain the data actually
> read from the file. Now, a length long string
> will be returned which may contain blank characters.
> 
> 
> Diffs
> -
> 
>   src/files/files.cpp 8ab8f8aad9cfe1e536b29e8994fcd8599627e590 
>   src/tests/api_tests.cpp b4bee760cbc5cc90b488554f58daf23b7482d915 
> 
> Diff: https://reviews.apache.org/r/50184/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> run mesos-master with --log_dir or --external_log_file specified. 
> Open a browser and goto 127.0.0.1:5050, click 'LOG' link on the home page.
> The log of master shall be shown and updated correctly.
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Review Request 50187: Removed the `os::sleep` from `Clock::settle`.

[Neil Conway]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

`Clock::settle` is used to wait until all pending libprocess events
have been handled. Test cases should typically use it only when there is
no other way to achieve the proper synchronization between two events.

Previously, `Clock::settle` also contained an `os::sleep` call to
workaround broken test cases that assumed `settle` provided stronger
guarantees than described above (e.g., some test cases assumed that
doing `http::get` followed by a `Clock::settle` ensured that the remote
side of the HTTP connection will have seen the request). Currently,
there are relatively few such test cases, so it is better to fix those
test cases (or add a `sleep` call to them) and remove the `sleep` from
`Clock::settle`.


Diffs
-

  3rdparty/libprocess/src/process.cpp 9661386afd4fddd1877d55941fa403afc9230280 
  3rdparty/libprocess/src/tests/metrics_tests.cpp 
5a82f4f49aecd03d12687de629516be5b7895036 

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


Testing
---

`make check` with lots of iterations.


Thanks,

Neil Conway

Re: Review Request 50186: Avoid a GMock warning in a reservation test case.

[Neil Conway]

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

(Updated July 19, 2016, 9:55 a.m.)


Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

Depending on timing, the master might rescind an offer, which causes a
GMock warning. It is better to wait for the offer and then wait for it
to be rescinded.


Diffs
-

  src/tests/reservation_tests.cpp 26bd762db657d5c83cf4b7575ecbbbc26e95d917 

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


Testing (updated)
---

`./src/mesos-tests 
--gtest_filter="ReservationTest.CompatibleCheckpointedResourcesWithPersistentVolumes"
 --verbose --gtest_repeat=400`

Without the patch, emits a GMock warning on ~1% of runs. With the patch, does 
not emit the warning. Note that the warning seems to occur more often if you 
remove the `os::sleep` from `Clock::settle`, which we want to do anyway.


Thanks,

Neil Conway

Re: Review Request 50181: [WIP] Fixed the flaky test case `MasterAPITest.GetTasks`.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [50181]

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

Error:
2016-07-19 09:48:36 URL:https://reviews.apache.org/r/50181/diff/raw/ 
[1492/1492] -> "50181.patch" [1]
Total errors found: 0
Checking 1 files
Error: Commit message summary (the first line) must start with a capital letter.

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

- Mesos ReviewBot


On July 19, 2016, 7:01 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50181/
> ---
> 
> (Updated July 19, 2016, 7:01 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jay Guo, and Vinod Kone.
> 
> 
> Bugs: MESOS-5860
> https://issues.apache.org/jira/browse/MESOS-5860
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes to wait for every `StatusUpdateAcknowledgementMessage`
> exactly in this test case to make sure they don't conflict each other.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp b4bee760cbc5cc90b488554f58daf23b7482d915 
> 
> Diff: https://reviews.apache.org/r/50181/diff/
> 
> 
> Testing
> ---
> 
> This is working in progress because I have not yet reproduce it.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 50072: Speed up reservation test by setting allocation interval in master flags.

[Abhishek Dasgupta]

> On July 19, 2016, 12:51 a.m., Vinod Kone wrote:
> > src/tests/api_tests.cpp, lines 946-1041
> > 
> >
> > Why this change? The context is not clear from the description or the 
> > ticket.
> 
> Abhishek Dasgupta wrote:
> My bad. MasterFlags should not be placed here. But class MasterAPITest 
> should look like this as it was in my first version: 
> class MasterAPITest
>   : public MesosTest,
> public WithParamInterface
> {
> public:
>   // Set up the master flags such that it allows registration of the 
> framework
>   // created with 'createFrameworkInfo'.
>   virtual master::Flags CreateMasterFlags()
>   {
> master::Flags flags = MesosTest::CreateMasterFlags();
> flags.allocation_interval = Milliseconds(50);
> flags.roles = createFrameworkInfo().role();
> return flags;
>   }
>   
> So here are results of RerverResource before and after making this change:
> 
> Before change:
> [==] Running 2 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 2 tests from ContentType/MasterAPITest
> [ RUN  ] ContentType/MasterAPITest.ReserveResources/0
> [   OK ] ContentType/MasterAPITest.ReserveResources/0 (1054 ms)
> [ RUN  ] ContentType/MasterAPITest.ReserveResources/1
> [   OK ] ContentType/MasterAPITest.ReserveResources/1 (1036 ms)
> [--] 2 tests from ContentType/MasterAPITest (2097 ms total)
> 
> [--] Global test environment tear-down
> [==] 2 tests from 1 test case ran. (2103 ms total)
> [  PASSED  ] 2 tests.
> 
> After change:
> [==] Running 2 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 2 tests from ContentType/MasterAPITest
> [ RUN  ] ContentType/MasterAPITest.ReserveResources/0
> [   OK ] ContentType/MasterAPITest.ReserveResources/0 (149 ms)
> [ RUN  ] ContentType/MasterAPITest.ReserveResources/1
> [   OK ] ContentType/MasterAPITest.ReserveResources/1 (129 ms)
> [--] 2 tests from ContentType/MasterAPITest (286 ms total)
> 
> 
> So, should I go ahead with the above mentioned change and change the 
> description of this review? Btw, I am putting you as reviewer of this patch.

I made the aforementioned changes.


- Abhishek


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


On July 19, 2016, 8:47 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50072/
> ---
> 
> (Updated July 19, 2016, 8:47 a.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
> ---
> 
> ReserveResource test (and UnreserveResource test) in v1 operator
> API is very slow and taking more than 1000 ms. If we set allocation
> interval in master flag to 50ms and use that flag to start up a
> master to use in the test, we can reduce the duration time for
> that test as low as 120 ms. So, in this patch we made that change.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp b4bee76 
> 
> Diff: https://reviews.apache.org/r/50072/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

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

[Abhishek Dasgupta]

> On July 19, 2016, 12:49 a.m., Vinod Kone wrote:
> > src/tests/mesos.hpp, lines 610-615
> > 
> >
> > I think a generic `createFrameworkInfo()` that returns 
> > DEFAULT_FRAMEWORK_INFO with "role1" is unintuitive for callers. How would 
> > they know they get "role1" FrameworkInfo?
> > 
> > I wouldn't do this change.
> 
> Abhishek Dasgupta wrote:
> Ok. I am discarding it.

Also, if possible can you close the ticket MESOS-5725?


- Abhishek


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


On July 18, 2016, 8:11 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49913/
> ---
> 
> (Updated July 18, 2016, 8:11 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 37bf2866e1dbf7a8bead0c93825666921f4228fb 
>   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 50072: Speed up reservation test by setting allocation interval in master flags.

[Abhishek Dasgupta]

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

(Updated July 19, 2016, 8:47 a.m.)


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


Summary (updated)
-

Speed up reservation test by setting allocation interval in master flags.


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


Repository: mesos


Description (updated)
---

ReserveResource test (and UnreserveResource test) in v1 operator
API is very slow and taking more than 1000 ms. If we set allocation
interval in master flag to 50ms and use that flag to start up a
master to use in the test, we can reduce the duration time for
that test as low as 120 ms. So, in this patch we made that change.


Diffs (updated)
-

  src/tests/api_tests.cpp b4bee76 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 50179: Added mesos-logrotate-logger utility executable.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [50179]

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

Error:
2016-07-19 08:36:24 URL:https://reviews.apache.org/r/50179/diff/raw/ 
[3059/3059] -> "50179.patch" [1]
No files to lint

Error: No line in the commit message summary may exceed 72 characters.

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

- Mesos ReviewBot


On July 19, 2016, 4:23 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50179/
> ---
> 
> (Updated July 19, 2016, 4:23 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 mesos-logrotate-logger utility executable.
> 
> This utility executable will allow to test logrotate module through 
> modulemanager.
> 
> 
> Diffs
> -
> 
>   src/slave/CMakeLists.txt bb9ad62b2372ca038c43a20d9906aaf43d9ead41 
>   src/slave/cmake/SlaveConfigure.cmake 
> ced57496970f1d7edf9e7e443b22d14d2ee948f0 
>   src/slave/container_loggers/CMakeLists.txt PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50179/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make mesos-logrotate-logger
> GLOG_v=1 ./src/mesos-tests 
> --gtest_filter="ContainerLogger*.LOGROTATE_RotateInSandbox" should run 
> successfully.
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 50172: Made `ports` resource configurable in sorter benchmark test.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50172]

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 19, 2016, 3:35 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50172/
> ---
> 
> (Updated July 19, 2016, 3:35 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added two new functions to sorter benchmark test to
> generate a `ports` resource which falls into a specifed ports
> range bounds.
> 
> 1) fragment: Returns a "ports" resource with the number of ranges.
> 2) makeRange: Returns the "ports" range bounds.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp b0e5ef8a55bbcca3553a221bd5691a9c801a04f7 
> 
> Diff: https://reviews.apache.org/r/50172/diff/
> 
> 
> Testing
> ---
> 
> ```
> ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1"
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1
> Using 1000 agents and 50 clients
> Added 50 clients in 872us
> Added 1000 agents in 26457us
> Added allocations for 1000 agents in 79697us
> Full sort of 50 clients took 1321us
> No-op sort of 50 clients took 28us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1 (115 ms)
> [--] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test (115 ms 
> total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (133 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50072: Set allocation interval in 'Master Flags' for operator API reservation tests.

[Abhishek Dasgupta]

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

(Updated July 19, 2016, 7:24 a.m.)


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


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


Repository: mesos


Description
---

Set allocation interval in 'Master Flags' for operator
API reservation tests.


Diffs
-

  src/tests/api_tests.cpp 37bf2866e1dbf7a8bead0c93825666921f4228fb 

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


Testing
---


Thanks,

Abhishek Dasgupta

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

[Abhishek Dasgupta]

> On July 19, 2016, 12:57 a.m., Vinod Kone wrote:
> > This change looks ok to me and indepdendent of the previous 2 reviews in 
> > the chain. If yes, I'm happy to commit this if you remove the dependency. 
> > You might be able to discard the first 2 reviews even.

I discarded the first one, but for 2nd one I put a comment. I would discard or 
update that patch based on your reply.


- Abhishek


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


On July 18, 2016, 8:20 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49914/
> ---
> 
> (Updated July 18, 2016, 8:20 a.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 37bf2866e1dbf7a8bead0c93825666921f4228fb 
> 
> Diff: https://reviews.apache.org/r/49914/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 50181: [WIP] Fixed the flaky test case `MasterAPITest.GetTasks`.

[haosdent huang]

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

(Updated July 19, 2016, 7:01 a.m.)


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


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


Repository: mesos


Description
---

This changes to wait for every `StatusUpdateAcknowledgementMessage`
exactly in this test case to make sure they don't conflict each other.


Diffs
-

  src/tests/api_tests.cpp b4bee760cbc5cc90b488554f58daf23b7482d915 

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


Testing (updated)
---

This is working in progress because I have not yet reproduce it.


Thanks,

haosdent huang

Review Request 50181: [WIP] Fixed the flaky test case `MasterAPITest.GetTasks`.

[haosdent huang]

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

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


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


Repository: mesos


Description
---

This changes to wait for every `StatusUpdateAcknowledgementMessage`
exactly in this test case to make sure they don't conflict each other.


Diffs
-

  src/tests/api_tests.cpp b4bee760cbc5cc90b488554f58daf23b7482d915 

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


Testing
---


Thanks,

haosdent huang

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

[Abhishek Dasgupta]

> On July 19, 2016, 12:49 a.m., Vinod Kone wrote:
> > src/tests/mesos.hpp, lines 610-615
> > 
> >
> > I think a generic `createFrameworkInfo()` that returns 
> > DEFAULT_FRAMEWORK_INFO with "role1" is unintuitive for callers. How would 
> > they know they get "role1" FrameworkInfo?
> > 
> > I wouldn't do this change.

Ok. I am discarding it.


- Abhishek


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


On July 18, 2016, 8:11 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49913/
> ---
> 
> (Updated July 18, 2016, 8:11 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 37bf2866e1dbf7a8bead0c93825666921f4228fb 
>   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 50072: Set allocation interval in 'Master Flags' for operator API reservation tests.

[Abhishek Dasgupta]

> On July 19, 2016, 12:51 a.m., Vinod Kone wrote:
> > src/tests/api_tests.cpp, lines 946-1041
> > 
> >
> > Why this change? The context is not clear from the description or the 
> > ticket.

My bad. MasterFlags should not be placed here. But class MasterAPITest should 
look like this as it was in my first version: 
class MasterAPITest
  : public MesosTest,
public WithParamInterface
{
public:
  // Set up the master flags such that it allows registration of the framework
  // created with 'createFrameworkInfo'.
  virtual master::Flags CreateMasterFlags()
  {
master::Flags flags = MesosTest::CreateMasterFlags();
flags.allocation_interval = Milliseconds(50);
flags.roles = createFrameworkInfo().role();
return flags;
  }
  
So here are results of RerverResource before and after making this change:

Before change:
[==] Running 2 tests from 1 test case.
[--] Global test environment set-up.
[--] 2 tests from ContentType/MasterAPITest
[ RUN  ] ContentType/MasterAPITest.ReserveResources/0
[   OK ] ContentType/MasterAPITest.ReserveResources/0 (1054 ms)
[ RUN  ] ContentType/MasterAPITest.ReserveResources/1
[   OK ] ContentType/MasterAPITest.ReserveResources/1 (1036 ms)
[--] 2 tests from ContentType/MasterAPITest (2097 ms total)

[--] Global test environment tear-down
[==] 2 tests from 1 test case ran. (2103 ms total)
[  PASSED  ] 2 tests.

After change:
[==] Running 2 tests from 1 test case.
[--] Global test environment set-up.
[--] 2 tests from ContentType/MasterAPITest
[ RUN  ] ContentType/MasterAPITest.ReserveResources/0
[   OK ] ContentType/MasterAPITest.ReserveResources/0 (149 ms)
[ RUN  ] ContentType/MasterAPITest.ReserveResources/1
[   OK ] ContentType/MasterAPITest.ReserveResources/1 (129 ms)
[--] 2 tests from ContentType/MasterAPITest (286 ms total)


So, should I go ahead with the above mentioned change and change the 
description of this review? Btw, I am putting you as reviewer of this patch.


- Abhishek


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


On July 18, 2016, 8:16 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50072/
> ---
> 
> (Updated July 18, 2016, 8:16 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Neil Conway.
> 
> 
> Bugs: MESOS-5725
> https://issues.apache.org/jira/browse/MESOS-5725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set allocation interval in 'Master Flags' for operator
> API reservation tests.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 37bf2866e1dbf7a8bead0c93825666921f4228fb 
> 
> Diff: https://reviews.apache.org/r/50072/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 50177: Add systemd watchdog support.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [50177]

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

Error:
2016-07-19 06:07:28 URL:https://reviews.apache.org/r/50177/diff/raw/ 
[3485/3485] -> "50177.patch" [1]
Total errors found: 0
Checking 3 files
Error: No line in the commit message summary may exceed 72 characters.

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

- Mesos ReviewBot


On July 19, 2016, 1:14 a.m., Lawrence Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50177/
> ---
> 
> (Updated July 19, 2016, 1:14 a.m.)
> 
> 
> Review request for mesos, David Robinson and Ian Downes.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds systemd watchdog support (see 
> http://0pointer.de/blog/projects/watchdog.html for context).
> 
> 
> Diffs
> -
> 
>   configure.ac d2136909b7305498ae901a5ea00133142b77f9e6 
>   src/linux/systemd.hpp 91134f1d4b100759e45931bd09ca4e1e1aeaaf8a 
>   src/linux/systemd.cpp 619aa2778da5f99d3a078a8e1208bdaa9dc77581 
>   src/slave/main.cpp 4624392d30cf391015dcd63f447fe2414a47a16a 
> 
> Diff: https://reviews.apache.org/r/50177/diff/
> 
> 
> Testing
> ---
> 
> Tested by sending SIGSTOP to running mesos and verifying via journalctl that 
> it was killed by the watchdog.
> 
> TODO: write actual test by using and forking a mock binary that initializes 
> systemd and sending SIGSTOP to that binary. test also needs a unit file for 
> that binary and for systemd to run so we can verify that systemd killed it.
> 
> 
> File Attachments
> 
> 
> mesos gets owned by watchdog
>   
> https://reviews.apache.org/media/uploaded/files/2016/07/19/3c73b595-d28e-45d4-adfe-697801ba02cc__Screen_Shot_2016-07-18_at_2.09.31_PM.png
> 
> 
> Thanks,
> 
> Lawrence Wu
> 
>