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



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 12, 2016, 2:01 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49781/
> ---
> 
> (Updated July 12, 2016, 2:01 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
>  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 49688: Added cmake build for mesos tests.

[Alex Clemmer]

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

(Also, don't forget that `ldcache_tests.cpp` is Linux-specific.)


- Alex


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


On July 11, 2016, 3:46 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49688/
> ---
> 
> (Updated July 11, 2016, 3:46 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
> -
> 
>   cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
>   src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 
>   src/tests/cmake/TestsConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49688/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> cmake check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

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

[Alex Clemmer]

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




src/examples/CMakeLists.txt (line 112)


Hmm, this review comes before #49863, but that review introduces this file.

It seems like it makes sense to merge these before I review them? Thoughts?


- Alex Clemmer


On July 11, 2016, 8:42 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49870/
> ---
> 
> (Updated July 11, 2016, 8:42 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
> -
> 
>   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 49874: Added logrotate_container_logger for running mesos tests.

[Alex Clemmer]

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




src/slave/CMakeLists.txt (lines 24 - 29)


So, what's the reason this is here? All the libraries and binaries have 
their own `CMakeLists.txt` (see, _e.g._, `src/health-check/CMakeLists.txt`, 
_etc._). Is there any advantage to not adhering to this pattern here?



src/slave/cmake/SlaveConfigure.cmake (line 19)


This is meant to be the target name, right? If so, this should probably be 
a `CACHE STRING` as the other targets are.

In addition, this should probably go with all the other agent targets, in 
`Process3rdpartyConfigure.cmake`. If this seems like an odd place to do this -- 
yes, it is. We should never have put these here, but they're there now, so 
please put this with those and leave a `TODO(hausdorff)` for me to clean that 
up. And, sorry about that.


- Alex Clemmer


On July 11, 2016, 8:42 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49874/
> ---
> 
> (Updated July 11, 2016, 8:42 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
> -
> 
>   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 49921: Fixed mesos tests to run 723 test on Unix.

[Alex Clemmer]

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




src/tests/CMakeLists.txt (line 51)


Is this review relevant anymore? In #49688 it looks like we're using a much 
more recent version of this file, and I don't see un-commenting these files as 
part of the diff?

Based on what I see here, it seems like we should probably discard this 
review and/or merge it with that last one.

Thoughts?


- Alex Clemmer


On July 11, 2016, 8:44 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49921/
> ---
> 
> (Updated July 11, 2016, 8:44 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
> -
> 
>   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 49688: Added cmake build for mesos tests.

[Alex Clemmer]

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

That said, I do believe you will have to add `state/leveldb.cpp` to the build 
before that works.


- Alex


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


On July 11, 2016, 3:46 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49688/
> ---
> 
> (Updated July 11, 2016, 3:46 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
> -
> 
>   cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
>   src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 
>   src/tests/cmake/TestsConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49688/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> cmake 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: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 (updated)
---

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 49688: Added cmake build for mesos tests.

[Alex Clemmer]

> On July 12, 2016, 4:16 a.m., Alex Clemmer wrote:
> > src/tests/CMakeLists.txt, line 71
> > 
> >
> > Can we please use the Mesos style TODO format? Specifically:
> > 
> > * add semicolons after the `TODO(xxx):`
> > * capitalize the first letter of the message
> > * end message with a period
> > * indent to line up with the rest of the block (i.e., put 2 spaces 
> > between the start of the line and the `#` character)
> > 
> > In this case, it should look something like:
> > 
> > ```
> > TODO(srbrahma): Needs leveldb to compile.
> > ```
> > 
> > Also would be good to have a bug # that corresponds to the issue.

Actually, it looks like all of these `TODO`s are not quite correct -- we do 
support leveldb build, and we don't need anything else except to build the qos 
controller that I mentioned earlier. These can all be safely uncommented as 
soon as you add that to the build.

Please do keep in mind the comment about the `TODO` formatting, though.


- Alex


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


On July 11, 2016, 3:46 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49688/
> ---
> 
> (Updated July 11, 2016, 3:46 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
> -
> 
>   cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
>   src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 
>   src/tests/cmake/TestsConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49688/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> cmake check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

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

[Srinivas Brahmaroutu]

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

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

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

[Guangya Liu]

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




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.


- Guangya Liu


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 49924: Added libprocess as a shared library.

[Alex Clemmer]

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




3rdparty/libprocess/src/CMakeLists.txt (line 90)


Same question as the comment in the similar review, #49862. I will quote 
again here for posterity, in case future contributors need to know the context, 
but I expect the discussion to carry on there.

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


- Alex Clemmer


On July 11, 2016, 8:49 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49924/
> ---
> 
> (Updated July 11, 2016, 8:49 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 libprocess as a shared library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 6641acf1a0ab62bdb836d5259b885d1a987b45f1 
> 
> Diff: https://reviews.apache.org/r/49924/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make
> 
> With this patch and https://reviews.apache.org/r/49862,  Converted libmesos, 
> http_parser and libprocess to shared libraries and we are using libevent 
> shared library, zookeeper does not have a shared library in the 3rdparty (I 
> guess the code is compiled as relocatable) and did not have issues linking.
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

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

[Alex Clemmer]

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




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.


- Alex Clemmer


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.

[Alex Clemmer]

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




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 

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.


- Alex Clemmer


On July 11, 2016, 3:46 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49688/
> ---
> 
> (Updated July 11, 2016, 3:46 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
> -
> 
>   cmake/MesosConfigure.cmake b2318ed8eb4e11de43abfc15f51d12b2c0ff8fa1 
>   src/tests/CMakeLists.txt 3c530631d22aa1cfdc2c600112059601bba7d6b7 
>   src/tests/cmake/TestsConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49688/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> cmake check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

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

[Alex Clemmer]

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




cmake/MesosConfigure.cmake (line 94)


Can you please explain a bit why this moved? I'd like to keep all 
configuration files for tests in the `CMakeLists.txt` that build those tests, 
if it is at all possible.

Just a few notes on the design, in case the context is helpful for 
understanding why this is important. stout, libprocess, and libmesos each have 
a relevant `[ProjectName]Configure.cmake`, and 
`[ProjectName]TestsConfigure.cmake` scripts. The `[ProjectName]Configure.cmake` 
are intended to be complete, self-contained configuration scripts that will 
fully configure any project that wishes to consume those libraries -- all they 
need to do is to `include([ProjectName]Configure)` in the relevant 
`CMakeLists.txt`, and they're good to go.

In contrast, the `[ProjectName]TestsConfigure.cmake` files are included 
only in the `CMakeLists.txt` that declares the data needed to build the test 
target, and nowhere else.

The rationale for this really strong split between test and library 
configuration is to keep the test configuration from "bleeding" into the 
library configuration. This keeps the build extremely simple, because in 
general we can know that the test configuration is limited to the 
`CMakeLists.txt` that builds the test, which greatly limits the applicable 
domain of error in the build system. Since we can't write tests for the build 
system, this separation is really important: we need to keep things in the 
build system really, really simple if we want to be able to keep reasoning 
about the behavior of the build on multiple platforms.



cmake/MesosConfigure.cmake (lines 96 - 97)


Hmm, this seems like it should break the build. We're defining these 
scripts in `MesosConfigure.cmake` before we define `MESOS_TARGET`, yet 
`CONTAINERIZER_TEST_LIBS` lists `MESOS_TARGET` as a dependency, no? So we 
should fail to emit flags that link to libmesos.

Am I missing something obvious?



src/tests/CMakeLists.txt (line 17)


General comment: when we have an `if`, can we please indent the code in the 
conditional block?



src/tests/CMakeLists.txt (line 18)


The style guide is totally _ad hoc_, but can we please not add extraneous 
spaces here?



src/tests/CMakeLists.txt (line 21)


It looks like at least some of the following tests are missing, and I don't 
see them grep'ing the codebase. Rough guess: we should be including these in 
the relevant test subdirectories? If this is the rationale, can we please add 
them all now? If this is not the rationale, can we please leave a comment 
explaining?

```
  tests/common/command_utils_tests.cpp  \
  tests/common/http_tests.cpp   \
  tests/common/recordio_tests.cpp   \
  tests/containerizer/composing_containerizer_tests.cpp \
  tests/containerizer/docker_containerizer_tests.cpp\
  tests/containerizer/docker_spec_tests.cpp \
  tests/containerizer/docker_tests.cpp  \
  tests/containerizer/external_containerizer_test.cpp   \
  tests/containerizer/isolator_tests.cpp\
  tests/containerizer/launcher.cpp  \
  tests/containerizer/memory_test_helper.cpp\
  tests/containerizer/mesos_containerizer_tests.cpp \
  tests/containerizer/provisioner_appc_tests.cpp\
  tests/containerizer/provisioner_backend_tests.cpp \
  tests/containerizer/provisioner_docker_tests.cpp
```

Also, though not a test, the following file also seems to be not listed 
here:

```
slave/qos_controllers/load.cpp
```



src/tests/CMakeLists.txt (line 35)


I don't think we have a `HAS_JAVA` option, do we? Does it make sense to add 
an `option` in the root `CMakeLists.txt`, in the same way we have for the 
`VERBOSE`, `REBUNDLED`, and `ENABLE_LIBEVENT` options?

(If you don't know, an `option` exposes a binary choice to the CMake 
command line, so you can do something like `cmake .. -DENABLE_LIBEVENT=1`, 
which would compile Mesos with libevent.)



src/tests/CMakeLists.txt (line 36)


See "please indent the `if` block" comment above.



src/tests/CMakeLists.txt (line 43)

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, 12:11 p.m.)


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


Changes
---

Add test results.


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

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

[--] 30 tests from ResourcesOperators/Resources_BENCHMARK_Test
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/0
Took 4550us to `AddAndAssign` "cpus:1" 1 times.
[   OK ] 
ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/0 (5 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/1
Took 9469us 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.568495secs to `AddAndAssign` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 
1 times.
[   OK ] 
ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/2 (5568 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/3
Took 14.068508secs to `AddAndAssign` "cpus(r0):1;mem(r0):100; ... 
cpus(r99):1;mem(r99):100" 1 times.
[   OK ] 
ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/3 (14069 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/4
Took 16679us to `AddAndAssign` "[1-2, 3-4, ... , 199-200]" 1 times.
[   OK ] 
ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/4 (17 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/0
Took 23720us to `Add` "cpus:1" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/0 (24 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/1
Took 27599us to `Add` "cpus:1;mem:2" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/1 (27 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/2
Took 6.361708secs to `Add` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/2 (6362 
ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/3
Took 15.174096secs to `Add` "cpus(r0):1;mem(r0):100; ... 
cpus(r99):1;mem(r99):100" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/3 (15175 
ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/4
Took 34929us to `Add` "[1-2, 3-4, ... , 199-200]" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/4 (35 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/0
Took 30136us to `Sub` "cpus:1" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/0 (30 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/1
Took 36232us to `Sub` "cpus:1;mem:2" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/1 (36 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/2
Took 431213us to `Sub` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/2 (432 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/3
Took 870109us to `Sub` "cpus(r0):1;mem(r0):100; ... cpus(r99):1;mem(r99):100" 
1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/3 (870 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/4
Took 22045us to `Sub` "[1-2, 3-4, ... , 199-200]" 1 times.
[   OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/4 (22 ms)
[ RUN  ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/0
Took 3338us 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 6255us 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 400747us to `SubAndAssign` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 1 
times.
[   OK ] 
ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/2 (401 ms)
[ RUN

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, 12:08 p.m.)


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


Changes
---

Update code diff.


Summary (updated)
-

Benchmark for Resources class (cpu, mem & port)


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


Thanks,

Klaus Ma

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

[haosdent huang]

> On July 12, 2016, 12:20 a.m., Vinod Kone wrote:
> >
> 
> Vinod Kone wrote:
> also, can you rebase the chain?

Thanks a lot for your reviews, just updated.


- haosdent


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


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 49800: Added test case for 'GetState' call in v1 agent API.

[haosdent huang]

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


Changes
---

Address @vinodkone's comment.


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

  src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49799: Added test case for 'GetTasks' call in v1 agent API.

[haosdent huang]

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

(Updated July 12, 2016, 4:02 a.m.)


Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.


Changes
---

Address @vinodkone's comment.


Repository: mesos


Description
---

Added test case for 'GetTasks' call in v1 agent API.


Diffs (updated)
-

  src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49798: Added test case for 'GetExecutors' call in v1 agent API.

[haosdent huang]

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

(Updated July 12, 2016, 4:02 a.m.)


Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.


Changes
---

Address @vinodkone's comment.


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


Repository: mesos


Description
---

Added test case for 'GetExecutors' call in v1 agent API.


Diffs (updated)
-

  src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49759: Implemented 'GetTasks' call in v1 agent API.

[haosdent huang]

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

(Updated July 12, 2016, 4:01 a.m.)


Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.


Changes
---

Rebase.


Repository: mesos


Description
---

Implemented 'GetTasks' call in v1 agent API.


Diffs (updated)
-

  include/mesos/agent/agent.proto cfd117de81396bf79049b7642f1ccd1ff4fbb676 
  include/mesos/v1/agent/agent.proto 213c428d424d8e4f0cc07bd86f1ed59b60df107c 
  src/slave/http.cpp 21c7ebf7c23fd06bee7125c90576eb892b249b4d 
  src/slave/slave.hpp 42afa9e2ebe5cf8e35802c8d169f52879d6073ac 
  src/slave/validation.cpp b07e80a5e0b7d6cd383cf2d9914b8c83f740770d 

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


Testing
---

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


Thanks,

haosdent huang

Re: Review Request 49797: Added test case for 'GetFrameworks' call in v1 agent API.

[haosdent huang]

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

(Updated July 12, 2016, 4:02 a.m.)


Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added test case for 'GetFrameworks' call in v1 agent API.


Diffs (updated)
-

  src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49760: Implemented 'GetState' call in v1 agent API.

[haosdent huang]

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

(Updated July 12, 2016, 4:02 a.m.)


Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Implemented 'GetState' call in v1 agent API.


Diffs (updated)
-

  include/mesos/agent/agent.proto cfd117de81396bf79049b7642f1ccd1ff4fbb676 
  include/mesos/v1/agent/agent.proto 213c428d424d8e4f0cc07bd86f1ed59b60df107c 
  src/slave/http.cpp 21c7ebf7c23fd06bee7125c90576eb892b249b4d 
  src/slave/slave.hpp 42afa9e2ebe5cf8e35802c8d169f52879d6073ac 

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


Testing
---

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


Thanks,

haosdent huang

Re: Review Request 49759: Implemented 'GetTasks' call in v1 agent API.

[haosdent huang]

> On July 8, 2016, 12:11 p.m., Jay Guo wrote:
> > src/slave/http.cpp, lines 1240-1261
> > 
> >
> > @zhitao and I once had a conversation to create a helper function to 
> > create approvers, which can be used by many methods we had. He had a patch 
> > here: https://reviews.apache.org/r/49130/ I wonder if we could still have 
> > it.

@guoer I saw the patch have already discarded. I think we could update it 
together if want to refactor this helper functions in the future.


- haosdent


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


On July 8, 2016, 10:24 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49759/
> ---
> 
> (Updated July 8, 2016, 10:24 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented 'GetTasks' call in v1 agent API.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto ef07b1f0668dca9dcebc898cfc6caf5680b24016 
>   include/mesos/v1/agent/agent.proto 8f845081d504cd045677a65f7dea7429472c5bbc 
>   src/slave/http.cpp 86803fe5c6524ada01956840e90404ab176ff8fb 
>   src/slave/slave.hpp a8952f00086c8feb486895eb6a41cc2d27432da0 
>   src/slave/validation.cpp b07e80a5e0b7d6cd383cf2d9914b8c83f740770d 
> 
> Diff: https://reviews.apache.org/r/49759/diff/
> 
> 
> Testing
> ---
> 
> Test case is https://reviews.apache.org/r/49799/
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49757: Implemented 'GetFrameworks' call in v1 agent API.

[haosdent huang]

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

(Updated July 12, 2016, 3:59 a.m.)


Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Implemented 'GetFrameworks' call in v1 agent API.


Diffs (updated)
-

  include/mesos/agent/agent.proto cfd117de81396bf79049b7642f1ccd1ff4fbb676 
  include/mesos/v1/agent/agent.proto 213c428d424d8e4f0cc07bd86f1ed59b60df107c 
  src/slave/http.cpp 21c7ebf7c23fd06bee7125c90576eb892b249b4d 
  src/slave/slave.hpp 42afa9e2ebe5cf8e35802c8d169f52879d6073ac 
  src/slave/validation.cpp b07e80a5e0b7d6cd383cf2d9914b8c83f740770d 

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


Testing
---

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


Thanks,

haosdent huang

Re: Review Request 49758: Implemented 'GetExecutors' call in v1 agent API.

[haosdent huang]

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

(Updated July 12, 2016, 4 a.m.)


Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Implemented 'GetExecutors' call in v1 agent API.


Diffs (updated)
-

  include/mesos/agent/agent.proto cfd117de81396bf79049b7642f1ccd1ff4fbb676 
  include/mesos/v1/agent/agent.proto 213c428d424d8e4f0cc07bd86f1ed59b60df107c 
  src/slave/http.cpp 21c7ebf7c23fd06bee7125c90576eb892b249b4d 
  src/slave/slave.hpp 42afa9e2ebe5cf8e35802c8d169f52879d6073ac 
  src/slave/validation.cpp b07e80a5e0b7d6cd383cf2d9914b8c83f740770d 

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


Testing
---

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


Thanks,

haosdent huang

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



Bad patch!

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

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

Error:
2016-07-12 03:51:54 URL:https://reviews.apache.org/r/49757/diff/raw/ 
[7371/7371] -> "49757.patch" [1]
error: patch failed: include/mesos/agent/agent.proto:181
error: include/mesos/agent/agent.proto: patch does not apply
error: patch failed: include/mesos/v1/agent/agent.proto:181
error: include/mesos/v1/agent/agent.proto: patch does not apply

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

- Mesos ReviewBot


On July 8, 2016, 4:13 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49800/
> ---
> 
> (Updated July 8, 2016, 4:13 p.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 4cb70720ec6df86ad113170fb664de1bfbd809aa 
> 
> Diff: https://reviews.apache.org/r/49800/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

(Updated 七月 12, 2016, 3:19 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 (updated)
-

  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 466us
Added 1000 agents in 27008us
Sorted 1 clients in 62us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/0 (31 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1
Using 1000 agents and 50 clients
Added 50 clients in 955us
Added 1000 agents in 25414us
Sorted 50 clients in 1307us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1 (33 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/2
Using 1000 agents and 100 clients
Added 100 clients in 1671us
Added 1000 agents in 26337us
Sorted 100 clients in 3136us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/2 (40 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/3
Using 1000 agents and 200 clients
Added 200 clients in 3755us
Added 1000 agents in 25517us
Sorted 200 clients in 5717us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/3 (54 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/4
Using 1000 agents and 500 clients
Added 500 clients in 8816us
Added 1000 agents in 27486us
Sorted 500 clients in 15121us
...


Thanks,

Guangya Liu

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

[Jay Guo]

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




src/tests/mesos.hpp (line 582)


why `role1` but not `role`?


- Jay Guo


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 48593: Refactor Ranges Subtraction.

[Yanyan Hu]

> On July 11, 2016, 11:37 p.m., Joseph Wu wrote:
> > Content-wise, looks good.  
> > 
> > I've left some comments below on a few stylistic nits, which I'll fix 
> > before committing.
> > I also went ahead and tweaked your patch description to explain what the 
> > improvement was.

Hi, Joseph, thank you so much for helping fix these issues!


- Yanyan


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


On July 11, 2016, 7:52 a.m., Yanyan Hu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48593/
> ---
> 
> (Updated July 11, 2016, 7:52 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Guangya Liu, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-5425
> https://issues.apache.org/jira/browse/MESOS-5425
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch reimplement Ranges subtraction using
> IntervalSet data type: Ranges values will be
> converted to IntervalSet values for subtraction
> and the result will be converted back to Ranges
> after subtraction is done. This change is for
> fixing jira MESOS-5425.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/48593/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yanyan Hu
> 
>

Review Request 49939: Updated GET_METRICS call in AGENT v1 api to return metrics grouped by types.

[Tuan-Anh Hoang-Vu]

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

Review request for mesos, Benjamin Mahler and Vinod Kone.


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


Repository: mesos


Description
---

Updated GET_METRICS call in agent v1 api to return metrics grouped by types.


Diffs
-

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

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


Testing
---

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


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


Thanks,

Tuan-Anh Hoang-Vu

Review Request 49938: Updated GET_METRICS call in MASTER v1 api to return metrics grouped by types.

[Tuan-Anh Hoang-Vu]

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

Review request for mesos, Benjamin Mahler and Vinod Kone.


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


Repository: mesos


Description
---

Updated GET_METRICS call in MASTER v1 api to return metrics grouped by types.


Diffs
-

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

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


Testing
---

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


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


Thanks,

Tuan-Anh Hoang-Vu

Review Request 49937: Added snapshotByTypes() method in libprocess.

[Tuan-Anh Hoang-Vu]

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

Review request for mesos, Benjamin Mahler and Vinod Kone.


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


Repository: mesos


Description
---

Added snapshotByTypes() method in libprocess.


Diffs
-

  3rdparty/libprocess/include/process/metrics/metrics.hpp 
54487ab2614c7f8e8df10d1e0c39880a6cf5bde3 
  3rdparty/libprocess/src/metrics/metrics.cpp 
ac1544e70f5884f946ce3c31c5430c6a2c1f9dd1 

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


Testing
---

make
make check


Thanks,

Tuan-Anh Hoang-Vu

Review Request 49936: Added metric types in libprocess.

[Tuan-Anh Hoang-Vu]

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

Review request for mesos, Benjamin Mahler and Vinod Kone.


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


Repository: mesos


Description
---

Added metric types in libprocess.


Diffs
-

  3rdparty/libprocess/include/process/metrics/counter.hpp 
a13cc7e18c8b23eae83c326d63874d9d2aaedc0d 
  3rdparty/libprocess/include/process/metrics/gauge.hpp 
c5bbeaf5d53c60f3636d1778db47cdb6cdda34e8 
  3rdparty/libprocess/include/process/metrics/metric.hpp 
21f162d5b7d9e56dc3289d65b6d86deb4c2fa721 
  3rdparty/libprocess/include/process/metrics/timer.hpp 
0a9c0227c457c6c81a59f65f901a5464ee00983d 
  3rdparty/libprocess/src/tests/metrics_tests.cpp 
5a82f4f49aecd03d12687de629516be5b7895036 

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


Testing
---

Modified MetricsTest.Counter, MetricsTest.Gauge and MetricsTest.Timer to make 
sure we set metric types correctly when creating them.


Thanks,

Tuan-Anh Hoang-Vu

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 七月 12, 2016, 2:01 a.m.)


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


Changes
---

Added dependency


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

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

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

[Guangya Liu]

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

Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Updated HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave to
call vector reserve to reserve soem spaces before using, this can
help improve the performance of the test.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
153c9b4cf4819e976910c5a7ad9602028e2d22eb 

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


Testing
---

make
make check

./bin/mesos-tests.sh --benchmark 
--gtest_filter="SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/*"
[==] Running 36 tests from 1 test case.
[--] Global test environment set-up.
[--] 36 tests from 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0
Using 1000 agents and 1 frameworks
Added 1 frameworks in 102us
Added 1000 agents in 1.110443secs
Updated 1000 agents in 583296us
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/0 
(1993 ms)
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/1
Using 1000 agents and 50 frameworks
Added 50 frameworks in 1826us
Added 1000 agents in 2.577197secs
Updated 1000 agents in 2.412386secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/1 
(5265 ms)
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/2
Using 1000 agents and 100 frameworks
Added 100 frameworks in 4260us
Added 1000 agents in 4.275021secs
Updated 1000 agents in 3.902358secs
[   OK ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.AddAndUpdateSlave/2 
(8494 ms)
...


Thanks,

Guangya Liu

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Anand Mazumdar]

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

(Updated July 12, 2016, 1:25 a.m.)


Review request for mesos, Vinod Kone and Zhitao Li.


Changes
---

Review comments


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


Repository: mesos


Description
---

This FD leak would only surface when running tests. We hold on to
a reference of the `Connection` object in the client so that it is
not destroyed before the connection is active. When running tests,
the IP:Port of libprocess remain the same which means the objects
keep on accumulating. In a real world cluster, we remove the
subscriber upon noticing a _disconnection_ i.e. this means the
socket has already been already closed upstream by Libprocess on
the server side.


Diffs (updated)
-

  src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
  src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 

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


Testing
---

make check (gtest_repeat=1000) no FD leaks


Thanks,

Anand Mazumdar

Re: Review Request 49924: Added libprocess as a shared library.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49688, 49862, 49863, 49870, 49874, 49921, 49924]

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 11, 2016, 8:49 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49924/
> ---
> 
> (Updated July 11, 2016, 8:49 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 libprocess as a shared library.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 6641acf1a0ab62bdb836d5259b885d1a987b45f1 
> 
> Diff: https://reviews.apache.org/r/49924/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make
> 
> With this patch and https://reviews.apache.org/r/49862,  Converted libmesos, 
> http_parser and libprocess to shared libraries and we are using libevent 
> shared library, zookeeper does not have a shared library in the 3rdparty (I 
> guess the code is compiled as relocatable) and did not have issues linking.
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Anand Mazumdar]

> On July 9, 2016, 7:21 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1765
> > 
> >
> > don't follow why this has to be Owned.
> 
> Anand Mazumdar wrote:
> Same reasoning as above. If it's not an `Owned`, it would be destroyed 
> when the stack allocated object (`Subscriber`) goes out of scope leading to 
> the `Pipe` getting closed.
> 
> Vinod Kone wrote:
> Why is ~Subscriber getting called when the subscriber is removed from the 
> map a problem? I might be missing something here.
> 
> Anand Mazumdar wrote:
> Let me elaborate a bit. 
> 
> `~Subscriber()` getting called when the subscriber is removed from the 
> map is not an issue. Infact, that is the desired behavior.
> 
> The problem however is `~Subscriber()` _also_ getting called for the 
> temporary objects on the stack and thereby un-intentinally closing the `Pipe` 
> i.e.
> 
> - https://github.com/apache/mesos/blob/master/src/master/master.cpp#L7595
> - https://github.com/apache/mesos/blob/master/src/master/master.cpp#L7576
> 
> Vinod Kone wrote:
> hmm. I see. my question then would be whether automatically doing 
> http.close() in ~Subscriber is safe? What is the guarantee that someone won't 
> come along in the future takes a copy of Subscriber? Should we disallow 
> copy/assignment ?

Agreed. Explicitly disallowing would be a good idea to avoid such potential 
gotchas in the future. I would update the patch shortly.


- Anand


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


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

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 11, 2016, 6:11 p.m.)


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


Changes
---

Sync comment about how Jenkins calls this script.


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

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 11, 2016, 5:35 p.m., Daniel Pravat wrote:
> > support/windows-build.bat, line 71
> > 
> >
> > If you add one more target Clean  you don't need to delete any folder.

Do you mean the "build/include" and "build/src" folders?  I've already 
committed the fix for MESOS-5756, so those are no longer necessary.


- Joseph


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


On July 11, 2016, 3:19 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49926/
> ---
> 
> (Updated July 11, 2016, 3:19 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
> 
> :: TODO(josephw): Remove this once MESOS-5756 is resolved.
> :: Remove any previously generated protobuf directories.
> :: This forces CMake to regenerate the protobufs.
> RMDIR /q /s "build/include"
> RMDIR /q /s "build/src"
> 
> :: 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 49844: Fixed a subscriber FD leak when running tests.

[Vinod Kone]

> On July 9, 2016, 7:21 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1765
> > 
> >
> > don't follow why this has to be Owned.
> 
> Anand Mazumdar wrote:
> Same reasoning as above. If it's not an `Owned`, it would be destroyed 
> when the stack allocated object (`Subscriber`) goes out of scope leading to 
> the `Pipe` getting closed.
> 
> Vinod Kone wrote:
> Why is ~Subscriber getting called when the subscriber is removed from the 
> map a problem? I might be missing something here.
> 
> Anand Mazumdar wrote:
> Let me elaborate a bit. 
> 
> `~Subscriber()` getting called when the subscriber is removed from the 
> map is not an issue. Infact, that is the desired behavior.
> 
> The problem however is `~Subscriber()` _also_ getting called for the 
> temporary objects on the stack and thereby un-intentinally closing the `Pipe` 
> i.e.
> 
> - https://github.com/apache/mesos/blob/master/src/master/master.cpp#L7595
> - https://github.com/apache/mesos/blob/master/src/master/master.cpp#L7576

hmm. I see. my question then would be whether automatically doing http.close() 
in ~Subscriber is safe? What is the guarantee that someone won't come along in 
the future takes a copy of Subscriber? Should we disallow copy/assignment ?


- Vinod


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


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Anand Mazumdar]

> On July 9, 2016, 7:21 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1765
> > 
> >
> > don't follow why this has to be Owned.
> 
> Anand Mazumdar wrote:
> Same reasoning as above. If it's not an `Owned`, it would be destroyed 
> when the stack allocated object (`Subscriber`) goes out of scope leading to 
> the `Pipe` getting closed.
> 
> Vinod Kone wrote:
> Why is ~Subscriber getting called when the subscriber is removed from the 
> map a problem? I might be missing something here.

Let me elaborate a bit. 

`~Subscriber()` getting called when the subscriber is removed from the map is 
not an issue. Infact, that is the desired behavior.

The problem however is `~Subscriber()` _also_ getting called for the temporary 
objects on the stack and thereby un-intentinally closing the `Pipe` i.e.

- https://github.com/apache/mesos/blob/master/src/master/master.cpp#L7595
- https://github.com/apache/mesos/blob/master/src/master/master.cpp#L7576


- Anand


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


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

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

[Daniel Pravat]

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




support/windows-build.bat (line 71)


If you add one more target Clean  you don't need to delete any folder.


- Daniel Pravat


On July 11, 2016, 10:19 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49926/
> ---
> 
> (Updated July 11, 2016, 10:19 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
> 
> :: TODO(josephw): Remove this once MESOS-5756 is resolved.
> :: Remove any previously generated protobuf directories.
> :: This forces CMake to regenerate the protobufs.
> RMDIR /q /s "build/include"
> RMDIR /q /s "build/src"
> 
> :: 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 49865: Renamed `info` to `executorInfo` in `Master::Http::_getExecutors`.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 10, 2016, 6:47 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49865/
> ---
> 
> (Updated July 10, 2016, 6:47 a.m.)
> 
> 
> Review request for mesos, Joerg Schad and Vinod Kone.
> 
> 
> Bugs: MESOS-5750
> https://issues.apache.org/jira/browse/MESOS-5750
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed `info` to `executorInfo` in `Master::Http::_getExecutors`.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
> 
> Diff: https://reviews.apache.org/r/49865/diff/
> 
> 
> Testing
> ---
> 
> # To address the comment @joerg84 posted at 
> https://reviews.apache.org/r/49685/
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49844: Fixed a subscriber FD leak when running tests.

[Vinod Kone]

> On July 9, 2016, 7:21 p.m., Vinod Kone wrote:
> > src/master/master.hpp, line 1765
> > 
> >
> > don't follow why this has to be Owned.
> 
> Anand Mazumdar wrote:
> Same reasoning as above. If it's not an `Owned`, it would be destroyed 
> when the stack allocated object (`Subscriber`) goes out of scope leading to 
> the `Pipe` getting closed.

Why is ~Subscriber getting called when the subscriber is removed from the map a 
problem? I might be missing something here.


- Vinod


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


On July 9, 2016, 3:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49844/
> ---
> 
> (Updated July 9, 2016, 3:21 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-5812
> https://issues.apache.org/jira/browse/MESOS-5812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This FD leak would only surface when running tests. We hold on to
> a reference of the `Connection` object in the client so that it is
> not destroyed before the connection is active. When running tests,
> the IP:Port of libprocess remain the same which means the objects
> keep on accumulating. In a real world cluster, we remove the
> subscriber upon noticing a _disconnection_ i.e. this means the
> socket has already been already closed upstream by Libprocess on
> the server side.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 845f2f6103b58e114dc5d50e3fcf70607c92a469 
>   src/master/master.cpp 79e3d78ba45060bc2f2532fdc3d105d1cc888d0f 
> 
> Diff: https://reviews.apache.org/r/49844/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=1000) no FD leaks
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

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

[Vinod Kone]

> On July 12, 2016, 12:20 a.m., Vinod Kone wrote:
> >

also, can you rebase the chain?


- Vinod


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


On July 8, 2016, 4:13 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49800/
> ---
> 
> (Updated July 8, 2016, 4:13 p.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 4cb70720ec6df86ad113170fb664de1bfbd809aa 
> 
> Diff: https://reviews.apache.org/r/49800/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

[Vinod Kone]

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




src/tests/api_tests.cpp (line 2838)


need to do Clock::pause() and Clock::settle()


- Vinod Kone


On July 8, 2016, 4:13 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49800/
> ---
> 
> (Updated July 8, 2016, 4:13 p.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 4cb70720ec6df86ad113170fb664de1bfbd809aa 
> 
> Diff: https://reviews.apache.org/r/49800/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49798: Added test case for 'GetExecutors' call in v1 agent API.

[Vinod Kone]

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




src/tests/api_tests.cpp (line 2592)


you probably want to do a Clock::pause() and Clock::settle() to ensure 
Framework::destroyExecutor() is processed.


- Vinod Kone


On July 8, 2016, 4:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49798/
> ---
> 
> (Updated July 8, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-5810
> https://issues.apache.org/jira/browse/MESOS-5810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case for 'GetExecutors' call in v1 agent API.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 4cb70720ec6df86ad113170fb664de1bfbd809aa 
> 
> Diff: https://reviews.apache.org/r/49798/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49799: Added test case for 'GetTasks' call in v1 agent API.

[Vinod Kone]

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




src/tests/api_tests.cpp (lines 2703 - 2709)


a task typically goes to completed when its terminal update is ACKed. you 
need to wait for ACK from the scheduler instead of stopping the scheduler and 
waiting for executor termination.


- Vinod Kone


On July 8, 2016, 10:22 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49799/
> ---
> 
> (Updated July 8, 2016, 10:22 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case for 'GetTasks' call in v1 agent API.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 4cb70720ec6df86ad113170fb664de1bfbd809aa 
> 
> Diff: https://reviews.apache.org/r/49799/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49798: Added test case for 'GetExecutors' call in v1 agent API.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 8, 2016, 4:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49798/
> ---
> 
> (Updated July 8, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-5810
> https://issues.apache.org/jira/browse/MESOS-5810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case for 'GetExecutors' call in v1 agent API.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 4cb70720ec6df86ad113170fb664de1bfbd809aa 
> 
> Diff: https://reviews.apache.org/r/49798/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49797: Added test case for 'GetFrameworks' call in v1 agent API.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 8, 2016, 4:14 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49797/
> ---
> 
> (Updated July 8, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-5809
> https://issues.apache.org/jira/browse/MESOS-5809
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case for 'GetFrameworks' call in v1 agent API.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 4cb70720ec6df86ad113170fb664de1bfbd809aa 
> 
> Diff: https://reviews.apache.org/r/49797/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

[Vinod Kone]

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




src/tests/master_slave_reconciliation_tests.cpp (lines 617 - 618)


don't need these as well?


- Vinod Kone


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

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49913, 49914]

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 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 49928: Fixed dependency on .proto sources to generate protobufs.

[Joseph Wu]

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


Ship it!




Confirmed that this regenerates the proper protobufs upon detecting a change.

- Joseph Wu


On July 11, 2016, 3 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49928/
> ---
> 
> (Updated July 11, 2016, 3 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alex Clemmer, and Joseph Wu.
> 
> 
> Bugs: MESOS-5756
> https://issues.apache.org/jira/browse/MESOS-5756
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed dependency on .proto sources to generate protobufs.
> 
> 
> Diffs
> -
> 
>   src/cmake/MesosProtobuf.cmake 273d74df581d85b9ef08a533c6ad8e31a23f1417 
> 
> Diff: https://reviews.apache.org/r/49928/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make 
> Changed a .proto source and see if that triggered recompilation of the proto 
> and the relevant cpp files that depende on this corresponding .cc file.
> 
> 
> 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/
---

(Updated 七月 11, 2016, 11:38 p.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 (updated)
-

  src/tests/sorter_tests.cpp 20e42419934e81b97676569876cd63ee0a550da3 

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


Testing (updated)
---

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 466us
Added 1000 agents in 27008us
Sorted 1 clients in 62us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/0 (31 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1
Using 1000 agents and 50 clients
Added 50 clients in 955us
Added 1000 agents in 25414us
Sorted 50 clients in 1307us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/1 (33 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/2
Using 1000 agents and 100 clients
Added 100 clients in 1671us
Added 1000 agents in 26337us
Sorted 100 clients in 3136us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/2 (40 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/3
Using 1000 agents and 200 clients
Added 200 clients in 3755us
Added 1000 agents in 25517us
Sorted 200 clients in 5717us
[   OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/3 (54 ms)
[ RUN  ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/4
Using 1000 agents and 500 clients
Added 500 clients in 8816us
Added 1000 agents in 27486us
Sorted 500 clients in 15121us
...


Thanks,

Guangya Liu

Re: Review Request 48593: Refactor Ranges Subtraction.

[Joseph Wu]

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


Ship it!




Content-wise, looks good.  

I've left some comments below on a few stylistic nits, which I'll fix before 
committing.
I also went ahead and tweaked your patch description to explain what the 
improvement was.


src/common/values.cpp (lines 275 - 276)


* Missing a space after `foreach`.
* Missing curly braces.
* Indent by 2 spaces, not 4.



src/common/values.cpp (lines 288 - 292)


* Opening curly brace here stays inline.
* Rename `s` variable to `interval`.
* Indent with 2 spaces, not 3 :)



src/common/values.cpp (lines 395 - 397)


Rename to `_left`, `_right`, `left`, and `right`.



src/common/values.cpp (line 401)


Missing some spaces here.



src/tests/values_tests.cpp (line 318)


Capitalize test name.  Here and below.



src/tests/values_tests.cpp (lines 338 - 339)


This fits on one line.



src/tests/values_tests.cpp (line 366)


Typo here: `tp`.



src/tests/values_tests.cpp (lines 369 - 370)


Fits on one line.


- Joseph Wu


On July 11, 2016, 12:52 a.m., Yanyan Hu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48593/
> ---
> 
> (Updated July 11, 2016, 12:52 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Guangya Liu, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-5425
> https://issues.apache.org/jira/browse/MESOS-5425
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch reimplement Ranges subtraction using
> IntervalSet data type: Ranges values will be
> converted to IntervalSet values for subtraction
> and the result will be converted back to Ranges
> after subtraction is done. This change is for
> fixing jira MESOS-5425.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/48593/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yanyan Hu
> 
>

Re: Review Request 49840: Fixed the right angle brackets in routing tests.

[Qian Zhang]

> On July 11, 2016, 10:15 p.m., Qian Zhang wrote:
> > Is this (">>") the coding convention of Mesos? If so, we may need to 
> > mention it in 
> > https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md. 
> > And I see there are still a lot of code in Mesos using "> >" now, maybe we 
> > need a separate patch to update all of them?
> 
> Benjamin Bannier wrote:
> The `> >` form is the exception to how C++ is used nowadays; that form 
> works around a deficiency where these right angle brackets might have been 
> understood as some form of `operator>>`. In C++11 which is used in Mesos we 
> do not need this workaround. We usually fix these as we touch code, but I 
> filed MESOS-5830 for the sweep.

Got it, thanks Ben!


- Qian


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


On July 9, 2016, 7:57 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49840/
> ---
> 
> (Updated July 9, 2016, 7:57 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the right angle brackets in routing tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/routing_tests.cpp 
> 452a56da84f7508709d6e71f121bcf6219f992e6 
> 
> Diff: https://reviews.apache.org/r/49840/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

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

[Vinod Kone]

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



LGTM from what I can understand. I'll let the Microsoft Guys take a look as 
well.

Great comments btw!

- Vinod Kone


On July 11, 2016, 10:19 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49926/
> ---
> 
> (Updated July 11, 2016, 10:19 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
> 
> :: TODO(josephw): Remove this once MESOS-5756 is resolved.
> :: Remove any previously generated protobuf directories.
> :: This forces CMake to regenerate the protobufs.
> RMDIR /q /s "build/include"
> RMDIR /q /s "build/src"
> 
> :: 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 49864: Fixed ExecutorPIDTest.

[Jie Yu]

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




src/tests/containerizer/mesos_containerizer_tests.cpp (line 1294)


Please include `slave/containerizer/mesos/constants.hpp` and having a 
`using` statement as Kevin suggested.


- Jie Yu


On July 10, 2016, 6:28 a.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49864/
> ---
> 
> (Updated July 10, 2016, 6:28 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed ExecutorPIDTest.
> 
> 
> Diffs
> -
> 
>   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 49813: Added stubs for the unified cgroups isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp (line 44)


I think for public interfaces, we use this style so that doxygen can pick 
up that. However, I don't think we should expose this class directly. 
Therefore, let's use `//` here.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp (lines 85 - 88)


Can we add this method later when we actually filling the impl.?



src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp (lines 91 - 94)


Ditto. Can we introduce this method when we actually fill the impl.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp (lines 118 - 154)


Ditto. Let's don't introduce those helpers in this method.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp (lines 158 - 160)


Let's use `//` here.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 56 - 62)


Please don't introduce those helper yet in this patch. Ditto for all 
others. Only keep those virtual methods that's defined in the parent class.



src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp (lines 17 - 18)


I would probably further split this patch into two:
1) introduce the cgroups isolator
2) introduce the subsystem

It'll be easier to review.



src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp (lines 74 - 75)


It's hard to tell why we need this function from this patch. That's the 
reason I suggested that we should adding helpers as we need those in the impl.


- Jie Yu


On July 9, 2016, 10:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49813/
> ---
> 
> (Updated July 9, 2016, 10:16 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added stubs for the unified cgroups isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 760519c5568d22f778980b25c3dfedc3c8ef83b1 
>   src/Makefile.am 28dd15166937ed672f81be5a598df149b8ed4302 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49813/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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 11, 2016, 3:19 p.m.)


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


Changes
---

Add more details.


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

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

:: TODO(josephw): Remove this once MESOS-5756 is resolved.
:: Remove any previously generated protobuf directories.
:: This forces CMake to regenerate the protobufs.
RMDIR /q /s "build/include"
RMDIR /q /s "build/src"

:: 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 49907: Added Timothy Chen to contributors.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49907]

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 11, 2016, 5:49 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49907/
> ---
> 
> (Updated July 11, 2016, 5:49 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Timothy Chen to contributors.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 7272b59bfff1c30958f705b849b75f5cc656321e 
> 
> Diff: https://reviews.apache.org/r/49907/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Review Request 49928: Fixed dependency on .proto sources to generate protobufs.

[Srinivas Brahmaroutu]

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

Review request for mesos, Anand Mazumdar, Alex Clemmer, and Joseph Wu.


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


Repository: mesos


Description
---

Fixed dependency on .proto sources to generate protobufs.


Diffs
-

  src/cmake/MesosProtobuf.cmake 273d74df581d85b9ef08a533c6ad8e31a23f1417 

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


Testing
---

cmake .. && make 
Changed a .proto source and see if that triggered recompilation of the proto 
and the relevant cpp files that depende on this corresponding .cc file.


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49920: Added backport of MESOS-5576 and MESOS-5740 to 0.28.3.

[Joseph Wu]

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

(Updated July 11, 2016, 2:58 p.m.)


Review request for mesos, Benjamin Mahler, Artem Harutyunyan, and Jie Yu.


Repository: mesos


Description
---

See summary.


Diffs
-

  CHANGELOG eb6cf9a41002c3e3c4f038f190d6be746a6806c9 

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


Testing (updated)
---

Running 0.28.3 + backports on internal CI...

Built and ran tests on Centos 6/7; Ubuntu 12,14,15; Fedora 23.
Some tests failed, but all within expectation, given the code at the time.


Thanks,

Joseph Wu

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

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


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.


Thanks,

Joseph Wu

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 11, 2016, 9:42 p.m.)


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


Changes
---

Updates based on review comments.


Summary (updated)
-

Consistency in persistent volumes between master and agent on failure.


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


Repository: mesos


Description (updated)
---

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 (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 48313: Creation and deletion of persistent volumes across agent restart.

[Anindya Sinha]

> On July 11, 2016, 4:31 p.m., Neil Conway wrote:
> > src/slave/slave.cpp, line 4778
> > 
> >
> > Should we also remove the target resources in the case when target and 
> > checkpointed resources are the same?

Yes, you are right. We should since we do not need the target resources in 
either case.


> On July 11, 2016, 4:31 p.m., Neil Conway wrote:
> > src/slave/state.cpp, line 712
> > 
> >
> > Can we avoid reusing the same variable name? How about `infoPath` and 
> > `targetPath`?

Actually, I consolidated the 2 variables into `path` based on a previous 
comment but I think keeping them separate make following the code easier.


- Anindya


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


On July 1, 2016, 9:39 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48313/
> ---
> 
> (Updated July 1, 2016, 9:39 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 484ba758b4c87935aabd2f76a0e654a3c6d09167 
>   src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
>   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 49843: Added benchmark test for sorter.

[Benjamin Mahler]

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




src/tests/sorter_tests.cpp (lines 493 - 498)


Recall that the sorter has "clients", not "frameworks". Can you update the 
terminology?

This is confusing for the reader. The comment here says that this 
parameterizes the number of frameworks but it actually also represents the 
number of agents.

Also, the output you showed in the Testing Done section doesn't mention how 
many agents are used, which is pretty important to know.

Can you also parameterize this seperately on the number of agents? The 
allocator benchmarks should have some example numbers:

```  
::testing::Values(1000U, 5000U, 1U, 2U, 3U, 5U)
```

For clients, let's copy the numbers from the allocator benchmarks as well?

```
::testing::Values(1U, 50U, 100U, 200U, 500U, 1000U))
```



src/tests/sorter_tests.cpp (lines 501 - 534)


We should more explicitly describe what we're looking to measure. It seems 
to me that you're trying to measure the time it takes to do a full sort from 
the dirty state?

Perhaps it would be more straightforward to do this benchmark in steps:

Step 1. Add S slaves. How long does this take?
Step 2. Add C clients. How long does this take?
Step 3. Fill the cluster with allocations. To make this simple to start 
with, you can allocate each agent's resources to single client (round robin 
through the clients).
Step 4. Do a full sort. How long does this take?

That should be a pretty comprehensive benchmark. We can call this 
Sorter_BENCHMARK_Test.FullSort.



src/tests/sorter_tests.cpp (lines 509 - 510)


Let's also have "disk", "ports" here to try to get closer to a real world 
scenario. We don't need to do any complicated port fragmentation for now.


- Benjamin Mahler


On July 10, 2016, 10:27 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49843/
> ---
> 
> (Updated July 10, 2016, 10:27 p.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 0a921347586808863e615ca3dcc23fae92b629f5 
> 
> Diff: https://reviews.apache.org/r/49843/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
>  ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="FrameworkCount/Sorter_BENCHMARK_Test.SortFrameworks*"
>  [==] Running 4 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 4 tests from FrameworkCount/Sorter_BENCHMARK_Test
> [ RUN  ] FrameworkCount/Sorter_BENCHMARK_Test.SortFrameworks/0
> Sorted 1000 frameworks  in 5443us
> [   OK ] FrameworkCount/Sorter_BENCHMARK_Test.SortFrameworks/0 (67 ms)
> [ RUN  ] FrameworkCount/Sorter_BENCHMARK_Test.SortFrameworks/1
> Sorted 2000 frameworks  in 10393us
> [   OK ] FrameworkCount/Sorter_BENCHMARK_Test.SortFrameworks/1 (114 ms)
> [ RUN  ] FrameworkCount/Sorter_BENCHMARK_Test.SortFrameworks/2
> Sorted 3000 frameworks  in 22161us
> [   OK ] FrameworkCount/Sorter_BENCHMARK_Test.SortFrameworks/2 (190 ms)
> [ RUN  ] FrameworkCount/Sorter_BENCHMARK_Test.SortFrameworks/3
> Sorted 5000 frameworks  in 31893us
> [   OK ] FrameworkCount/Sorter_BENCHMARK_Test.SortFrameworks/3 (341 ms)
> [--] 4 tests from FrameworkCount/Sorter_BENCHMARK_Test (713 ms total)
> 
> [--] Global test environment tear-down
> [==] 4 tests from 1 test case ran. (728 ms total)
> [  PASSED  ] 4 tests.
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 49812: Added missing header `version.hpp` in `src/linux/perf.hpp`.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 9, 2016, 10:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49812/
> ---
> 
> (Updated July 9, 2016, 10:16 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `version.hpp` is required in `perf.hpp`, otherwise compile would failed
> if include `perf.hpp` without `version.hpp`.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp 674d5f886ea41b939a8e48832ee6595a78b2f6ce 
> 
> Diff: https://reviews.apache.org/r/49812/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49846: The SorterTest.RevocableResources should add total resources for slave.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On July 9, 2016, 3:59 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49846/
> ---
> 
> (Updated July 9, 2016, 3:59 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The SorterTest.RevocableResources should add total resources for slave.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp 0a921347586808863e615ca3dcc23fae92b629f5 
> 
> Diff: https://reviews.apache.org/r/49846/diff/
> 
> 
> Testing
> ---
> 
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from SorterTest
> [ RUN  ] SorterTest.RevocableResources
> [   OK ] SorterTest.RevocableResources (6 ms)
> [--] 1 test from SorterTest (6 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (21 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 40487: MESOS-3959: show slave hostname on executor page

[Benjamin Mahler]

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



Hey Ian,

It does appear that we still do not have the hostname in this page:
https://github.com/apache/mesos/blob/master/src/webui/master/static/agent_executor.html

However, this patch needs a rebase. Can you also include a screenshot to prove 
you've tested this? :)

- Benjamin Mahler


On July 10, 2016, 12:44 a.m., Ian Babrou wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40487/
> ---
> 
> (Updated July 10, 2016, 12:44 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3959: show slave hostname on executor page
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/slave_executor.html 
> 7c66405090f46f89bdd29806a58c05dc76c0ad23 
> 
> Diff: https://reviews.apache.org/r/40487/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Babrou
> 
>

Re: Review Request 40456: MESOS-3950: show running task count in web ui

[Benjamin Mahler]

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



It appears to be already implemented now that this review has become stale:
https://github.com/apache/mesos/blob/39b9ba6870c9691a92e51c0a75f9eded5da67ec7/src/webui/master/static/home.html#L61-L99

Sorry we missed this at the time. Closing as discarded, please re-open if I'm 
misisng something!

- Benjamin Mahler


On July 10, 2016, 12:45 a.m., Ian Babrou wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40456/
> ---
> 
> (Updated July 10, 2016, 12:45 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3950: show running task count in web ui
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/home.html 4fc64c0aea558fca18083dc317f8370670d7a4d3 
>   src/webui/master/static/js/controllers.js 
> ccf5c31715e298e96f493cce58921bfe6b16b779 
>   src/webui/master/static/slave.html a1446bce827944609faf10f72e788f33d275d6f9 
> 
> Diff: https://reviews.apache.org/r/40456/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Babrou
> 
>

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

[Guangya Liu]

> On 七月 7, 2016, 6:38 a.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/isolators/appc/runtime.cpp, lines 215-219
> > 
> >
> > So for the case of sh=0,value=0,argv=1,Exec=1, what about the value of 
> > `Exec[1]...` etc? Should not it be `./Exec[0] Exec[1] ... argv`
> 
> Gilbert Song wrote:
> Thanks Guangya. Let's keep this open. 
> 
> It depends on whether or not users have the ability to overwrite.

Yes, but I thin at least we need to clarify the behaviro in comments and the 
document if there are multiple exec.


> On 七月 7, 2016, 6:38 a.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/isolators/appc/runtime.cpp, line 265
> > 
> >
> > Seems you are losing the logic to handle logic in row 2 when there are 
> > arguments?
> 
> Gilbert Song wrote:
> The logic seems fine to me here. That case is covered.

Seems this will only cover line 1 but not line 2, comments?


- Guangya


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


On 七月 6, 2016, 6:32 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49348/
> ---
> 
> (Updated 七月 6, 2016, 6:32 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 49920: Added backport of MESOS-5576 and MESOS-5740 to 0.28.3.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On July 11, 2016, 8:26 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49920/
> ---
> 
> (Updated July 11, 2016, 8:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   CHANGELOG eb6cf9a41002c3e3c4f038f190d6be746a6806c9 
> 
> Diff: https://reviews.apache.org/r/49920/diff/
> 
> 
> Testing
> ---
> 
> Running 0.28.3 + backports on internal CI... results pending.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 49924: Added libprocess as a shared library.

[Srinivas Brahmaroutu]

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

Review request for mesos, Alex Clemmer and Joseph Wu.


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


Repository: mesos


Description
---

Added libprocess as a shared library.


Diffs
-

  3rdparty/libprocess/src/CMakeLists.txt 
6641acf1a0ab62bdb836d5259b885d1a987b45f1 

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


Testing
---

cmake .. && make

With this patch and https://reviews.apache.org/r/49862,  Converted libmesos, 
http_parser and libprocess to shared libraries and we are using libevent shared 
library, zookeeper does not have a shared library in the 3rdparty (I guess the 
code is compiled as relocatable) and did not have issues linking.


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49813: Added stubs for the unified cgroups isolator.

[Gilbert Song]

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



This patch looks good to me. Will come back with a ship it after verifying the 
following patches good with those two `struct`.

- Gilbert Song


On July 9, 2016, 3:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49813/
> ---
> 
> (Updated July 9, 2016, 3:16 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added stubs for the unified cgroups isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 760519c5568d22f778980b25c3dfedc3c8ef83b1 
>   src/Makefile.am 28dd15166937ed672f81be5a598df149b8ed4302 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49813/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49813: Added stubs for the unified cgroups isolator.

[Gilbert Song]

> On July 10, 2016, 2:50 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp, lines 101-116
> > 
> >
> > Suggest to change to:
> > ```
> >   struct Info
> >   {
> > Info(const ContainerID& _containerId, const std::string& _cgroup)
> >   : containerId(_containerId), cgroup(_cgroup) {}
> > 
> > const ContainerID containerId;
> > const std::string cgroup;
> > 
> > // This promise will complete if a container is impacted
> > // by a resource limitation and should be terminated.
> > process::Promise limitation;
> >   };
> > ```

+1 on this.


- Gilbert


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


On July 9, 2016, 3:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49813/
> ---
> 
> (Updated July 9, 2016, 3:16 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added stubs for the unified cgroups isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 760519c5568d22f778980b25c3dfedc3c8ef83b1 
>   src/Makefile.am 28dd15166937ed672f81be5a598df149b8ed4302 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49813/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

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
-

  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 11, 2016, 8:42 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 (updated)
---

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 11, 2016, 8:42 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 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 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 (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 49812: Added missing header `version.hpp` in `src/linux/perf.hpp`.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On July 9, 2016, 3:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49812/
> ---
> 
> (Updated July 9, 2016, 3:16 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `version.hpp` is required in `perf.hpp`, otherwise compile would failed
> if include `perf.hpp` without `version.hpp`.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp 674d5f886ea41b939a8e48832ee6595a78b2f6ce 
> 
> Diff: https://reviews.apache.org/r/49812/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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`

Thanks Guangya. Let's keep this open. 

It depends on whether or not users have the ability to overwrite.


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

The logic seems fine to me here. That case is covered.


> On July 6, 2016, 11:38 p.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/isolators/appc/runtime.cpp, line 287
> > 
> >
> > s/WorkingDirectory/workingDirectory

good catch.


- Gilbert


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


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

Re: Review Request 49920: Added backport of MESOS-5576 and MESOS-5740 to 0.28.3.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 11, 2016, 8:26 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49920/
> ---
> 
> (Updated July 11, 2016, 8:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   CHANGELOG eb6cf9a41002c3e3c4f038f190d6be746a6806c9 
> 
> Diff: https://reviews.apache.org/r/49920/diff/
> 
> 
> Testing
> ---
> 
> Running 0.28.3 + backports on internal CI... results pending.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 49920: Added backport of MESOS-5576 and MESOS-5740 to 0.28.3.

[Joseph Wu]

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

Review request for mesos, Benjamin Mahler, Artem Harutyunyan, and Jie Yu.


Repository: mesos


Description
---

See summary.


Diffs
-

  CHANGELOG eb6cf9a41002c3e3c4f038f190d6be746a6806c9 

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


Testing
---

Running 0.28.3 + backports on internal CI... results pending.


Thanks,

Joseph Wu

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



Could you address Guangya's comments? Thanks.


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


Please fix this comment.



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


no need this logic here.



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


Newline above.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (lines 286 - 287)


Could we double check this one in an Appc image?


- Gilbert Song


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

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

[Jie Yu]

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




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.


- Jie Yu


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

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

[Alex Clemmer]

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



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

- Alex Clemmer


On July 11, 2016, 3:47 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, 3:47 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 49906: Add lawrencew to contributors

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [49906]

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

Error:
2016-07-11 20:08:07 URL:https://reviews.apache.org/r/49906/diff/raw/ [529/529] 
-> "49906.patch" [1]
No files to lint

Error: Commit message summary (the first line) must end in a period.

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

- Mesos ReviewBot


On July 11, 2016, 5:47 p.m., Lawrence Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49906/
> ---
> 
> (Updated July 11, 2016, 5:47 p.m.)
> 
> 
> Review request for mesos and Ian Downes.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add lawrencew to contributors
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 7272b59bfff1c30958f705b849b75f5cc656321e 
> 
> Diff: https://reviews.apache.org/r/49906/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Lawrence Wu
> 
>

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

[Abhishek Dasgupta]

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

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 49903: Printed empty set if `Resources` instance is empty.

[Isabel Jimenez]

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


Ship it!




Ship It!

- Isabel Jimenez


On July 11, 2016, 4:31 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49903/
> ---
> 
> (Updated July 11, 2016, 4:31 p.m.)
> 
> 
> Review request for mesos, Isabel Jimenez, Michael Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, empty `Resources` instances have been omitted
> if printed to a stream. This showed up as double space, which is
> confusing. Now, an empty `Resources` instance is explicitly printed.
> 
> Prior:
> <...> with oversubscribed resources  (total: <...>, allocated: )
> 
> After:
> <...> with oversubscribed resources {} (total: <...>, allocated: {})
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> Diff: https://reviews.apache.org/r/49903/diff/
> 
> 
> Testing
> ---
> 
> make check + visual inspection
> 
> Prior:
> ```
> Agent 0aa85f40-9409-45d7-b45c-6b6905b2836d-S0 (192.168.0.18) updated with 
> oversubscribed resources  (total: cpus(*):8; mem(*):15360; disk(*):470847; 
> ports(*):[31000-32000], allocated: )
> ```
> 
> After:
> ```
> Agent cd200600-03f7-4f5a-8721-94659c149145-S0 (192.168.0.18) updated with 
> oversubscribed resources {} (total: cpus(*):8; mem(*):15360; disk(*):470847; 
> ports(*):[31000-32000], allocated: {})
> ```
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 49903: Printed empty set if `Resources` instance is empty.

[Till Toenshoff]

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


Ship it!




Thanks for improving the logging Alex - I am a great fan of concise logs that 
actually make sense to the user :).

I wish we did not have to do this in two places for your patch, but that is a 
whole different question right now.

- Till Toenshoff


On July 11, 2016, 4:31 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49903/
> ---
> 
> (Updated July 11, 2016, 4:31 p.m.)
> 
> 
> Review request for mesos, Isabel Jimenez, Michael Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, empty `Resources` instances have been omitted
> if printed to a stream. This showed up as double space, which is
> confusing. Now, an empty `Resources` instance is explicitly printed.
> 
> Prior:
> <...> with oversubscribed resources  (total: <...>, allocated: )
> 
> After:
> <...> with oversubscribed resources {} (total: <...>, allocated: {})
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> Diff: https://reviews.apache.org/r/49903/diff/
> 
> 
> Testing
> ---
> 
> make check + visual inspection
> 
> Prior:
> ```
> Agent 0aa85f40-9409-45d7-b45c-6b6905b2836d-S0 (192.168.0.18) updated with 
> oversubscribed resources  (total: cpus(*):8; mem(*):15360; disk(*):470847; 
> ports(*):[31000-32000], allocated: )
> ```
> 
> After:
> ```
> Agent cd200600-03f7-4f5a-8721-94659c149145-S0 (192.168.0.18) updated with 
> oversubscribed resources {} (total: cpus(*):8; mem(*):15360; disk(*):470847; 
> ports(*):[31000-32000], allocated: {})
> ```
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

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

[Abhishek Dasgupta]

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

Review request for mesos, Anand Mazumdar 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 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/#review141305
---



Partial review. Let's discuss first.


src/master/master.cpp (lines 3495 - 3498)


`task.has_executor()` doesn't always lead to additional resources being 
used/charged against the framework. See `Master::addTask()`. It will suck a lot 
if we have to duplicate that logic here though so we might have to consider 
doing the pending task refactor first...



src/master/master.cpp (lines 3597 - 3598)






src/master/master.cpp (lines 3907 - 3910)


Need to check whether this executor actually consumes resources.



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


A comment on what it is used for?

```
Due to the two stages in the allocation algorithm and the nature of shared 
resources being re-offerable even if already allocated, the same shared 
resources can appear in two distinct offers in one allocation cycle. This would 
be bad because the allocator API contract shouldn't depend on its 
implementation details. For now we make sure a shared resource is only 
allocated once in one offer cycle. To achieve this we use 
`offeredSharedResources` to keep track of shared resources already allocated in 
the current cycle.
```



src/master/allocator/mesos/hierarchical.cpp (lines 1331 - 1340)


Given this is the 1st stage of the allocate cycle there's no chance a 
shared resource could be in `offeredSharedResources` already?



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


IIUC the comment still means "this is done to make sure shared resources 
are counted at most once in the framework's allocation".

The last revision had:

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

How is this longer version different?



src/master/allocator/mesos/hierarchical.cpp (lines 1499 - 1513)


Is this equivalent?

```
Resources available = slaves[slaveId].total.nonShared() - 
slaves[slaveId].allocated).nonShared();
available += slaves[slaveId].total.shared() - 
offeredSharedResources[slaveId];
```



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


Explain the `nonShared()` here.

`remainingClusterResources` doesn't exclude shared resources.



src/master/allocator/mesos/hierarchical.cpp (lines 1595 - 1605)


Ditto to my comment on the same code above.



src/master/allocator/sorter/drf/sorter.cpp (lines 163 - 170)


So `DRFSorter::allocated` makes sure there's no duplicate shared resources 
in `allocations` so the caller can call it without pruning out the redundant 
copies. However the `unallocated` counterpart doesn't do the same? Ideally 
usage of the pair of API should be symmetric.



src/master/allocator/sorter/drf/sorter.cpp (line 194)


No longer relevant?



src/master/master.hpp (line 274)


Add some comment to help explain why we need this method:

```
  // Return a subset of `resources` offered to `frameworkId` that can 
  // be recovered. Right now we filter out shared resources that are
  // still used by `frameworkId`.
  // NOTE: A framework can reuse a shared resource to launch multiple
  // tasks and the allocator only recovers a shared resource allocated
  // to the framework if it's not used by any task of the framework.
```



src/master/master.hpp (line 278)


s/updated/result/.



src/master/master.hpp (lines 280 - 286)


If `updated` has multple copies of a shared resource, the `-=` here can't 
remove it.

We have to add it back. The result would have a single copy of the shared 
resource that are unused (by this framework; I think this is what we want).

```
Resources recoverable(const Resources& resources)
{
  Resources recoverable = resources.nonShared();
  foreach (const Resource& resource, resources.shared()) {
if (!usedResources[frameworkId].contains(resource)) {
  recoverable += resource;
}
  }

  return recoverable;
}
```

Re: Review Request 49907: Added Timothy Chen to contributors.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 11, 2016, 5:49 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49907/
> ---
> 
> (Updated July 11, 2016, 5:49 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Timothy Chen to contributors.
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 7272b59bfff1c30958f705b849b75f5cc656321e 
> 
> Diff: https://reviews.apache.org/r/49907/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Review Request 49907: Added Timothy Chen to contributors.

[Timothy Chen]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added Timothy Chen to contributors.


Diffs
-

  docs/contributors.yaml 7272b59bfff1c30958f705b849b75f5cc656321e 

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


Testing
---

make


Thanks,

Timothy Chen

Re: Review Request 49906: Add lawrencew to contributors

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 11, 2016, 5:47 p.m., Lawrence Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49906/
> ---
> 
> (Updated July 11, 2016, 5:47 p.m.)
> 
> 
> Review request for mesos and Ian Downes.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add lawrencew to contributors
> 
> 
> Diffs
> -
> 
>   docs/contributors.yaml 7272b59bfff1c30958f705b849b75f5cc656321e 
> 
> Diff: https://reviews.apache.org/r/49906/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Lawrence Wu
> 
>