Re: Review Request 46298: Rejected relative path agent work_dir.

[Klaus Ma]

> On July 12, 2016, 4:23 a.m., Jie Yu wrote:
> > src/slave/main.cpp, lines 175-179
> > 
> >
> > Can we do that check in `add` function. `add` function supports an 
> > optional validate lambda to be passed in.
> 
> Klaus Ma wrote:
> @jie, thanks for the comments. Try to use validated lamba today, but 
> there some issues:
> 
> 1. The `work_dir` is required without default value; if using validate 
> lamba, a "default" value is necessary: `add(T1* t1, name, alias, 
> defaultValue, lamba)`
> 2. The default value is pass by const reference currently, we can not 
> pass some empty value to it, e.g. nullptr or 0
> 
> For the default value, I'm thinking to replace const reference with 
> `Option`; any suggestion?
> 
> Jie Yu wrote:
> Oh, I don't realize that we don't have validation support for required 
> field without default value.
> 
> Can we introduce another `add` variant in flags that support the above 
> case? Is that difficult?

Found a `add` function to add required field; this function need an additional 
"optional alias" to avoid conflict.


- Klaus


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


On July 13, 2016, 1:45 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46298/
> ---
> 
> (Updated July 13, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Jie Yu.
> 
> 
> Bugs: MESOS-5123
> https://issues.apache.org/jira/browse/MESOS-5123
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rejected relative path agent work_dir.
> 
> 
> Diffs
> -
> 
>   src/slave/flags.cpp 84dbb2db41bd9849ab1245969884675d9d16555b 
> 
> Diff: https://reviews.apache.org/r/46298/diff/
> 
> 
> Testing
> ---
> 
> 1. make && make check
> 2. e2e test: 
>  
> ```
> $ ./src/mesos-slave --work_dir=aa --master=aa
> The required option `--work_dir` must be absolute path.
> ```
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 46298: Rejected relative path agent work_dir.

[Klaus Ma]

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

(Updated July 13, 2016, 1:45 p.m.)


Review request for mesos, Alexander Rukletsov and Jie Yu.


Changes
---

Using validate lamba to reject relative path.


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


Repository: mesos


Description
---

Rejected relative path agent work_dir.


Diffs (updated)
-

  src/slave/flags.cpp 84dbb2db41bd9849ab1245969884675d9d16555b 

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


Testing
---

1. make && make check
2. e2e test: 
 
```
$ ./src/mesos-slave --work_dir=aa --master=aa
The required option `--work_dir` must be absolute path.
```


Thanks,

Klaus Ma

Re: Review Request 49616: Add suppression benchmark.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp (lines 3668 - 3670)


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 3680 - 3681)


Ditto.


- Jiang Yan Xu


On July 8, 2016, 10:55 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49616/
> ---
> 
> (Updated July 8, 2016, 10:55 a.m.)
> 
> 
> Review request for mesos, James Peach, Joris Van Remoortere, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5781
> https://issues.apache.org/jira/browse/MESOS-5781
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Useful for high framework clusters which carry
>   many suppressed frameworks that are considered
>   during allocation.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/49616/diff/
> 
> 
> Testing
> ---
> 
> MESOS_BENCHMARK=1 GTEST_FILTER="*BENCHMARK_Test.SuppressOffers*" make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 49975: Clarified motivation and responsibilities of maintainers.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [49975]

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

Error:
2016-07-13 05:32:01 URL:https://reviews.apache.org/r/49975/diff/raw/ 
[3877/3877] -> "49975.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/14291/console

- Mesos ReviewBot


On July 13, 2016, 4:26 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49975/
> ---
> 
> (Updated July 13, 2016, 4:26 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> I will be following up with additional patches:
>   * A proposal for how one becomes a maintainer.
>   * A more comprehensive and finer-grained YAML listing of maintainers as a 
> top-level MAINTAINERS file.
> 
> The rendered markdown can be viewed here:
> https://gist.github.com/bmahler/b1bcbd0bc1d985028d51d663a1f0370f
> 
> A google-doc version for easier commenting can be found here:
> https://docs.google.com/document/d/1AMzNvvTJcpd6c0ilZtsq8cO-hPGxYs4Af4mrifj5IrA/edit?usp=sharing
> 
> 
> Diffs
> -
> 
>   docs/committers.md 4e8352c7f3fc58dff5b71962d8163d582e7369d7 
> 
> Diff: https://reviews.apache.org/r/49975/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

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

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


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

Re: Review Request 46298: Rejected relative path agent work_dir.

[Jie Yu]

> On July 11, 2016, 8:23 p.m., Jie Yu wrote:
> > src/slave/main.cpp, lines 175-179
> > 
> >
> > Can we do that check in `add` function. `add` function supports an 
> > optional validate lambda to be passed in.
> 
> Klaus Ma wrote:
> @jie, thanks for the comments. Try to use validated lamba today, but 
> there some issues:
> 
> 1. The `work_dir` is required without default value; if using validate 
> lamba, a "default" value is necessary: `add(T1* t1, name, alias, 
> defaultValue, lamba)`
> 2. The default value is pass by const reference currently, we can not 
> pass some empty value to it, e.g. nullptr or 0
> 
> For the default value, I'm thinking to replace const reference with 
> `Option`; any suggestion?

Oh, I don't realize that we don't have validation support for required field 
without default value.

Can we introduce another `add` variant in flags that support the above case? Is 
that difficult?


- Jie


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


On April 19, 2016, 7:31 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46298/
> ---
> 
> (Updated April 19, 2016, 7:31 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Jie Yu.
> 
> 
> Bugs: MESOS-5123
> https://issues.apache.org/jira/browse/MESOS-5123
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rejected relative path agent work_dir.
> 
> 
> Diffs
> -
> 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
> 
> Diff: https://reviews.apache.org/r/46298/diff/
> 
> 
> Testing
> ---
> 
> 1. make && make check
> 2. e2e test: 
>  
> ```
> $ ./src/mesos-slave --work_dir=aa --master=aa
> The required option `--work_dir` must be absolute path.
> ```
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Review Request 49975: Clarified motivation and responsibilities of maintainers.

[Benjamin Mahler]

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

Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.


Repository: mesos


Description
---

I will be following up with additional patches:
  * A proposal for how one becomes a maintainer.
  * A more comprehensive and finer-grained YAML listing of maintainers as a 
top-level MAINTAINERS file.

The rendered markdown can be viewed here:
https://gist.github.com/bmahler/b1bcbd0bc1d985028d51d663a1f0370f

A google-doc version for easier commenting can be found here:
https://docs.google.com/document/d/1AMzNvvTJcpd6c0ilZtsq8cO-hPGxYs4Af4mrifj5IrA/edit?usp=sharing


Diffs
-

  docs/committers.md 4e8352c7f3fc58dff5b71962d8163d582e7369d7 

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


Testing
---

N/A


Thanks,

Benjamin Mahler

Re: Review Request 49781: Code cleanup in hierarchical_allocator_tests.cpp.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49943, 49781]

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 13, 2016, 1:27 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49781/
> ---
> 
> (Updated July 13, 2016, 1:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5800
> https://issues.apache.org/jira/browse/MESOS-5800
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixed the following issues in benchmark test:
> 1) Enabled emplatized test fixture for DeclineOffers and
> ResourceLabels.
> 2) Updated the output for the benchmark to be more clear.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 153c9b4cf4819e976910c5a7ad9602028e2d22eb 
> 
> Diff: https://reviews.apache.org/r/49781/diff/
> 
> 
> Testing
> ---
> 
> LiuGuangyas-MacBook-Pro:build gyliu$ ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1”
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1
> Using 1000 agents and 50 frameworks
> round 0 allocate() took 2.318337secs to make 1000 offers
> round 1 allocate() took 2.409893secs to make 1000 offers
> round 2 allocate() took 2.321126secs to make 1000 offers
> round 3 allocate() took 2.395405secs to make 1000 offers
> round 4 allocate() took 2.374936secs to make 1000 offers
> round 5 allocate() took 2.440044secs to make 1000 offers
> round 6 allocate() took 2.439823secs to make 1000 offers
> round 7 allocate() took 2.487971secs to make 1000 offers
> round 8 allocate() took 2.481203secs to make 1000 offers
> round 9 allocate() took 2.527503secs to make 1000 offers
> round 10 allocate() took 2.541704secs to make 1000 offers
> round 11 allocate() took 2.6053secs to make 1000 offers
> round 12 allocate() took 2.734203secs to make 1000 offers
> …
> 
> LiuGuangyas-MacBook-Pro:build gyliu$ ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1”
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1
> Using 1000 agents and 50 frameworks
> round 0 allocate() took 2.557151secs to make 1000 offers
> round 1 allocate() took 2.538777secs to make 1000 offers
> round 2 allocate() took 2.492293secs to make 1000 offers
> round 3 allocate() took 2.515341secs to make 1000 offers
> round 4 allocate() took 2.629221secs to make 1000 offers
> round 5 allocate() took 2.701026secs to make 1000 offers
> round 6 allocate() took 2.596663secs to make 1000 offers
> round 7 allocate() took 2.696374secs to make 1000 offers
> …
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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

[Guangya Liu]

> On 七月 12, 2016, 4:32 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3196
> > 
> >
> > Two issues for this patch:
> > 
> > 1) The new added framework number will introduce some cases that agent 
> > number is less than framework number and some frameworks may not able to 
> > get resources.
> > 2) This will slow down the unit test, it is better introduce this and 
> > fix https://issues.apache.org/jira/browse/MESOS-4558 together by 
> > introducing `batchsize` for benchmark test.
> 
> Alexander Rukletsov wrote:
> 1) I think this a good thing to test / benchmark.
> 2) Benchmark tests are not part of the default test suite, hence it seems 
> fine if this test takes more time to run.

For 2), one minor comment is MESOS-4558 is tracking the time of benchmark tests 
because want to enable them on ASF CI, so it is better to reduce the time of 
the benchmark test.


- Guangya


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


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

Re: Review Request 49781: Code cleanup in hierarchical_allocator_tests.cpp.

[Guangya Liu]

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

(Updated 七月 13, 2016, 1:27 a.m.)


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


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


Repository: mesos


Description
---

This patch fixed the following issues in benchmark test:
1) Enabled emplatized test fixture for DeclineOffers and
ResourceLabels.
2) Updated the output for the benchmark to be more clear.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
153c9b4cf4819e976910c5a7ad9602028e2d22eb 

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


Testing (updated)
---

LiuGuangyas-MacBook-Pro:build gyliu$ ./bin/mesos-tests.sh --benchmark 
--gtest_filter="SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1”
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1
Using 1000 agents and 50 frameworks
round 0 allocate() took 2.318337secs to make 1000 offers
round 1 allocate() took 2.409893secs to make 1000 offers
round 2 allocate() took 2.321126secs to make 1000 offers
round 3 allocate() took 2.395405secs to make 1000 offers
round 4 allocate() took 2.374936secs to make 1000 offers
round 5 allocate() took 2.440044secs to make 1000 offers
round 6 allocate() took 2.439823secs to make 1000 offers
round 7 allocate() took 2.487971secs to make 1000 offers
round 8 allocate() took 2.481203secs to make 1000 offers
round 9 allocate() took 2.527503secs to make 1000 offers
round 10 allocate() took 2.541704secs to make 1000 offers
round 11 allocate() took 2.6053secs to make 1000 offers
round 12 allocate() took 2.734203secs to make 1000 offers
…

LiuGuangyas-MacBook-Pro:build gyliu$ ./bin/mesos-tests.sh --benchmark 
--gtest_filter="SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1”
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1
Using 1000 agents and 50 frameworks
round 0 allocate() took 2.557151secs to make 1000 offers
round 1 allocate() took 2.538777secs to make 1000 offers
round 2 allocate() took 2.492293secs to make 1000 offers
round 3 allocate() took 2.515341secs to make 1000 offers
round 4 allocate() took 2.629221secs to make 1000 offers
round 5 allocate() took 2.701026secs to make 1000 offers
round 6 allocate() took 2.596663secs to make 1000 offers
round 7 allocate() took 2.696374secs to make 1000 offers
…


Thanks,

Guangya Liu

Re: Review Request 49921: Fixed mesos tests to run 723 test on Unix.

[Srinivas Brahmaroutu]

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

(Updated July 13, 2016, 12:59 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
---

Fixed mesos tests to run 723 test on Unix.


Diffs (updated)
-

  src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 

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


Testing
---

cmake .. && make
src/tests/mesos-tests  (runs 723 tests with no failures)
I did not enable any module that has even a single failue. There are many more 
tests that are passing.


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49874: Added logrotate_container_logger for running mesos tests.

[Srinivas Brahmaroutu]

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

(Updated July 13, 2016, 12:38 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 logrotate_container_logger for running mesos tests.


Diffs (updated)
-

  src/slave/CMakeLists.txt d31440cb5e784d3e4f1236ac45001e80384f36f6 
  src/slave/cmake/SlaveConfigure.cmake ca4575653e00e8f868160976344ac1d57b5f7d27 

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


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

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 13, 2016, 12:38 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 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 49921: Fixed mesos tests to run 723 test on Unix.

[Srinivas Brahmaroutu]

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

(Updated July 13, 2016, 12:38 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
---

Fixed mesos tests to run 723 test on Unix.


Diffs (updated)
-

  src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 

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


Testing
---

cmake .. && make
src/tests/mesos-tests  (runs 723 tests with no failures)
I did not enable any module that has even a single failue. There are many more 
tests that are passing.


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49862: Changed libmesos from static library to a shared library.

[Srinivas Brahmaroutu]

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

(Updated July 13, 2016, 12:37 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
---

Changed libmesos from static library to a shared library.


Diffs (updated)
-

  3rdparty/cmake/Mesos3rdpartyConfigure.cmake 
eeb27860f6f95d297ccfe273ed76de5355b50ff8 
  3rdparty/http-parser/CMakeLists.txt.template 
7f7105f5d55fd66f3e826c1c37c0074775bf89ea 
  src/CMakeLists.txt 760519c5568d22f778980b25c3dfedc3c8ef83b1 
  src/master/cmake/MasterConfigure.cmake 
6bbd7e87273976f40527d719cc9450ff9a1d2ac7 
  src/slave/cmake/SlaveConfigure.cmake ca4575653e00e8f868160976344ac1d57b5f7d27 

Diff: https://reviews.apache.org/r/49862/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 13, 2016, 12:38 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 Test Modules that are loaded by mesos tests.


Diffs (updated)
-

  CMakeLists.txt ee8f565586b958929bc4e95b09827244d6d4155f 
  cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
  src/CMakeLists.txt 760519c5568d22f778980b25c3dfedc3c8ef83b1 
  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 for mesos tests.

[Srinivas Brahmaroutu]

> On July 12, 2016, 4:23 a.m., Alex Clemmer wrote:
> > src/tests/CMakeLists.txt, line 156
> > 
> >
> > Also, I think this is not the right command to run for Mesos tests -- 
> > can you confirm? The same goes for the containerizer memory tests. Running 
> > `make test` fails on my OS X 10.10 machine, with the following error:
> > 
> > ```
> > /usr/local/Cellar/cmake/3.4.0/bin/ctest --force-new-ctest-process
> > Test project /Users/alex/src/mesos/build
> > Start 1: StoutTests
> > 1/4 Test #1: StoutTests ...   Passed0.45 sec
> > Start 2: ProcessTests
> > 2/4 Test #2: ProcessTests .   Passed1.74 sec
> > Start 3: MesosTests
> > 3/4 Test #3: MesosTests ...***Failed0.31 sec
> > Usage: /Users/alex/src/mesos/build/src/tests/mesos-tests  > username>
> > 
> > Start 4: MesosContainerizerMemoryTests
> > 4/4 Test #4: MesosContainerizerMemoryTests ***Failed0.05 sec
> > Usage: 
> > /Users/alex/src/mesos/build/src/tests/containerizer/mesos-containerizer-memory_test
> >   [OPTIONS]
> > 
> > Available subcommands:
> > help
> > MemoryTestHelperMain
> > 
> > 
> > 
> > 50% tests passed, 2 tests failed out of 4
> > 
> > Total Test time (real) =   2.56 sec
> > 
> > The following tests FAILED:
> >   3 - MesosTests (Failed)
> >   4 - MesosContainerizerMemoryTests (Failed)
> > Errors while running CTest
> > ```
> > 
> > We will need to fix this before we check in.

Intention is not to run any tests in this patch. I require other dependencies 
from examples, etc to run the code without changin source code. What we are 
building is a dummy executable with only utility classes and the 'main' coming 
from 'active_user_test_helper.cpp'. That patched are lined up to make a 
meaningful 'mesos-tests' in the end. If you think we do it the other way, I 
will submit individual patches for examples modules first and then send in this 
patch.


- Srinivas


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


On July 12, 2016, 4:26 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49688/
> ---
> 
> (Updated July 12, 2016, 4:26 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 for mesos tests.
> 
> 
> Diffs
> -
> 
>   src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 
>   src/tests/cmake/TestsConfigure.cmake PRE-CREATION 
>   src/tests/containerizer/CMakeLists.txt 
> dab8cb07f09f5554123ede4ec8b45b60abf62eee 
> 
> 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]

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

(Updated July 13, 2016, 12:37 a.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


Summary (updated)
-

Added cmake build variables for mesos tests.


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


Repository: mesos


Description (updated)
---

Added cmake build variables for mesos tests.


Diffs (updated)
-

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

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


Testing
---

cmake ..
cmake check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49926: Added Windows build batch script.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49926]

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 12, 2016, 11:16 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49926/
> ---
> 
> (Updated July 12, 2016, 11:16 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5822
> https://issues.apache.org/jira/browse/MESOS-5822
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a script that will be run on the ASF CI, similar to how the
> `docker_build.sh` script is used for the existing Linux-based CI.
> 
> This Jenkins job for Mesos on Windows can be found here:
> https://builds.apache.org/job/Mesos-Windows/
> 
> 
> Diffs
> -
> 
>   support/windows-build.bat PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49926/diff/
> 
> 
> Testing
> ---
> 
> Pointed the Jenkins job at my local branch and triggered the build.
> 
> This is what Jenkins is running:
> ```
> :: Run a script provided by Visual Studio which sets up a
> :: couple environment variables needed by the build.
> CALL "F:/Microsoft/Visual Studio CE 2015/VC/vcvarsall.bat"
> 
> @echo on
> 
> :: Recreate the `/tmp` directory needed by tests.
> RMDIR /q /s %CD:~0,3%tmp
> MKDIR %CD:~0,3%tmp
> if %errorlevel% neq 0 exit /b %errorlevel%
> 
> :: We need to specify the path to GNU Patch,
> :: since it is installed in a non-default location.
> SET OTHER_CMAKE_OPTIONS=-DPATCHEXE_PATH="F:/Program Files (x86)/GnuWin32/bin"
> 
> :: Call the Windows build script.
> "support/windows-build.bat"
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 49926: Added Windows build batch script.

[Joseph Wu]

> On July 12, 2016, 2:40 p.m., Alex Clemmer wrote:
> > support/windows-build.bat, lines 32-33
> > 
> >
> > I'm not super familiar with Jenkins, but I wonder why we create `tmp` 
> > in the Jenkins script, but create `build` here? It seems like we'd want to 
> > create the `tmp` file in this script, and leave the Jenkins script to do 
> > only the configuration stuff, like getting patch.exe? What do you think?

I left everything specific to the ASF machine in the Jenkins script.  (i.e. 
things that have absolute paths)

The `tmp` folder may or may not exist on someone's machine; and maybe it holds 
some non-temporary stuff (after all, `/tmp` is not a temporary folder on 
Windows :D ).  I didn't want this build script to unilaterally create or delete 
things outside of the Mesos directory.

---

This reminded me to add a check for the root directory.  It doesn't make sense 
to run this script in any other location.


> On July 12, 2016, 2:40 p.m., Alex Clemmer wrote:
> > support/windows-build.bat, line 34
> > 
> >
> > Do you need to check the error code for `MKDIR` too? I'm not super up 
> > on my batch semantics.

I left it out, because the folder might already exist.  Although, I think MKDIR 
in windows does not error if the folder already exists.  (Either way, no need 
to check if `CD` will check for us.)


> On July 12, 2016, 2:40 p.m., Alex Clemmer wrote:
> > support/windows-build.bat, line 40
> > 
> >
> > Interesting, why does Jenkins pass in `PATCHEXE_PATH` but not 
> > `ENABLE_LIBEVENT`?

On Windows, libevent is required (you can't `cmake ..` without it).  But the 
GNU Patch path is specific to the Jenkins box.


> On July 12, 2016, 2:40 p.m., Alex Clemmer wrote:
> > support/windows-build.bat, line 44
> > 
> >
> > Heh. Do we want to actually explain that this is a bug? This comment 
> > seems more like an inside joke right now. :)

I'm not clear whether this is a bug or not.  We definitely want to explain why 
we're passing the option though.

And AFAICT, specifying the `PreferredToolArchitecture` only changes the build 
time (by a lot).  I don't think it changes the behavior of the executables.


> On July 12, 2016, 2:40 p.m., Alex Clemmer wrote:
> > support/windows-build.bat, line 1
> > 
> >
> > Although we use this commend syntax in other batch scripts, we should 
> > avoid the LABEL syntax `::` and instead use `REM`.
> > 
> > The latter is a standards-compliant, well-documented way of adding 
> > comments, while the former is not a commenting scheme at all. And because 
> > it is not a commenting scheme, there are many rules about where you can and 
> > can't place them (for example, you can't put any whitespace before the `::` 
> > symbols, and they can't appear in a `for` block), and if you disregard 
> > these things, you will get weird and hard-to-diagnose errors (for example, 
> > it might cause you to accidentally ignore parts of lexical blocks or output 
> > errors like `The system cannot find the drive specified` for seemingly no 
> > reason).
> > 
> > For more information, see the bulleted list at the end of this post: 
> > http://www.robvanderwoude.com/comments.php

I'll make the change, but it's a lot less pretty/readable though.  I almost 
feel like I need to add a comment, explaining that `REM` lines are comments :D

And it shows up when you have `@echo on`.


- Joseph


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


On July 12, 2016, 4:16 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49926/
> ---
> 
> (Updated July 12, 2016, 4:16 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5822
> https://issues.apache.org/jira/browse/MESOS-5822
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a script that will be run on the ASF CI, similar to how the
> `docker_build.sh` script is used for the existing Linux-based CI.
> 
> This Jenkins job for Mesos on Windows can be found here:
> https://builds.apache.org/job/Mesos-Windows/
> 
> 
> Diffs
> -
> 
>   support/windows-build.bat PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49926/diff/
> 
> 
> Testing
> ---
> 
> Pointed the Jenkins job at my local branch and 

Re: Review Request 49926: Added Windows build batch script.

[Joseph Wu]

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

(Updated July 12, 2016, 4:16 p.m.)


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


Changes
---

Change comment syntax.  Add check for running the script in a specific location.


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


Repository: mesos


Description
---

This adds a script that will be run on the ASF CI, similar to how the
`docker_build.sh` script is used for the existing Linux-based CI.

This Jenkins job for Mesos on Windows can be found here:
https://builds.apache.org/job/Mesos-Windows/


Diffs (updated)
-

  support/windows-build.bat PRE-CREATION 

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


Testing
---

Pointed the Jenkins job at my local branch and triggered the build.

This is what Jenkins is running:
```
:: Run a script provided by Visual Studio which sets up a
:: couple environment variables needed by the build.
CALL "F:/Microsoft/Visual Studio CE 2015/VC/vcvarsall.bat"

@echo on

:: Recreate the `/tmp` directory needed by tests.
RMDIR /q /s %CD:~0,3%tmp
MKDIR %CD:~0,3%tmp
if %errorlevel% neq 0 exit /b %errorlevel%

:: We need to specify the path to GNU Patch,
:: since it is installed in a non-default location.
SET OTHER_CMAKE_OPTIONS=-DPATCHEXE_PATH="F:/Program Files (x86)/GnuWin32/bin"

:: Call the Windows build script.
"support/windows-build.bat"
```


Thanks,

Joseph Wu

Re: Review Request 49967: Added a capability to opt-in to receive shared resources in offers.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On July 12, 2016, 3:55 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49967/
> ---
> 
> (Updated July 12, 2016, 3:55 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4325
> https://issues.apache.org/jira/browse/MESOS-4325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Note that this just adds the capability. The functionality for shared
> resources would be added in future commits.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 40d4c3b293edf24171db8703bfe404e880988e56 
>   include/mesos/v1/mesos.proto 57d94bc8dbf3a009c327cca2cc73d752776a0101 
> 
> Diff: https://reviews.apache.org/r/49967/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 49960: Added private registry with auth document and fixed correspondings.

[Jie Yu]

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


Fix it, then Ship it!





docs/container-image.md (lines 191 - 192)


Per container credential is not supported yet (coming soon).



docs/container-image.md (line 218)


will be ignore.


- Jie Yu


On July 12, 2016, 4:53 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49960/
> ---
> 
> (Updated July 12, 2016, 4:53 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-5716
> https://issues.apache.org/jira/browse/MESOS-5716
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added private registry with auth document and fixed correspondings.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c7ec3c516882130a35d66c5e2390ade04046ac70 
>   docs/container-image.md 6d180e4044a484d5b9f389500abc17f33b200eaa 
>   docs/docker-containerizer.md 7a7cb950dd5a386ad037c6fb6c53817774c11a68 
>   src/slave/flags.cpp 84dbb2db41bd9849ab1245969884675d9d16555b 
> 
> Diff: https://reviews.apache.org/r/49960/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

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

[Anindya Sinha]

> On July 12, 2016, 9:04 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, line 292
> > 
> >
> > s/shareable/shared/ here and elsewhere in comments.

Fixed in https://reviews.apache.org/r/49967/.


- Anindya


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


On July 1, 2016, 10:29 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45966/
> ---
> 
> (Updated July 1, 2016, 10:29 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 SHAREABLE_RESOURCES that frameworks need to opt
> in if they are interested in receiving shared resources in their
> offers.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
>   include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
>   src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
>   src/master/allocator/mesos/hierarchical.hpp 
> b72ba16277a3210e4d309b616d185a10e2029a66 
>   src/master/allocator/mesos/hierarchical.cpp 
> eca949e07abb00423a2f35a56eedc5d4287d81f3 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/45966/diff/
> 
> 
> Testing
> ---
> 
> Tests updated with new capability.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Review Request 49967: Added a capability to opt-in to receive shared resources in offers.

[Anindya Sinha]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Note that this just adds the capability. The functionality for shared
resources would be added in future commits.


Diffs
-

  include/mesos/mesos.proto 40d4c3b293edf24171db8703bfe404e880988e56 
  include/mesos/v1/mesos.proto 57d94bc8dbf3a009c327cca2cc73d752776a0101 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 49958: Fixed Docker daemon args generation.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 12, 2016, 5:04 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49958/
> ---
> 
> (Updated July 12, 2016, 5:04 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5834
> https://issues.apache.org/jira/browse/MESOS-5834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix prevents the Docker containerizer from specifying
> --volume_driver multiple times.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp aad5380f5848f1b49d5524032f3b5b18c2aa1c56 
>   src/tests/containerizer/docker_tests.cpp 
> 00ccc2fc54a984f3a67573ce6fe7aee3e60ce3a2 
> 
> Diff: https://reviews.apache.org/r/49958/diff/
> 
> 
> Testing
> ---
> 
> Added two new tests and they pass.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 49921: Fixed mesos tests to run 723 test on Unix.

[Srinivas Brahmaroutu]

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

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


Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Fixed mesos tests to run 723 test on Unix.


Diffs (updated)
-

  src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 

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


Testing
---

cmake .. && make
src/tests/mesos-tests  (runs 723 tests with no failures)
I did not enable any module that has even a single failue. There are many more 
tests that are passing.


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49874: Added logrotate_container_logger for running mesos tests.

[Srinivas Brahmaroutu]

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

(Updated July 12, 2016, 9:55 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 logrotate_container_logger for running mesos tests.


Diffs (updated)
-

  src/slave/CMakeLists.txt d31440cb5e784d3e4f1236ac45001e80384f36f6 
  src/slave/cmake/SlaveConfigure.cmake ca4575653e00e8f868160976344ac1d57b5f7d27 

Diff: https://reviews.apache.org/r/49874/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 12, 2016, 9:55 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 760519c5568d22f778980b25c3dfedc3c8ef83b1 
  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 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 12, 2016, 9:55 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 49926: Added Windows build batch script.

[Alex Clemmer]

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




support/windows-build.bat (line 1)


Although we use this commend syntax in other batch scripts, we should avoid 
the LABEL syntax `::` and instead use `REM`.

The latter is a standards-compliant, well-documented way of adding 
comments, while the former is not a commenting scheme at all. And because it is 
not a commenting scheme, there are many rules about where you can and can't 
place them (for example, you can't put any whitespace before the `::` symbols, 
and they can't appear in a `for` block), and if you disregard these things, you 
will get weird and hard-to-diagnose errors (for example, it might cause you to 
accidentally ignore parts of lexical blocks or output errors like `The system 
cannot find the drive specified` for seemingly no reason).

For more information, see the bulleted list at the end of this post: 
http://www.robvanderwoude.com/comments.php



support/windows-build.bat (lines 32 - 33)


I'm not super familiar with Jenkins, but I wonder why we create `tmp` in 
the Jenkins script, but create `build` here? It seems like we'd want to create 
the `tmp` file in this script, and leave the Jenkins script to do only the 
configuration stuff, like getting patch.exe? What do you think?



support/windows-build.bat (line 34)


Do you need to check the error code for `MKDIR` too? I'm not super up on my 
batch semantics.



support/windows-build.bat (line 40)


Interesting, why does Jenkins pass in `PATCHEXE_PATH` but not 
`ENABLE_LIBEVENT`?



support/windows-build.bat (line 44)


Heh. Do we want to actually explain that this is a bug? This comment seems 
more like an inside joke right now. :)


- Alex Clemmer


On July 12, 2016, 1:11 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49926/
> ---
> 
> (Updated July 12, 2016, 1:11 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5822
> https://issues.apache.org/jira/browse/MESOS-5822
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a script that will be run on the ASF CI, similar to how the
> `docker_build.sh` script is used for the existing Linux-based CI.
> 
> This Jenkins job for Mesos on Windows can be found here:
> https://builds.apache.org/job/Mesos-Windows/
> 
> 
> Diffs
> -
> 
>   support/windows-build.bat PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49926/diff/
> 
> 
> Testing
> ---
> 
> Pointed the Jenkins job at my local branch and triggered the build.
> 
> This is what Jenkins is running:
> ```
> :: Run a script provided by Visual Studio which sets up a
> :: couple environment variables needed by the build.
> CALL "F:/Microsoft/Visual Studio CE 2015/VC/vcvarsall.bat"
> 
> @echo on
> 
> :: Recreate the `/tmp` directory needed by tests.
> RMDIR /q /s %CD:~0,3%tmp
> MKDIR %CD:~0,3%tmp
> if %errorlevel% neq 0 exit /b %errorlevel%
> 
> :: We need to specify the path to GNU Patch,
> :: since it is installed in a non-default location.
> SET OTHER_CMAKE_OPTIONS=-DPATCHEXE_PATH="F:/Program Files (x86)/GnuWin32/bin"
> 
> :: Call the Windows build script.
> "support/windows-build.bat"
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 49958: Fixed Docker daemon args generation.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49958]

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 12, 2016, 5:04 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49958/
> ---
> 
> (Updated July 12, 2016, 5:04 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5834
> https://issues.apache.org/jira/browse/MESOS-5834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix prevents the Docker containerizer from specifying
> --volume_driver multiple times.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp aad5380f5848f1b49d5524032f3b5b18c2aa1c56 
>   src/tests/containerizer/docker_tests.cpp 
> 00ccc2fc54a984f3a67573ce6fe7aee3e60ce3a2 
> 
> Diff: https://reviews.apache.org/r/49958/diff/
> 
> 
> Testing
> ---
> 
> Added two new tests and they pass.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 49958: Fixed Docker daemon args generation.

[Gastón Kleiman]

> On July 12, 2016, 8:53 p.m., Gilbert Song wrote:
> > src/docker/docker.cpp, lines 620-631
> > 
> >
> > Should we consider just deprecate `DockerInfo.volume_driver` in this 
> > patch? Since 1.0 will be released these couple days.

It is already deprecated, see: 
https://github.com/apache/mesos/blob/master/include/mesos/mesos.proto#L1839


- Gastón


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


On July 12, 2016, 5:04 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49958/
> ---
> 
> (Updated July 12, 2016, 5:04 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5834
> https://issues.apache.org/jira/browse/MESOS-5834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix prevents the Docker containerizer from specifying
> --volume_driver multiple times.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp aad5380f5848f1b49d5524032f3b5b18c2aa1c56 
>   src/tests/containerizer/docker_tests.cpp 
> 00ccc2fc54a984f3a67573ce6fe7aee3e60ce3a2 
> 
> Diff: https://reviews.apache.org/r/49958/diff/
> 
> 
> Testing
> ---
> 
> Added two new tests and they pass.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

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

[Jiang Yan Xu]

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



LGTM but let's commit the protos first, could you separate them out into their 
own review? In the proto field let's add a TODO:

```
// TODO(anisinha): This is currently a no-op.
```

We can remove the TODO when the chain is committed.


include/mesos/mesos.proto (line 292)


s/shareable/shared/ here and elsewhere in comments.



include/mesos/mesos.proto (line 293)


s/SHAREABLE_RESOURCES/SHARED_RESOURCES/



include/mesos/v1/mesos.proto (lines 292 - 293)


Ditto.


- Jiang Yan Xu


On July 1, 2016, 3:29 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45966/
> ---
> 
> (Updated July 1, 2016, 3:29 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 SHAREABLE_RESOURCES that frameworks need to opt
> in if they are interested in receiving shared resources in their
> offers.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
>   include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
>   src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
>   src/master/allocator/mesos/hierarchical.hpp 
> b72ba16277a3210e4d309b616d185a10e2029a66 
>   src/master/allocator/mesos/hierarchical.cpp 
> eca949e07abb00423a2f35a56eedc5d4287d81f3 
>   src/tests/hierarchical_allocator_tests.cpp 
> 0498cd5e54b0e4b87a767585a77699653aa52179 
> 
> Diff: https://reviews.apache.org/r/45966/diff/
> 
> 
> Testing
> ---
> 
> Tests updated with new capability.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 49958: Fixed Docker daemon args generation.

[Gilbert Song]

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



The patch LGTM! Thanks!

Will come back with a Ship it.


src/docker/docker.cpp (lines 620 - 631)


Should we consider just deprecate `DockerInfo.volume_driver` in this patch? 
Since 1.0 will be released these couple days.



src/tests/containerizer/docker_tests.cpp (lines 843 - 875)


We dont need this test since `volume_driver` in DockerInfo will be 
deprecated soon.


- Gilbert Song


On July 12, 2016, 10:04 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49958/
> ---
> 
> (Updated July 12, 2016, 10:04 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5834
> https://issues.apache.org/jira/browse/MESOS-5834
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix prevents the Docker containerizer from specifying
> --volume_driver multiple times.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp aad5380f5848f1b49d5524032f3b5b18c2aa1c56 
>   src/tests/containerizer/docker_tests.cpp 
> 00ccc2fc54a984f3a67573ce6fe7aee3e60ce3a2 
> 
> Diff: https://reviews.apache.org/r/49958/diff/
> 
> 
> Testing
> ---
> 
> Added two new tests and they pass.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

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

[Anindya Sinha]

> On July 12, 2016, 5:29 p.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp, line 4803
> > 
> >
> > "or no target resources are present": We are inside the 
> > 
> > ```
> > if (resourcesState.get().target.isSome()) {
> > }
> > ```
> > 
> > block, so we are certain that the target exists right? 
> > 
> > ```
> > CHECK(os::exists(paths::getResourcesTargetPath(metaDir)));
> > ```
> > 
> > instead?

I fixed the comment but did not add the `CHECK()` since although it should 
never happen, crashing the agent does not seem necessary especially because we 
do a `LOG(ERROR)` if `os::rm()` fails on the target resources file.


- Anindya


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


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

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

[Anindya Sinha]

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

(Updated July 12, 2016, 8:25 p.m.)


Review request for mesos, Neil Conway and Jiang Yan Xu.


Changes
---

Cosmetic changes based on review comments.


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


Repository: mesos


Description (updated)
---

When the agent receives CheckpointedResourcesMessage, we store the
target checkpoint on disk. On successful create and destroy of
persistent volumes as a part of handling this messages, we commit
the checkpoint on the disk, and clear the target checkpoint.

However, incase of any failure we do not commit the checkpoint to
disk, and exit the agent. When the agent restarts and there is a
target checkpoint present on disk which differs from the committed
checkpoint, we retry to sync the target and committed checkpoint.
On success, we reregister the agent with the master, but in case it
fails, we do not commit the checkpoint and the agent exists.


Diffs (updated)
-

  src/slave/paths.hpp 339e539863c678b6ed4d4670d75c7ff4c54daa79 
  src/slave/paths.cpp 03157f93b1e703006f95ef6d0a30afae375dcdb5 
  src/slave/slave.hpp 42afa9e2ebe5cf8e35802c8d169f52879d6073ac 
  src/slave/slave.cpp 02982d542c9e6b5b5f7fc8b3c73db6f5bac01358 
  src/slave/state.hpp 0de2a4ee4fabaad612c4526166157b001c380bdb 
  src/slave/state.cpp 9cec0868b1187ed3ccac7f065e8a21c2f52178d9 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49960, 49961]

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

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/#review141951
---



This patch looks good to me. Will give a ship it once the comments are 
addressed.


src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 202 - 203)


s/ignored/overwritten/g



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 260 - 261)


could you use `const appc::spec::ImageManifest::App app = ...` here?


- Gilbert Song


On July 12, 2016, 9:15 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49348/
> ---
> 
> (Updated July 12, 2016, 9:15 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 49348: Added implementation to Appc Runtime Isolator.

[Gilbert Song]

> On July 6, 2016, 11:38 p.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?

We dont need to do anything special to line 2. Could you verify? I can be wrong.


- Gilbert


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


On July 12, 2016, 9:15 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49348/
> ---
> 
> (Updated July 12, 2016, 9:15 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 49864: Fixed ExecutorPIDTest.

[Haris Choudhary]

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

(Updated July 12, 2016, 7:05 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Fixed ExecutorPIDTest.


Diffs (updated)
-

  src/tests/containerizer/mesos_containerizer_tests.cpp 
57588cc1fb918924a163bdb40b195cc5f4231f6e 

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


Testing
---

Make and make check


Thanks,

Haris Choudhary

Re: Review Request 49689: Added Appc runtime isolator tests.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: [49689]

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

- Mesos ReviewBot


On July 12, 2016, 4:15 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49689/
> ---
> 
> (Updated July 12, 2016, 4:15 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 Appc runtime isolator tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 
>   src/tests/containerizer/appc_archive.hpp PRE-CREATION 
>   src/tests/containerizer/appc_runtime_isolator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49689/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

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

[Gilbert Song]

> On July 6, 2016, 11:38 p.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.

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?


- Gilbert


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


On July 12, 2016, 9:15 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49348/
> ---
> 
> (Updated July 12, 2016, 9:15 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 49232: Added appcManifest to ImageIt nfo and ProvisionInfo.

[Gilbert Song]

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




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


why do you change it as a Future?



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


As mentioned before, this var is not needed.



src/slave/containerizer/mesos/provisioner/appc/store.cpp (lines 212 - 221)


You just need to check isError here.



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


no need this extra var `manifest`. You can just have _manifest.get() here.



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


As mentioned before, please fix the indentation. Thanks.


- Gilbert Song


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

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49955]

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 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 49914: Improved the speed of 'MasterAPITest.UnreserveResources'.

[Abhishek Dasgupta]

> On July 12, 2016, 12:35 p.m., Neil Conway wrote:
> > Seems like this review contains a bunch of changes, in addition to lowering 
> > the `allocation_interval`. Are these changes related? Can you move the 
> > unrelated changes to a separate review, and/or explain why you have made 
> > them?

Actually, all of the changes were needed for speeding up the test. Still after 
lowering the allocation_interval, UnreserveResource Test was slow and it 
speeded up after removing unnecessary tests for varifying if the resources were 
unreserved. Do you still think it would be good to make these two changes in 
separate patches?


- Abhishek


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


On July 11, 2016, 7:39 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49914/
> ---
> 
> (Updated July 11, 2016, 7:39 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-5732
> https://issues.apache.org/jira/browse/MESOS-5732
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Overloaded createMasterFlags in MasterAPITest to increase
> the speed of 'MasterAPITest.ReserveResources' and
> 'MasterAPITest.UnreserveResources'.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49914/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

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

[Abhishek Dasgupta]

> On July 12, 2016, 2:24 a.m., Jay Guo wrote:
> > src/tests/mesos.hpp, line 582
> > 
> >
> > why `role1` but not `role`?

There are three reasons for this:

1. Renaming to role instead of role1 needs more changes in the codebases.
2. In persistant_volume tests, it is testing against JSON for "role : role1", 
here it looks a little qeer if used "role : role".
3. In future, if we need to add more roles, it would be rational to add as 
"role2", "role3" .. So, this one is named as role1.

What do you think?


- Abhishek


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


On July 11, 2016, 7:12 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49913/
> ---
> 
> (Updated July 11, 2016, 7:12 p.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
> ---
> 
> Clean up code to define createFrameworkInfo() once in
> tests/mesos.hpp and use that in various other inherited
> tests.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
>   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 49781: Code cleanup in hierarchical_allocator_tests.cpp.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp (lines 3481 - 3483)


Can we keep the "round #" info in this test? 

In the new SuppressOffers test we use the number of suppressed frameworks 
to distinguish the rounds explicitly but for this test we only have the round # 
(until we determine how to refactor this test). When there are thousands of 
lines printed out the round # helps us infer what the cluster state a 
particular line corresponds to (which is very implicit but for the clean up 
let's keep it this way).



src/tests/hierarchical_allocator_tests.cpp (lines 3632 - 3634)


Ditto.


- Jiang Yan Xu


On July 11, 2016, 7:01 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49781/
> ---
> 
> (Updated July 11, 2016, 7:01 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5800
> https://issues.apache.org/jira/browse/MESOS-5800
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixed the following issues in benchmark test:
> 1) Enabled emplatized test fixture for DeclineOffers and
> ResourceLabels.
> 2) Updated the output for the benchmark to be more clear.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 153c9b4cf4819e976910c5a7ad9602028e2d22eb 
> 
> Diff: https://reviews.apache.org/r/49781/diff/
> 
> 
> Testing
> ---
> 
> LiuGuangyas-MacBook-Pro:build gyliu$ ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1”
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1
> Using 1000 agents and 50 frameworks
>  allocate() took 625869us to make 1000 offers
>  allocate() took 628127us to make 1000 offers
>  allocate() took 618838us to make 1000 offers
>  allocate() took 595152us to make 1000 offers
>  allocate() took 622273us to make 1000 offers
>  allocate() took 595577us to make 1000 offers
>  allocate() took 628503us to make 1000 offers
>  allocate() took 623374us to make 1000 offers
> ….
> 
> LiuGuangyas-MacBook-Pro:build gyliu$ ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1”
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1
> Using 1000 agents and 50 frameworks
>  allocate() took 670083us to make 1000 offers
>  allocate() took 665393us to make 1000 offers
>  allocate() took 635591us to make 1000 offers
>  allocate() took 691869us to make 1000 offers
> ….
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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

[Neil Conway]

---
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.


Changes
---

Update changelog, upgrade guide.


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 (updated)
-

  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 49957: Fixed typos in the docs.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On July 12, 2016, 4:23 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49957/
> ---
> 
> (Updated July 12, 2016, 4:23 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed typos in the docs.
> 
> 
> Diffs
> -
> 
>   docs/agent-recovery.md 6d1b402ae5a3e26832d4fa665ca3666d4c096bc2 
>   docs/attributes-resources.md 98fabfb67cf78466bf1ef8d37ebab3e987742f8e 
>   docs/configuration.md 526308a803307da48928f2cf663dfea5deb4b3a1 
>   docs/high-availability.md e467a0e1d592fa1aa8d8746f59cb753f28fc30e2 
>   docs/mesos-containerizer.md 94144c7dfeba6ecea2271cf8b78abb8dc5dfa81e 
>   docs/monitoring.md dff337d890ed68a3f615c122a940d64cf14ee1e5 
>   docs/port-mapping-isolator.md 6a6bab9df3f6f3960ceeab38b6302823dceae9c2 
>   docs/testing-patterns.md df11e91f2e373774fad999b0cd3fe1db9a4671de 
> 
> Diff: https://reviews.apache.org/r/49957/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49957: Fixed typos in the docs.

[Alexander Rukletsov]

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



It seems unfortunate that we use huge lines; they are hard to diff.


docs/agent-recovery.md (line 61)


Not yours: extra space before "Therefore"


- Alexander Rukletsov


On July 12, 2016, 4:23 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49957/
> ---
> 
> (Updated July 12, 2016, 4:23 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed typos in the docs.
> 
> 
> Diffs
> -
> 
>   docs/agent-recovery.md 6d1b402ae5a3e26832d4fa665ca3666d4c096bc2 
>   docs/attributes-resources.md 98fabfb67cf78466bf1ef8d37ebab3e987742f8e 
>   docs/configuration.md 526308a803307da48928f2cf663dfea5deb4b3a1 
>   docs/high-availability.md e467a0e1d592fa1aa8d8746f59cb753f28fc30e2 
>   docs/mesos-containerizer.md 94144c7dfeba6ecea2271cf8b78abb8dc5dfa81e 
>   docs/monitoring.md dff337d890ed68a3f615c122a940d64cf14ee1e5 
>   docs/port-mapping-isolator.md 6a6bab9df3f6f3960ceeab38b6302823dceae9c2 
>   docs/testing-patterns.md df11e91f2e373774fad999b0cd3fe1db9a4671de 
> 
> Diff: https://reviews.apache.org/r/49957/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/slave/paths.hpp (lines 63 - 68)


Missing some vertical lines.

```
//   |-- meta
//   |   |-- boot_id
//   |   |-- resources
//   |   |   |-- resources.info
//   |   |   |-- resources.target
//   |   |-- slaves
```



src/slave/slave.cpp (line 2524)


I think Neil was suggesting s/any of operations/any of the operations/.

'all operations are successful' reads fine. (And you have 'after all 
operations are successful' 4 lines down).



src/slave/slave.cpp (lines 2594 - 2595)


Two space indentation per the style guide.



src/slave/slave.cpp (line 4785)


s/resources are/resources that are/



src/slave/slave.cpp (line 4787)


"or no target resources are present": We are inside the 

```
if (resourcesState.get().target.isSome()) {
}
```

block, so we are certain that the target exists right? 

```
CHECK(os::exists(paths::getResourcesTargetPath(metaDir)));
```

instead?



src/slave/state.cpp (lines 732 - 734)


Fix indentation.



src/slave/state.cpp (lines 804 - 805)


Use one blank line here.


- Jiang Yan Xu


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

Review Request 49958: Fixed Docker daemon args generation.

[Gastón Kleiman]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

This fix prevents the Docker containerizer from specifying
--volume_driver multiple times.


Diffs (updated)
-

  src/docker/docker.cpp aad5380f5848f1b49d5524032f3b5b18c2aa1c56 
  src/tests/containerizer/docker_tests.cpp 
00ccc2fc54a984f3a67573ce6fe7aee3e60ce3a2 

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


Testing
---

Added two new tests and they pass.


Thanks,

Gastón Kleiman

Review Request 49960: Added private registry with auth document and fixed correspondings.

[Gilbert Song]

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

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


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


Repository: mesos


Description
---

Added private registry with auth document and fixed correspondings.


Diffs
-

  docs/configuration.md c7ec3c516882130a35d66c5e2390ade04046ac70 
  docs/container-image.md 6d180e4044a484d5b9f389500abc17f33b200eaa 
  docs/docker-containerizer.md 7a7cb950dd5a386ad037c6fb6c53817774c11a68 
  src/slave/flags.cpp 84dbb2db41bd9849ab1245969884675d9d16555b 

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


Testing
---

make check


Thanks,

Gilbert Song

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

[Gilbert Song]

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

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


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


Repository: mesos


Description
---

Supported relative container path in docker containerizer.


Diffs
-

  src/docker/docker.cpp aad5380f5848f1b49d5524032f3b5b18c2aa1c56 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 49736: Added PATCH_CMD in 3rdparty/CMakeLists.txt for ELFIO.

[Alex Clemmer]

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




3rdparty/CMakeLists.txt (line 141)


This probably won't work on Windows. You will notice that for (_e.g._) 
`HTTP_PARSER_PATCH_CMD`, we have something like this instead of `PATCH_CMD`:

```
  set(HTTP_PARSER_PATCH_CMD ${PATCHEXE_LOCATION} --binary -p1 < 
${MESOS_3RDPARTY_BIN}/http-parser-${HTTP_PARSER_VERSION}.patch)
```

This is pretty unfortuante and as I look at this file, it seems like we're 
not patching a lot of the libraries. This is probably my doing. #clowntown I've 
filed an issue to make sure we follow up on this: MESOS-5835


- Alex Clemmer


On July 6, 2016, 11:58 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49736/
> ---
> 
> (Updated July 6, 2016, 11:58 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added PATCH_CMD in 3rdparty/CMakeLists.txt for ELFIO.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt a7ae76a40fcf5c6d6c9dc86daaa7524c220980dd 
> 
> Diff: https://reviews.apache.org/r/49736/diff/
> 
> 
> Testing
> ---
> 
> $ cmake ..
> $ make -j
> $ make -j check
> 
> Manually inspected: `3rdparty/elfio-3.1/src/elfio-3.1/elfio/elfio_note.hpp`
> and saw that the patch is applied.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 43138: Updated a comment for oversubscribed resources for clarity.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43105, 44331, 43138]

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 12, 2016, 2:31 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43138/
> ---
> 
> (Updated July 12, 2016, 2:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 7d4064535a20b93950f5a95eef1ad3f0d37d305b 
> 
> Diff: https://reviews.apache.org/r/43138/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 49862: Changed libmesos from static library to a shared library.

[Srinivas Brahmaroutu]

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

(Updated July 12, 2016, 4:30 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
---

Changed libmesos from static library to a shared library.


Diffs (updated)
-

  3rdparty/cmake/Mesos3rdpartyConfigure.cmake 
eeb27860f6f95d297ccfe273ed76de5355b50ff8 
  3rdparty/http-parser/CMakeLists.txt.template 
7f7105f5d55fd66f3e826c1c37c0074775bf89ea 
  src/CMakeLists.txt 760519c5568d22f778980b25c3dfedc3c8ef83b1 
  src/master/cmake/MasterConfigure.cmake 
6bbd7e87273976f40527d719cc9450ff9a1d2ac7 
  src/slave/cmake/SlaveConfigure.cmake ca4575653e00e8f868160976344ac1d57b5f7d27 

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


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49862: Changed libmesos from static library to a shared library.

[Srinivas Brahmaroutu]

> On July 12, 2016, 4:28 a.m., Alex Clemmer wrote:
> > src/CMakeLists.txt, line 477
> > 
> >
> > I'll copy my earlier comment and open up an issue here just to make it 
> > clear that I consider it a ship-blocker:
> > 
> > > Changing the linking structure of this project has a few very 
> > important implications for Windows, and we will need to proceed extremely 
> > cautiously.
> > > Before we get into it, could you please explain explain what the 
> > immediate reason for the patch is? It would be helpful also to have this 
> > justification captured in the commit description, so that it appears in git 
> > log.

I have having issue with libev when using static libraries, I had shared 
libraries work well for both linux and OSX. If we decide to go with static 
libraries I can investigate further on my runtime failures.


- Srinivas


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


On July 11, 2016, 8:39 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49862/
> ---
> 
> (Updated July 11, 2016, 8:39 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
> ---
> 
> Changed libmesos from static library to a shared library.
> 
> 
> Diffs
> -
> 
>   3rdparty/cmake/Mesos3rdpartyConfigure.cmake 
> eeb27860f6f95d297ccfe273ed76de5355b50ff8 
>   3rdparty/http-parser/CMakeLists.txt.template 
> 7f7105f5d55fd66f3e826c1c37c0074775bf89ea 
>   src/CMakeLists.txt 760519c5568d22f778980b25c3dfedc3c8ef83b1 
>   src/master/cmake/MasterConfigure.cmake 
> 6bbd7e87273976f40527d719cc9450ff9a1d2ac7 
>   src/slave/cmake/SlaveConfigure.cmake 
> ca4575653e00e8f868160976344ac1d57b5f7d27 
> 
> Diff: https://reviews.apache.org/r/49862/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

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

[Srinivas Brahmaroutu]

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

(Updated July 12, 2016, 4:26 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 for mesos tests.


Diffs (updated)
-

  src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 
  src/tests/cmake/TestsConfigure.cmake PRE-CREATION 
  src/tests/containerizer/CMakeLists.txt 
dab8cb07f09f5554123ede4ec8b45b60abf62eee 

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


Testing
---

cmake ..
cmake check


Thanks,

Srinivas Brahmaroutu

Review Request 49957: Fixed typos in the docs.

[Neil Conway]

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

Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

Fixed typos in the docs.


Diffs
-

  docs/agent-recovery.md 6d1b402ae5a3e26832d4fa665ca3666d4c096bc2 
  docs/attributes-resources.md 98fabfb67cf78466bf1ef8d37ebab3e987742f8e 
  docs/configuration.md 526308a803307da48928f2cf663dfea5deb4b3a1 
  docs/high-availability.md e467a0e1d592fa1aa8d8746f59cb753f28fc30e2 
  docs/mesos-containerizer.md 94144c7dfeba6ecea2271cf8b78abb8dc5dfa81e 
  docs/monitoring.md dff337d890ed68a3f615c122a940d64cf14ee1e5 
  docs/port-mapping-isolator.md 6a6bab9df3f6f3960ceeab38b6302823dceae9c2 
  docs/testing-patterns.md df11e91f2e373774fad999b0cd3fe1db9a4671de 

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


Testing
---


Thanks,

Neil Conway

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 12, 2016, 4:15 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 PRE-CREATION 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49689: Added Appc runtime isolator tests.

[Srinivas Brahmaroutu]

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

(Updated July 12, 2016, 4:15 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 Appc runtime isolator tests.


Diffs (updated)
-

  src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 
  src/tests/containerizer/appc_archive.hpp PRE-CREATION 
  src/tests/containerizer/appc_runtime_isolator_tests.cpp PRE-CREATION 

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


Testing
---

Make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49232: Added appcManifest to ImageIt nfo and ProvisionInfo.

[Srinivas Brahmaroutu]

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

(Updated July 12, 2016, 4:14 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Summary (updated)
-

Added appcManifest to ImageIt nfo and ProvisionInfo.


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


Repository: mesos


Description (updated)
---

Added appcManifest to ImageIt nfo and ProvisionInfo.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
e900d752b70165236a198f70c8cb24876a678e4b 
  src/slave/containerizer/mesos/provisioner/appc/store.cpp 
aaa0efe63e587b9e604082b52a3cb8c11545fbb9 
  src/slave/containerizer/mesos/provisioner/provisioner.hpp 
48a05059969e068a0ee0d38b61be9e7104e3188d 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
6825742546a87c8148097e6e13a94592e3b6754e 
  src/slave/containerizer/mesos/provisioner/store.hpp 
1d477ef13ddd24fd8badae0decaa4a2271ecc746 

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


Testing
---

Make Check.


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49219: Added runtime isolator interface to run appc containers.

[Srinivas Brahmaroutu]

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

(Updated July 12, 2016, 4:14 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 runtime isolator interface to run appc containers.


Diffs (updated)
-

  src/CMakeLists.txt 760519c5568d22f778980b25c3dfedc3c8ef83b1 
  src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 
  src/slave/containerizer/mesos/isolators/appc/runtime.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/appc/runtime.cpp PRE-CREATION 

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


Testing
---

Make Check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

[Srinivas Brahmaroutu]

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

(Updated July 12, 2016, 4:14 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 tests to check if appc spec is properly parsed.


Diffs (updated)
-

  src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 
  src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_appc_tests.cpp 
061f80c62319817b22a5c1880a4858fdafbfb72a 

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


Testing
---

Make check


Thanks,

Srinivas Brahmaroutu

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

[Neil Conway]

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

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
-

  docs/configuration.md 526308a803307da48928f2cf663dfea5deb4b3a1 
  docs/high-availability-framework-guide.md 
ae5617b8b5a7f82499c4860130f03b2a8c669419 
  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 48315: Restart slave if a volume is CREATED on a non-empty path.

[Anindya Sinha]

> On July 12, 2016, 9:25 a.m., Neil Conway wrote:
> > Is this change still necessary, given the changes in 
> > https://reviews.apache.org/r/48313 ? I'd prefer to keep the ability to 
> > create `MOUNT` volumes on existing filesystem content.

Without this change, we run the possibility of exposing data from a different 
persistent volume to another framework (role) which is not desirable.
Do you think we can limit the check if the persistent volume being created is 
of type MOUNT? Basically, enforce the check (as in this RR) to volumes being 
created of type != MOUNT, and preserve the original functionality for volumes 
being created for MOUNT disks?


- Anindya


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


On July 1, 2016, 9:41 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48315/
> ---
> 
> (Updated July 1, 2016, 9:41 p.m.)
> 
> 
> Review request for mesos, Neil Conway and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5448
> https://issues.apache.org/jira/browse/MESOS-5448
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Root of a MOUNT disk is not deleted on volume DELETE. When we do a
> CREATE on a persistent volume and the root directory exists (which
> can happen for MOUNT disks), we allow the operation only if the
> contents within the root directory is empty. If not, we do not update
> the checkpoint with this volume and exit, so as to cleanup when the
> slave restarts and handles the CheckpointResourcesMessage.
> 
> 
> Diffs
> -
> 
>   docs/multiple-disk.md 20b99e0e57360ad43804201b27e593d7ed48ce2c 
>   src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
>   src/tests/persistent_volume_tests.cpp 
> 5125a8da44759d1235fddac26e9eb5436c3d037b 
> 
> Diff: https://reviews.apache.org/r/48315/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 42441: Speed up ExamplesTest test cases.

[Alexander Rukletsov]

> On Jan. 22, 2016, 1:59 p.m., Jian Qiu wrote:
> > The reason I think is that the master starting from local drop the 
> > framework authentication message before it is fully started which cause the 
> > authentication timeout. I am not sure whether we can delay the start of 
> > framework until master has been fully started?
> 
> haosdent huang wrote:
> Because add a delay in a example looks strange and may let user confuse, 
> I change the authenticate timeout here.

Could you please describe the reason why tests are slow in the Jira?


- Alexander


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


On April 9, 2016, 8:41 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42441/
> ---
> 
> (Updated April 9, 2016, 8:41 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Jian Qiu.
> 
> 
> Bugs: MESOS-4155
> https://issues.apache.org/jira/browse/MESOS-4155
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up ExamplesTest test cases by decrease scheduler authenticate
> timeout.
> 
> 
> Diffs
> -
> 
>   src/sched/constants.hpp df8a1cc83ee3986400d633b2192b6da7fbe6b626 
>   src/sched/flags.hpp b4ca12b667283cee1f96a4b421fcf3b06bbe59d7 
>   src/sched/sched.cpp 5f6f5518f0858c680dc0dffc933c0bb03bba6991 
>   src/tests/java_exception_test.sh fd02322096d09cee6cc22340f18681ad372867b5 
>   src/tests/java_framework_test.sh 8fccc699133c6db6130153c367ce01ba3931c1a2 
>   src/tests/no_executor_framework_test.sh 
> 4fa154ee52454fe3f56161abaa29e58793e627e3 
>   src/tests/persistent_volume_framework_test.sh 
> 074cdcbc4a738170e84887c1773cd7c118112d58 
>   src/tests/python_framework_test.sh dc3a50f4fe8d3340429996622e62f732941985a2 
>   src/tests/test_framework_test.sh 617ca5283bd626b535d0f8091bd1d622d175f6d8 
>   src/tests/test_http_framework_test.sh 
> 3a2b24cd5017f3535340cb8ade13b34e341cd7ce 
> 
> Diff: https://reviews.apache.org/r/42441/diff/
> 
> 
> Testing
> ---
> 
> Repeat test in CentOS 7.1
> ```
> $ sudo ./bin/mesos-tests.sh --gtest_filter="ExamplesTest.*" 
> --gtest_repeat=200 --gtest_break_on_failure
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 43520: Speeded up GarbageCollectorIntegrationTest.Restart.

[Alexander Rukletsov]

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


Fix it, then Ship it!




I'll commit this now.


src/tests/gc_tests.cpp (lines 268 - 269)


Blank line.


- Alexander Rukletsov


On April 16, 2016, 3:37 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43520/
> ---
> 
> (Updated April 16, 2016, 3:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-4172
> https://issues.apache.org/jira/browse/MESOS-4172
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up GarbageCollectorIntegrationTest.Restart by reduce
> `executor_shutdown_grace_period`.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp 4cb7c2f612984f7f5a9378a7f972f2438bbf28c5 
> 
> Diff: https://reviews.apache.org/r/43520/diff/
> 
> 
> Testing
> ---
> 
> Repeat test in CentOS 7.1
> 
> ```
> $ sudo GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="GarbageCollectorIntegrationTest.Restart" --verbose 
> --gtest_repeat=200 --gtest_break_on_failure
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49910: Output disk resource source information.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49910]

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 12, 2016, 6:15 a.m., Tim Harper wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49910/
> ---
> 
> (Updated July 12, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Joseph Wu.
> 
> 
> Bugs: MESOS-5824
> https://issues.apache.org/jira/browse/MESOS-5824
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Output disk resource source information.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp cf07072 
> 
> Diff: https://reviews.apache.org/r/49910/diff/
> 
> 
> Testing
> ---
> 
> I ran `make check` in my OS X build environment. I had to disable SVN tests 
> because they didn't work, but I doubt this affected that feature set.
> 
> Resolves [MESOS-5824](https://issues.apache.org/jira/browse/MESOS-5824)
> 
> 
> Thanks,
> 
> Tim Harper
> 
>

Re: Review Request 43105: Ensured the allocator does not double account resources.

[Alexander Rukletsov]

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

(Updated July 12, 2016, 2:33 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Changes
---

Rebased.


Repository: mesos


Description
---

If `addFramework()` is called before `addSlave()` double accounting
of resources is possible. We have not observed bugs because Mesos
currently always adds a framework in the allocator after all agents
with the framework's tasks have been added.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
41a9d457286e30431490ca626e680d85684b48d6 
  src/master/allocator/mesos/hierarchical.cpp 
7d4064535a20b93950f5a95eef1ad3f0d37d305b 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 44331: Added a test to ensure the allocator does not double account resources.

[Alexander Rukletsov]

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

(Updated July 12, 2016, 2:31 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
153c9b4cf4819e976910c5a7ad9602028e2d22eb 

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


Testing
---

On Mac OS 10.10.4:

`make check`

`GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
--gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`


Thanks,

Alexander Rukletsov

Re: Review Request 43138: Updated a comment for oversubscribed resources for clarity.

[Alexander Rukletsov]

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

(Updated July 12, 2016, 2:31 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
7d4064535a20b93950f5a95eef1ad3f0d37d305b 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 49381: Benchmark for Resources class (cpu, mem & port).

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49381]

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 12, 2016, 6:57 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49381/
> ---
> 
> (Updated July 12, 2016, 6:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-5700
> https://issues.apache.org/jira/browse/MESOS-5700
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Benchmark for Resources class (cpu, mem & port).
> 
> 
> Diffs
> -
> 
>   src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 
> 
> Diff: https://reviews.apache.org/r/49381/diff/
> 
> 
> Testing
> ---
> 
> make
> MESOS_BENCHMARK=1 GTEST_FILTER="*Resources_BENCHMARK_Test.Operator*" make 
> check
> 
> [--] Global test environment set-up.
> [--] 30 tests from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN  ] 
> ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/0
> Took 5429us to `AddAndAssign` "cpus:1" 1 times.
> [   OK ] 
> ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/0 (6 ms)
> [ RUN  ] 
> ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/1
> Took 9317us to `AddAndAssign` "cpus:1;mem:2" 1 times.
> [   OK ] 
> ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/1 (10 ms)
> [ RUN  ] 
> ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/2
> **Took 5.271509secs to `AddAndAssign` "cpus(r0):1;cpus(r1):1; ... 
> cpus(r99):1" 1 times.**
> [   OK ] 
> ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/2 (5271 ms)
> [ RUN  ] 
> ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/3
> **Took 13.017557secs to `AddAndAssign` "cpus(r0):1;mem(r0):100; ... 
> cpus(r99):1;mem(r99):100" 1 times.**
> [   OK ] 
> ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/3 (13018 ms)
> [ RUN  ] 
> ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/4
> **Took 3.357184secs to `AddAndAssign` "[1-2, 4-5, ... , 298-299]" 1 
> times.**
> [   OK ] 
> ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/4 (3358 ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/0
> Took 20977us to `Add` "cpus:1" 1 times.
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/0 (21 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/1
> Took 26069us to `Add` "cpus:1;mem:2" 1 times.
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/1 (26 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/2
> **Took 5.891177secs to `Add` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 1 
> times.**
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/2 (5891 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/3
> **Took 14.894299secs to `Add` "cpus(r0):1;mem(r0):100; ... 
> cpus(r99):1;mem(r99):100" 1 times.**
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/3 
> (14895 ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/4
> **Took 2.321527secs to `Add` "[1-2, 4-5, ... , 298-299]" 1 times.**
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/4 (2321 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/0
> Took 26362us to `Sub` "cpus:1" 1 times.
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/0 (27 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/1
> Took 30828us to `Sub` "cpus:1;mem:2" 1 times.
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/1 (31 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/2
> Took 431531us to `Sub` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 1 times.
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/2 (431 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/3
> Took 837827us to `Sub` "cpus(r0):1;mem(r0):100; ... cpus(r99):1;mem(r99):100" 
> 1 times.
> [   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/3 (838 
> ms)
> [ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/4
> **Took 2.173558secs to `Sub` "[1-2, 4-5, ... , 298-299]" 1 

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

[Neil Conway]

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



Seems like this review contains a bunch of changes, in addition to lowering the 
`allocation_interval`. Are these changes related? Can you move the unrelated 
changes to a separate review, and/or explain why you have made them?

- Neil Conway


On July 11, 2016, 7:39 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49914/
> ---
> 
> (Updated July 11, 2016, 7:39 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-5732
> https://issues.apache.org/jira/browse/MESOS-5732
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Overloaded createMasterFlags in MasterAPITest to increase
> the speed of 'MasterAPITest.ReserveResources' and
> 'MasterAPITest.UnreserveResources'.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49914/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

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

[Alexander Rukletsov]

> On July 12, 2016, 4:32 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 3196
> > 
> >
> > Two issues for this patch:
> > 
> > 1) The new added framework number will introduce some cases that agent 
> > number is less than framework number and some frameworks may not able to 
> > get resources.
> > 2) This will slow down the unit test, it is better introduce this and 
> > fix https://issues.apache.org/jira/browse/MESOS-4558 together by 
> > introducing `batchsize` for benchmark test.

1) I think this a good thing to test / benchmark.
2) Benchmark tests are not part of the default test suite, hence it seems fine 
if this test takes more time to run.


- Alexander


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


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

Re: Review Request 49689: Added Appc runtime isolator tests.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: [49689]

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

- Mesos ReviewBot


On July 12, 2016, 4:40 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49689/
> ---
> 
> (Updated July 12, 2016, 4:40 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Appc runtime isolator tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 
>   src/tests/containerizer/appc_archive.hpp PRE-CREATION 
>   src/tests/containerizer/appc_runtime_isolator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49689/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

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

[Guangya Liu]

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



Did some test with `valgrind --tool=callgrind  ./src/.libs/mesos-tests  
--benchmark   
--gtest_filter="AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35"` and the 
test result is here 
https://docs.google.com/document/d/1oilen04e8trIOgYbj-jxAd7esTRDaySll8y2BQ6p5nU/edit?usp=sharing
 , the resource called count number is very heavy for some APIs, such as 
`Resource::roles::validate` etc.

- Guangya Liu


On 七月 12, 2016, 9:09 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49843/
> ---
> 
> (Updated 七月 12, 2016, 9:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Bugs: MESOS-5701
> https://issues.apache.org/jira/browse/MESOS-5701
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added benchmark test for sorter.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp 20e42419934e81b97676569876cd63ee0a550da3 
> 
> Diff: https://reviews.apache.org/r/49843/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
>  
>  ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/*"
> [==] Running 36 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 36 tests from AgentAndClientCount/Sorter_BENCHMARK_Test
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/0
> Using 1000 agents and 1 clients
> Added 1 clients in 1047us
> Added 1000 agents in 30147us
> Sorted 1 clients in 87us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/0 (33 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1
> Using 1000 agents and 50 clients
> Added 50 clients in 884us
> Added 1000 agents in 30129us
> Sorted 50 clients in 1284us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1 (37 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/2
> Using 1000 agents and 100 clients
> Added 100 clients in 1676us
> Added 1000 agents in 25157us
> Sorted 100 clients in 3814us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/2 (41 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/3
> Using 1000 agents and 200 clients
> Added 200 clients in 3930us
> Added 1000 agents in 25743us
> Sorted 200 clients in 7780us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/3 (58 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/4
> Using 1000 agents and 500 clients
> Added 500 clients in 9219us
> Added 1000 agents in 29179us
> Sorted 500 clients in 16210us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/4 (112 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/5
> Using 1000 agents and 1000 clients
> Added 1000 clients in 22029us
> Added 1000 agents in 28043us
> Sorted 1000 clients in 36030us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/5 (192 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/6
> Using 5000 agents and 1 clients
> Added 1 clients in 43us
> Added 5000 agents in 134324us
> Sorted 1 clients in 42us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/6 (136 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/7
> Using 5000 agents and 50 clients
> Added 50 clients in 845us
> Added 5000 agents in 137171us
> Sorted 50 clients in 1274us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/7 (144 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/8
> Using 5000 agents and 100 clients
> Added 100 clients in 1698us
> Added 5000 agents in 137456us
> Sorted 100 clients in 2582us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/8 (152 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/9
> Using 5000 agents and 200 clients
> Added 200 clients in 3460us
> Added 5000 agents in 129285us
> Sorted 200 clients in 5587us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/9 (159 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/10
> Using 5000 agents and 500 clients
> Added 500 clients in 8871us
> Added 5000 agents in 133412us
> Sorted 500 clients in 16717us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/10 (212 ms)
> [ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/11
> Using 5000 agents and 1000 clients
> Added 1000 clients in 22705us
> Added 5000 agents in 143366us
> Sorted 1000 clients in 36587us
> [   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/11 (313 ms)
> [ RUN  ] 

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49784]

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

Re: Review Request 48315: Restart slave if a volume is CREATED on a non-empty path.

[Neil Conway]

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



Is this change still necessary, given the changes in 
https://reviews.apache.org/r/48313 ? I'd prefer to keep the ability to create 
`MOUNT` volumes on existing filesystem content.

- Neil Conway


On July 1, 2016, 9:41 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48315/
> ---
> 
> (Updated July 1, 2016, 9:41 p.m.)
> 
> 
> Review request for mesos, Neil Conway and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5448
> https://issues.apache.org/jira/browse/MESOS-5448
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Root of a MOUNT disk is not deleted on volume DELETE. When we do a
> CREATE on a persistent volume and the root directory exists (which
> can happen for MOUNT disks), we allow the operation only if the
> contents within the root directory is empty. If not, we do not update
> the checkpoint with this volume and exit, so as to cleanup when the
> slave restarts and handles the CheckpointResourcesMessage.
> 
> 
> Diffs
> -
> 
>   docs/multiple-disk.md 20b99e0e57360ad43804201b27e593d7ed48ce2c 
>   src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
>   src/tests/persistent_volume_tests.cpp 
> 5125a8da44759d1235fddac26e9eb5436c3d037b 
> 
> Diff: https://reviews.apache.org/r/48315/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 49910: Output disk resource source information.

[Alexander Rojas]

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



Hey Tim. Beyond AlexR suggestions, the patch looks good to me. However, you 
need to remember the description will become the commit message and therefore 
we prefer to avoid repeating the summary in the description. How about 
something along the lines:

> This patch addresses the generation of confusing error 
> messages when requested disk resources do not include 
> any source information and therefore end up being treated 
> as root volumes.
>
> Compare the generated old error message:
>
> ```
> Task uses more resources
> cpus(*):4; mem(*):4096; ports(*):[31000-31000]; disk(kafka, 
> kafka)[kafka_0:data]:960679
> than available
> cpus(*):32; mem(*):256819;  ports(*):[31000-32000]; disk(kafka, 
> kafka)[kafka_0:data]:960679;   disk(*):240169;
> ```
>
> With new:
>
> _Your example here_

- Alexander Rojas


On July 12, 2016, 8:15 a.m., Tim Harper wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49910/
> ---
> 
> (Updated July 12, 2016, 8:15 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Joseph Wu.
> 
> 
> Bugs: MESOS-5824
> https://issues.apache.org/jira/browse/MESOS-5824
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Output disk resource source information.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp cf07072 
> 
> Diff: https://reviews.apache.org/r/49910/diff/
> 
> 
> Testing
> ---
> 
> I ran `make check` in my OS X build environment. I had to disable SVN tests 
> because they didn't work, but I doubt this affected that feature set.
> 
> Resolves [MESOS-5824](https://issues.apache.org/jira/browse/MESOS-5824)
> 
> 
> Thanks,
> 
> Tim Harper
> 
>

Re: Review Request 49800: Added test case for 'GetState' call in v1 agent API.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49757, 49758, 49759, 49760, 49797, 49798, 49799, 49800]

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 12, 2016, 4:03 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49800/
> ---
> 
> (Updated July 12, 2016, 4:03 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-5516
> https://issues.apache.org/jira/browse/MESOS-5516
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case for 'GetState' call in v1 agent API.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49800/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49910: Output disk resource source information.

[Alexander Rojas]

> On July 12, 2016, 8:27 a.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 1486-1488
> > 
> >
> > Let's add the default case as well (see e.g. how `Resource` is printed).

Not sure adding a default case is the way to go. At least lately we have move 
away from it. The reason being, if we add more elements to the ENUM and there 
is a default case, we wont get neither a warning nor a compulation error if it 
is not handled in every `switch`.


- Alexander


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


On July 12, 2016, 8:15 a.m., Tim Harper wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49910/
> ---
> 
> (Updated July 12, 2016, 8:15 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Joseph Wu.
> 
> 
> Bugs: MESOS-5824
> https://issues.apache.org/jira/browse/MESOS-5824
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Output disk resource source information.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp cf07072 
> 
> Diff: https://reviews.apache.org/r/49910/diff/
> 
> 
> Testing
> ---
> 
> I ran `make check` in my OS X build environment. I had to disable SVN tests 
> because they didn't work, but I doubt this affected that feature set.
> 
> Resolves [MESOS-5824](https://issues.apache.org/jira/browse/MESOS-5824)
> 
> 
> Thanks,
> 
> Tim Harper
> 
>

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

[Jiang Yan Xu]

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



Adding to the previous partial review after discussion.

## Invariant checking and documentation

An overall comment is that I think since we keep track of shared resource 
across Master, allocator and sorter, each for a different aspect (allocated vs. 
used vs. available, etc.) and with different semantics and expactations on 
uniquesness/number of copies etc., we should very explicitly document these 
variables and method arguments and check their invariants.

Specifically, the following diagram helped me understand the relationship 
between these variables.

Sorter
|
|- allocations: hashmap
 |
 |- resources: hashmap
 |- scalarQuantities 

`Allocation.resources` expects 1 copy of each shared resource because it's 
tracking whether a client has been allocated this resource and doesn't care 
about how many times it's been allocated or used by the client. Therefore 
distinct shared resources is an invariant that we should check in `allocated()`.

Allocator
|
|- slaves: hashmap
|
|- allocated: Resources

`Slave.allocated` expects 1 copy of each shared resource **per client** and 
it's grouped into one Resources object.

Master
|
|- Slave
|  |
|  |- usedResources: hashmap
|
|- Framework
   |
   |- usedResources: hashmap
   
`Slave.usedResources` and `Framework.usedResources` expect 1 copy of each 
shared resource **per task** as they are tracking "usage".


src/master/allocator/mesos/hierarchical.cpp (lines 450 - 459)


Seems like in `slaves[slaveId].allocated` we should be maintaining one copy 
of each shared resource **per framework** but this if condition here assumes 
one copy of each shared resource across framework.



src/master/allocator/mesos/hierarchical.cpp (lines 1386 - 1400)


Ignore my previous comment on this. It wasn't clear to me what invariant 
for `slaves[slaveId].allocated` is. Now I have looked again, we define shared 
resources in `slaves[slaveId].allocated` as: We maintain one copy of shared 
resource in `slaves[slaveId].allocated` **for each framework it is allocated 
to**. 

The first question is why do we do this? I can understand that this way if 
a shared resource is allocated to N frameworks, when it's recovered from all N 
frameworks it disappears from `slaves[slaveId].allocated`.

We should document this where this variable is defined and check this 
invariant wherever appropriate.

And in here, is this equivalent to the block of code?

```
slaves[slaveId].allocated += resources - allocation.shared();
```



src/master/allocator/mesos/hierarchical.cpp (lines 1406 - 1409)


Since `allocated` above already excludes shared resources already allocated 
to this framework, why couldn't be just pass it along to 
`frameworkSorters[role]->add()|allocated()`?

For `roleSorter->allocated()` and `quotaRoleSorter->allocated()` we can use 
`resources - slaves[slaveId].allocated.shared()` right?



src/master/allocator/mesos/hierarchical.cpp (lines 1570 - 1571)


To expand on this: I understand that if shared resources are offered out in 
the 1st stage, it'll be exluded from `remainingClusterResources` so by 
excluding shared resources from `scalarQuantity` we are comparing apples to 
apples. 

On the other hand it's possible for some shared resources to be not offered 
in the 1st stage so it'll remain in `remainingClusterResources`, if we  exclude 
these shared resources from `scalarQuantity` we could be over-allocating 
nonshared resources.

Perhaps we should be consistently checking nonshared resources to prevent 
over-allocation in stage 2. i.e.,

```
// We exlude shared resources from over-allocation check because shared 
resources are alwas allocatable.
if (!remainingClusterResources.nonShared().contains(
(allocatedStage2 + scalarQuantity).nonShared())) {
  continue;
}
```



src/master/allocator/sorter/drf/sorter.hpp (line 168)


Some comments about shared resources would be nice.

```
// We maintain one copy of each shared resource allocated to a client here. 
A shared resource may be offered to the same client multiple times but here we 
are only concerned with whether it's in the 

Re: Review Request 49381: Benchmark for Resources class (cpu, mem & port).

[Klaus Ma]

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

(Updated July 12, 2016, 2:57 p.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, and Michael Park.


Changes
---

Update test data for 100 ports.


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


Repository: mesos


Description (updated)
---

Benchmark for Resources class (cpu, mem & port).


Diffs (updated)
-

  src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 

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


Testing (updated)
---

make
MESOS_BENCHMARK=1 GTEST_FILTER="*Resources_BENCHMARK_Test.Operator*" make check

[--] Global test environment set-up.
[--] 30 tests from ResourcesOperators/Resources_BENCHMARK_Test
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/0
Took 5429us to `AddAndAssign` "cpus:1" 1 times.
[   OK ] 
ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/0 (6 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/1
Took 9317us to `AddAndAssign` "cpus:1;mem:2" 1 times.
[   OK ] 
ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/1 (10 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/2
**Took 5.271509secs to `AddAndAssign` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 
1 times.**
[   OK ] 
ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/2 (5271 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/3
**Took 13.017557secs to `AddAndAssign` "cpus(r0):1;mem(r0):100; ... 
cpus(r99):1;mem(r99):100" 1 times.**
[   OK ] 
ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/3 (13018 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/4
**Took 3.357184secs to `AddAndAssign` "[1-2, 4-5, ... , 298-299]" 1 times.**
[   OK ] 
ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/4 (3358 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/0
Took 20977us to `Add` "cpus:1" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/0 (21 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/1
Took 26069us to `Add` "cpus:1;mem:2" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/1 (26 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/2
**Took 5.891177secs to `Add` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 1 
times.**
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/2 (5891 
ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/3
**Took 14.894299secs to `Add` "cpus(r0):1;mem(r0):100; ... 
cpus(r99):1;mem(r99):100" 1 times.**
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/3 (14895 
ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/4
**Took 2.321527secs to `Add` "[1-2, 4-5, ... , 298-299]" 1 times.**
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/4 (2321 
ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/0
Took 26362us to `Sub` "cpus:1" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/0 (27 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/1
Took 30828us to `Sub` "cpus:1;mem:2" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/1 (31 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/2
Took 431531us to `Sub` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/2 (431 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/3
Took 837827us to `Sub` "cpus(r0):1;mem(r0):100; ... cpus(r99):1;mem(r99):100" 
1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/3 (838 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/4
**Took 2.173558secs to `Sub` "[1-2, 4-5, ... , 298-299]" 1 times.**
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/4 (2174 
ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/0
Took 3536us to `SubAndAssign` "cpus:1" 1 times.
[   OK ] 
ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/0 (4 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/1
Took 6091us to `SubAndAssign` "cpus:1;mem:2" 1 times.
[   OK ] 
ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/1 (6 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/2
Took 399855us to `SubAndAssign` "cpus(r0):1;cpus(r1):1; 

Re: Review Request 49910: Output disk resource source information.

[Tim Harper]

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

(Updated July 12, 2016, 6:14 a.m.)


Review request for mesos, Alexander Rukletsov and Alexander Rojas.


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


Repository: mesos


Description
---

Output disk resource source information.


Diffs
-

  src/common/resources.cpp cf07072 

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


Testing
---

I ran `make check` in my OS X build environment. I had to disable SVN tests 
because they didn't work, but I doubt this affected that feature set.

Resolves [MESOS-5824](https://issues.apache.org/jira/browse/MESOS-5824)


Thanks,

Tim Harper

Review Request 49910: Output disk resource source information.

[Tim Harper]

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

Review request for mesos, Alexander Rukletsov and Alexander Rojas.


Repository: mesos


Description
---

Output disk resource source information.


Diffs
-

  src/common/resources.cpp cf07072 

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


Testing
---

I ran `make check` in my OS X build environment. I had to disable SVN tests 
because they didn't work, but I doubt this affected that feature set.

Resolves [MESOS-5824](https://issues.apache.org/jira/browse/MESOS-5824)


Thanks,

Tim Harper