Review Request 52546: Moved a few test helpers to the common header.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Gilbert Song.


Repository: mesos


Description
---

Moved a few test helpers to the common header.


Diffs
-

  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
5b6a1d409293e8d16f6bd78bc9c6f5a524c5e0fe 
  src/tests/mesos.hpp a05e3e0fad92169dc550f1a1a8b704074716d99e 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Review Request 52547: Moved image volume related tests to a separate file.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Gilbert Song.


Repository: mesos


Description
---

Moved image volume related tests to a separate file.


Diffs
-

  src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
5b6a1d409293e8d16f6bd78bc9c6f5a524c5e0fe 
  src/tests/containerizer/volume_image_isolator_tests.cpp PRE-CREATION 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Review Request 52545: Replaced POSIX `int` with `FileDesc` abstraction.

[Daniel Pravat]

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

Review request for mesos, Joseph Wu and Michael Park.


Repository: mesos


Description
---

On POSIX this should have no effect.


Diffs
-

  3rdparty/libprocess/include/process/io.hpp 
eec5efd7e6b71a783f2bb40826054d0488cee71f 
  3rdparty/libprocess/include/process/network.hpp 
52110667185370a4c92e2fa524819ab1f34bdec9 
  3rdparty/libprocess/include/process/socket.hpp 
67551a904ebc4c2f97d65ad7ab5d4ab8c07f16db 
  3rdparty/libprocess/include/process/subprocess_base.hpp 
1d02454d5541f96cb4928bf027fcae3764989d67 
  3rdparty/libprocess/include/process/windows/subprocess.hpp 
f452f6743d01f0b99010fa5e5bcbaae1358c8241 
  3rdparty/libprocess/src/io.cpp d930ceebc90468e082b984e41385549f906dc6ae 
  3rdparty/libprocess/src/poll_socket.hpp 
d04f3f2d1bcf70464ac659b29f96574bbd233414 
  3rdparty/libprocess/src/poll_socket.cpp 
d9ab3fbbf385c39c0e43ee92ff06609ae8d47e2d 
  3rdparty/libprocess/src/process.cpp f1d746c52cfe659f5cd7da4b7a6424ff585619a3 
  3rdparty/libprocess/src/socket.cpp 6089248639793603226210421a2c2193d14ea049 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
c8350cf8e512dca23933725e6edb3e3d94380211 

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


Testing
---


Thanks,

Daniel Pravat

Review Request 52544: Introduced `FileDesc` class.

[Daniel Pravat]

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

Review request for mesos, Joseph Wu and Michael Park.


Repository: mesos


Description
---

On POSIX the `socket`,`pipe` and the `filedescriptor` are
represented by an int type. In Windows a socket is kept in a
`SOCKET` type (64 bit wide), a pipe in a `HANDLE` (64 bit wide) and
a file descriptor in an int. Ths class unifies all Windows types.
In POSIX this class defaults to int (can be easily extended).


Diffs
-

  3rdparty/stout/include/stout/os.hpp c38e434d90c8c25570118c255f2eec72f96b348d 
  3rdparty/stout/include/stout/os/filedescriptor.hpp PRE-CREATION 
  3rdparty/stout/include/stout/posix/os.hpp 
c37e64db662ba3cee83d2f55de0f9d71ad72c038 
  3rdparty/stout/include/stout/windows/os.hpp 
7d6530e37e389a314185e5aaa85d8096a28b9c41 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 52471: Fixed the wrong sandbox directory of the tasks in Web UI.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52470, 52520, 52471]

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 Oct. 4, 2016, 6:02 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52471/
> ---
> 
> (Updated Oct. 4, 2016, 6:02 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For the task launched by default-executor, its sandbox directory is
> 'executor_directory/tasks/task_id/'. This patch generates the
> corresponding sandbox directory in Web UI for the task according to its
> executor type.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent_executor.html 
> 8b83ed5acc2ecd56f20b1571878ec9f6794efbd2 
>   src/webui/master/static/framework.html 
> bc3c56adf22030e6cd1dcd8a2c945d44ff79aa4b 
>   src/webui/master/static/home.html 179cb15140f85811df0ea0a87620dd3c90dd30c7 
>   src/webui/master/static/js/app.js 400a4286d9358c699453326d87a0bd49dbb105a7 
>   src/webui/master/static/js/controllers.js 
> 29a5a1c8754cc2fb934854750d6dfb04f1eaeae4 
> 
> Diff: https://reviews.apache.org/r/52471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 50912: Added the infrastructure for a new python-based CLI.

[Kevin Klues]

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

(Updated Oct. 5, 2016, 4:12 a.m.)


Review request for mesos, Haris Choudhary and Joseph Wu.


Changes
---

Updated test procedure.


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


Repository: mesos


Description
---

Added the infrastructure for a new python-based CLI.


Diffs
-

  src/cli_new/.gitignore PRE-CREATION 
  src/cli_new/README.md PRE-CREATION 
  src/cli_new/activate PRE-CREATION 
  src/cli_new/bin/config.py PRE-CREATION 
  src/cli_new/bin/main.py PRE-CREATION 
  src/cli_new/bin/mesos PRE-CREATION 
  src/cli_new/bootstrap PRE-CREATION 
  src/cli_new/deactivate PRE-CREATION 
  src/cli_new/lib/mesos/__init__.py PRE-CREATION 
  src/cli_new/lib/mesos/docopt.py PRE-CREATION 
  src/cli_new/lib/mesos/exceptions.py PRE-CREATION 
  src/cli_new/lib/mesos/plugins/__init__.py PRE-CREATION 
  src/cli_new/lib/mesos/plugins/base.py PRE-CREATION 
  src/cli_new/lib/mesos/util.py PRE-CREATION 
  src/cli_new/mesos.bash_completion PRE-CREATION 
  src/cli_new/pip-requirements.txt PRE-CREATION 
  src/cli_new/pylint.config PRE-CREATION 

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


Testing (updated)
---

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ mesos --help


Thanks,

Kevin Klues

Re: Review Request 51008: Added infrastructure for unit tests in the new python-based CLI.

[Kevin Klues]

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

(Updated Oct. 5, 2016, 4:11 a.m.)


Review request for mesos, Haris Choudhary and Joseph Wu.


Changes
---

Update test procedure.


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


Repository: mesos


Description
---

Added infrastructure for unit tests in the new python-based CLI.


Diffs
-

  src/cli_new/bin/mesos-tests PRE-CREATION 
  src/cli_new/bin/tests.py PRE-CREATION 
  src/cli_new/pip-requirements.txt PRE-CREATION 

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


Testing (updated)
---

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ mesos-tests


Thanks,

Kevin Klues

Review Request 52543: Added configure/make options to build the new CLI and run unit tests.

[Kevin Klues]

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

Review request for mesos and Joseph Wu.


Bugs: MESOS-6008 and MESOS-6032
https://issues.apache.org/jira/browse/MESOS-6008
https://issues.apache.org/jira/browse/MESOS-6032


Repository: mesos


Description
---

Added configure/make options to build the new CLI and run unit tests.


Diffs
-

  configure.ac 8f971c96bfea0afd7462ea9b91c69458324435b6 
  docs/getting-started.md de3fba4a821ca7fa9b9e2fd692b33b40edd35b68 
  src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 

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


Testing
---

../configure --enable-new-cli
make -C src mesos
src/mesos

GTEST_FILTER="" make -j check


Thanks,

Kevin Klues

Re: Review Request 51008: Added infrastructure for unit tests in the new python-based CLI.

[Kevin Klues]

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

(Updated Oct. 5, 2016, 4:06 a.m.)


Review request for mesos, Haris Choudhary and Joseph Wu.


Changes
---

Updated to address Joseph's comments in 50912 and move logic from Makefile.am 
out of this commit.


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


Repository: mesos


Description
---

Added infrastructure for unit tests in the new python-based CLI.


Diffs (updated)
-

  src/cli_new/bin/mesos-tests PRE-CREATION 
  src/cli_new/bin/tests.py PRE-CREATION 
  src/cli_new/pip-requirements.txt PRE-CREATION 

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


Testing
---

GTEST_FILTER="" make -j check


Thanks,

Kevin Klues

Re: Review Request 50912: Added the infrastructure for a new python-based CLI.

[Kevin Klues]

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

(Updated Oct. 5, 2016, 4:05 a.m.)


Review request for mesos, Haris Choudhary and Joseph Wu.


Changes
---

Updated to address Joseph's comments.


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


Repository: mesos


Description
---

Added the infrastructure for a new python-based CLI.


Diffs (updated)
-

  src/cli_new/.gitignore PRE-CREATION 
  src/cli_new/README.md PRE-CREATION 
  src/cli_new/activate PRE-CREATION 
  src/cli_new/bin/config.py PRE-CREATION 
  src/cli_new/bin/main.py PRE-CREATION 
  src/cli_new/bin/mesos PRE-CREATION 
  src/cli_new/bootstrap PRE-CREATION 
  src/cli_new/deactivate PRE-CREATION 
  src/cli_new/lib/mesos/__init__.py PRE-CREATION 
  src/cli_new/lib/mesos/docopt.py PRE-CREATION 
  src/cli_new/lib/mesos/exceptions.py PRE-CREATION 
  src/cli_new/lib/mesos/plugins/__init__.py PRE-CREATION 
  src/cli_new/lib/mesos/plugins/base.py PRE-CREATION 
  src/cli_new/lib/mesos/util.py PRE-CREATION 
  src/cli_new/mesos.bash_completion PRE-CREATION 
  src/cli_new/pip-requirements.txt PRE-CREATION 
  src/cli_new/pylint.config PRE-CREATION 

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


Testing
---

../configure --enable-new-cli
make -C src mesos
src/mesos


Thanks,

Kevin Klues

Re: Review Request 52541: Renamed the filesystem isolator tests file name.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52538, 52539, 52540, 52541]

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 Oct. 5, 2016, 1:16 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52541/
> ---
> 
> (Updated Oct. 5, 2016, 1:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed the filesystem isolator tests file name.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 5b6a1d409293e8d16f6bd78bc9c6f5a524c5e0fe 
> 
> Diff: https://reviews.apache.org/r/52541/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 50910: Added a python linter to mesos-style.cpp.

[Kevin Klues]

> On Oct. 5, 2016, 12:18 a.m., Joseph Wu wrote:
> > support/mesos-style.py, lines 270-272
> > 
> >
> > It may help to change the order of reviews (no rebase necessary, just 
> > the "depends on" field).  
> > 
> > Because you add a linter in this review, that depends on the next one ( 
> > https://reviews.apache.org/r/50912/ ), this review fails to lint itself :)
> 
> Kevin Klues wrote:
> I'm confused. The order of these patches (and their dependencies) should 
> be correct. This patch "depends on" 50907, not 50912. Also, when I 
> cherry-pick each of these patches onto master individually, they all get 
> linted just as expected.

For example, from master:
```
klueska@c99:~/projects/mesos$ support/apply-reviews.py -r 50907
2016-10-04 20:31:30 URL:https://reviews.apache.org/r/50907/diff/raw/ 
[16055/16055] -> "50907.patch" [1]
No C++ files to lint
[cli-test d536361] Abstracted mesos-style.py to wrap the cpp linter in a class.
 1 file changed, 249 insertions(+), 169 deletions(-)
 rewrite support/mesos-style.py (95%)
klueska@c99:~/projects/mesos$ support/apply-reviews.py -r 50910
2016-10-04 20:31:39 URL:https://reviews.apache.org/r/50910/diff/raw/ 
[2997/2997] -> "50910.patch" [1]
No C++ files to lint
No Python files to lint
[cli-test 2b9502e] Added a python linter to mesos-style.cpp.
 Author: Haris Choudhary 
 1 file changed, 67 insertions(+), 2 deletions(-)
klueska@c99:~/projects/mesos$ support/apply-reviews.py -r 50912
2016-10-04 20:31:51 URL:https://reviews.apache.org/r/50912/diff/raw/ 
[40930/40930] -> "50912.patch" [1]
No C++ files to lint
Checking 8 Python files
Total errors found: 0
[cli-test 88e5726] Added the infrastructure for a new python-based CLI.
 20 files changed, 1019 insertions(+), 3 deletions(-)
 create mode 100644 src/cli_new/.gitignore
 create mode 100644 src/cli_new/README.md
 create mode 100644 src/cli_new/activate
 create mode 100644 src/cli_new/bin/config.py
 create mode 100644 src/cli_new/bin/main.py
 create mode 100755 src/cli_new/bin/mesos
 create mode 100755 src/cli_new/bootstrap
 create mode 100644 src/cli_new/deactivate
 create mode 100644 src/cli_new/lib/mesos/__init__.py
 create mode 100644 src/cli_new/lib/mesos/docopt.py
 create mode 100644 src/cli_new/lib/mesos/exceptions.py
 create mode 100644 src/cli_new/lib/mesos/plugins/__init__.py
 create mode 100644 src/cli_new/lib/mesos/plugins/base.py
 create mode 100644 src/cli_new/lib/mesos/util.py
 create mode 100644 src/cli_new/mesos.bash_completion
 create mode 100644 src/cli_new/pip-requirements.txt
 create mode 100644 src/cli_new/pylint.config
```


- Kevin


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


On Aug. 11, 2016, 9:54 p.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50910/
> ---
> 
> (Updated Aug. 11, 2016, 9:54 p.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: Mesos-6006
> https://issues.apache.org/jira/browse/Mesos-6006
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It currently doesn't run over any files in the code base, but we will
> be adding the new python CLI in a subsequent commit, which will use
> this new linter.
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 061cbe711bd9407b7341fa146f8c723eaa9fc438 
> 
> Diff: https://reviews.apache.org/r/50910/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>

Re: Review Request 50910: Added a python linter to mesos-style.cpp.

[Kevin Klues]

> On Oct. 5, 2016, 12:18 a.m., Joseph Wu wrote:
> > support/mesos-style.py, lines 270-272
> > 
> >
> > It may help to change the order of reviews (no rebase necessary, just 
> > the "depends on" field).  
> > 
> > Because you add a linter in this review, that depends on the next one ( 
> > https://reviews.apache.org/r/50912/ ), this review fails to lint itself :)

I'm confused. The order of these patches (and their dependencies) should be 
correct. This patch "depends on" 50907, not 50912. Also, when I cherry-pick 
each of these patches onto master individually, they all get linted just as 
expected.


- Kevin


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


On Aug. 11, 2016, 9:54 p.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50910/
> ---
> 
> (Updated Aug. 11, 2016, 9:54 p.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: Mesos-6006
> https://issues.apache.org/jira/browse/Mesos-6006
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It currently doesn't run over any files in the code base, but we will
> be adding the new python CLI in a subsequent commit, which will use
> this new linter.
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 061cbe711bd9407b7341fa146f8c723eaa9fc438 
> 
> Diff: https://reviews.apache.org/r/50910/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>

Re: Review Request 51774: Add `AGENT_ADDED` and `AGENT_REMOVED` master events.

[Anand Mazumdar]

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



Ditto comments for the versioned protobuf.


include/mesos/master/master.proto (line 475)


s/disconnect/disconnection



include/mesos/master/master.proto (line 476)


This can fit on the previous line itself?



include/mesos/master/master.proto (line 481)


1. Can we also add some examples on when an agent can be removed similar to 
what we did for `AGENT_ADDED`?

2. Can we also add a `NOTE` that it's possible that an agent might become 
active once it has been removed i.e., if the master has gc'ed its list of known 
"dead" agents. See MESOS-5965 for context.



include/mesos/master/master.proto (line 483)


Let's change this to be just `AgentID` since this is all the subscribers 
should care about per se i.e., they would already have the agent details as 
part of the cluster snapshot or the `AGENT_ADDED` event.


- Anand Mazumdar


On Oct. 4, 2016, 2:45 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51774/
> ---
> 
> (Updated Oct. 4, 2016, 2:45 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-6102
> https://issues.apache.org/jira/browse/MESOS-6102
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `AGENT_ADDED` and `AGENT_REMOVED` master events.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
>   include/mesos/v1/master/master.proto 
> 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 
> 
> Diff: https://reviews.apache.org/r/51774/diff/
> 
> 
> Testing
> ---
> 
> New unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52534: Dispatch filter expiration twice.

[Guangya Liu]

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



1) Instead of this fix, can we simply kill 
https://github.com/apache/mesos/blob/1.0.x/src/tests/hierarchical_allocator_tests.cpp#L782-L783
 ? Even without 
https://github.com/apache/mesos/blob/1.0.x/src/tests/hierarchical_allocator_tests.cpp#L782-L783
 , we can still make sure that the agent does not allocate to the origninal 
framework after filter time out as the filter time out is smaller than the 
allocation interval.

2) Can you please make your three patch a chain but do not let one patch 
/r/51027 depend on two patches? A proposal would be /r/51028 -> /r/52534 -> 
/r/51027

- Guangya Liu


On 十月 4, 2016, 11:28 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52534/
> ---
> 
> (Updated 十月 4, 2016, 11:28 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - With an asynchronous `batch()` allocation,
>   this ensures that filters do not expire
>   before the next allocation.
> - This patch should be reverted when allocation
>   occurs on resource recovery.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/52534/diff/
> 
> 
> Testing
> ---
> 
> With https://reviews.apache.org/r/51027/: 
> 
> GTEST_FILTER="-*SmallOfferFilter*" make check -j8
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 52537: Added unit test for provisioner 'RecoverNestedContainerNoParentImage'.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52536, 52537]

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 Oct. 5, 2016, 12:50 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52537/
> ---
> 
> (Updated Oct. 5, 2016, 12:50 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-6302
> https://issues.apache.org/jira/browse/MESOS-6302
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit test for provisioner 'RecoverNestedContainerNoParentImage'.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 6ef1c926d7aa942e241a24d4d3838a5f2d7c4bd1 
> 
> Diff: https://reviews.apache.org/r/52537/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Verified that this test would fail if we removed the fix in 
> https://reviews.apache.org/r/52480/
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52471: Fixed the wrong sandbox directory of the tasks in Web UI.

[haosdent huang]

> On Oct. 3, 2016, 10:35 p.m., Vinod Kone wrote:
> > src/webui/master/static/js/app.js, line 42
> > 
> >
> > Is this backwards compatible? What if some one is depending on this 
> > path to exist? Morever, I think it's worthwhile for people to have access 
> > to executor sandbox through the WebUI.
> > 
> > Maybe we should keep AgetExecutorRerouteCtrl and add a new 
> > AgentTaskRerouterCtrl.
> 
> haosdent huang wrote:
> Hi, @vinod Thanks for your reviews!
> 
> > Is this backwards compatible?
> 
> Yes, it is backwards compatible. Suppose user uses the old version 
> javascript, here it could not find the type of the executor, so it would 
> enter the directory of the executor. And if user uses the new version 
> javascript, it would enter correspoding directory according to the type of 
> the executor.
> 
> > What if some one is depending on this path to exist?
> 
> All the dependencies on this url have been updated. We only use this to 
> enter the sandbox of the task. For executor, we use `executor.directory` 
> directly instead of use `AgetExecutorRerouteCtrl`.
> 
> > Morever, I think it's worthwhile for people to have access to executor 
> sandbox through the WebUI.
> 
> Currently we need to access the sandbox of the executor in 
> `agent_executor.html`. And in that page, we use `executor.directory` which 
> get from the `/state` of the agent and avoid to use the reroute trick here.
> 
> > Maybe we should keep AgetExecutorRerouteCtrl and add a new 
> AgentTaskRerouterCtrl.
> 
> Because `AgetExecutorRerouteCtrl` is not used anymore, I prefer to remove 
> it. However, if we plan to show executors in `home.html`, keep 
> `AgetExecutorRerouteCtrl` would be useful so that we don't need to add it 
> back at that time.
> 
> Vinod Kone wrote:
> What I meant by "someone depending on the path" is whether someother 
> service/tool (outside of the WebUI javascript) is using the path. For example 
> Aurora or Marathon or DC/OS.

Good point! Let me keep the old one.


- haosdent


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


On Oct. 4, 2016, 6:02 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52471/
> ---
> 
> (Updated Oct. 4, 2016, 6:02 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For the task launched by default-executor, its sandbox directory is
> 'executor_directory/tasks/task_id/'. This patch generates the
> corresponding sandbox directory in Web UI for the task according to its
> executor type.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent_executor.html 
> 8b83ed5acc2ecd56f20b1571878ec9f6794efbd2 
>   src/webui/master/static/framework.html 
> bc3c56adf22030e6cd1dcd8a2c945d44ff79aa4b 
>   src/webui/master/static/home.html 179cb15140f85811df0ea0a87620dd3c90dd30c7 
>   src/webui/master/static/js/app.js 400a4286d9358c699453326d87a0bd49dbb105a7 
>   src/webui/master/static/js/controllers.js 
> 29a5a1c8754cc2fb934854750d6dfb04f1eaeae4 
> 
> Diff: https://reviews.apache.org/r/52471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52385: Fixed flag stringification bug in Linux filesystem isolator.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Sept. 29, 2016, 12:54 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52385/
> ---
> 
> (Updated Sept. 29, 2016, 12:54 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3335
> https://issues.apache.org/jira/browse/MESOS-3335
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To stringify a `Flag` we are required to pass the `Flags` instance
> actually holding the `Flag` to `Flag::stringify`. Make sure we do that
> here.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> e55f3d127f6a735fbe1c59b4a8de0d73e7232473 
> 
> Diff: https://reviews.apache.org/r/52385/diff/
> 
> 
> Testing
> ---
> 
> Tested as part of the review chain ending in 
> https://reviews.apache.org/r/52388/ on various Linux configurations in 
> internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Guangya Liu]

> On 十月 5, 2016, 1:02 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1313-1318
> > 
> >
> > The intention of making allocate asynchronous and returning a Future is 
> > that the caller could tell when the asynchronous allocation completes and 
> > could set up a continuation based on this.
> > 
> > As it stands this function just returns Nothing always? Seems it should 
> > return to the caller once the caller's requested allocation completes.

Here we want to enqueue only one `allocate(slaves)` request and then update the 
`allocationCandidates` for other event trigger allocation requests before the 
`allocate(slaves)` request was processed, after the `allocate(slaves)` request 
was processed, another `allocate(slaves)` request will be enqueued again, so 
seems we do not need to wait till the allocation completes, but just check the 
allocation status for each `allocate(slaves)` request, comments?


- Guangya


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


On 十月 4, 2016, 11:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 十月 4, 2016, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Review Request 52540: Converted the SandboxEnvironmentVariable test to an end to end test.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Gilbert Song.


Repository: mesos


Description
---

Converted the SandboxEnvironmentVariable test to an end to end test.


Diffs
-

  src/tests/containerizer/filesystem_isolator_tests.cpp 
5b6a1d409293e8d16f6bd78bc9c6f5a524c5e0fe 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Review Request 52539: Refactored and simplified LinuxFilesystemIsolatorTests.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Gilbert Song.


Repository: mesos


Description
---

Refactored and simplified LinuxFilesystemIsolatorTests.


Diffs
-

  src/tests/containerizer/filesystem_isolator_tests.cpp 
5b6a1d409293e8d16f6bd78bc9c6f5a524c5e0fe 
  src/tests/mesos.hpp a05e3e0fad92169dc550f1a1a8b704074716d99e 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Review Request 52541: Renamed the filesystem isolator tests file name.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Gilbert Song.


Repository: mesos


Description
---

Renamed the filesystem isolator tests file name.


Diffs
-

  src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
5b6a1d409293e8d16f6bd78bc9c6f5a524c5e0fe 

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


Testing
---

make check


Thanks,

Jie Yu

Review Request 52538: Reordered filesystem isolator tests.

[Jie Yu]

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

Review request for mesos, Benjamin Hindman and Gilbert Song.


Repository: mesos


Description
---

This is a pure code movement. Moved all the end to end tests to the
bottom of the file.


Diffs
-

  src/tests/containerizer/filesystem_isolator_tests.cpp 
5b6a1d409293e8d16f6bd78bc9c6f5a524c5e0fe 

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


Testing
---

sudo make check


Thanks,

Jie Yu

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

[Benjamin Mahler]

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




src/master/allocator/mesos/hierarchical.cpp (lines 1305 - 1310)


The intention of making allocate asynchronous and returning a Future is 
that the caller could tell when the asynchronous allocation completes and could 
set up a continuation based on this.

As it stands this function just returns Nothing always? Seems it should 
return to the caller once the caller's requested allocation completes.


- Benjamin Mahler


On Oct. 4, 2016, 11:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Oct. 4, 2016, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 52471: Fixed the wrong sandbox directory of the tasks in Web UI.

[Vinod Kone]

> On Oct. 3, 2016, 10:35 p.m., Vinod Kone wrote:
> > src/webui/master/static/js/app.js, line 42
> > 
> >
> > Is this backwards compatible? What if some one is depending on this 
> > path to exist? Morever, I think it's worthwhile for people to have access 
> > to executor sandbox through the WebUI.
> > 
> > Maybe we should keep AgetExecutorRerouteCtrl and add a new 
> > AgentTaskRerouterCtrl.
> 
> haosdent huang wrote:
> Hi, @vinod Thanks for your reviews!
> 
> > Is this backwards compatible?
> 
> Yes, it is backwards compatible. Suppose user uses the old version 
> javascript, here it could not find the type of the executor, so it would 
> enter the directory of the executor. And if user uses the new version 
> javascript, it would enter correspoding directory according to the type of 
> the executor.
> 
> > What if some one is depending on this path to exist?
> 
> All the dependencies on this url have been updated. We only use this to 
> enter the sandbox of the task. For executor, we use `executor.directory` 
> directly instead of use `AgetExecutorRerouteCtrl`.
> 
> > Morever, I think it's worthwhile for people to have access to executor 
> sandbox through the WebUI.
> 
> Currently we need to access the sandbox of the executor in 
> `agent_executor.html`. And in that page, we use `executor.directory` which 
> get from the `/state` of the agent and avoid to use the reroute trick here.
> 
> > Maybe we should keep AgetExecutorRerouteCtrl and add a new 
> AgentTaskRerouterCtrl.
> 
> Because `AgetExecutorRerouteCtrl` is not used anymore, I prefer to remove 
> it. However, if we plan to show executors in `home.html`, keep 
> `AgetExecutorRerouteCtrl` would be useful so that we don't need to add it 
> back at that time.

What I meant by "someone depending on the path" is whether someother 
service/tool (outside of the WebUI javascript) is using the path. For example 
Aurora or Marathon or DC/OS.


- Vinod


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


On Oct. 4, 2016, 6:02 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52471/
> ---
> 
> (Updated Oct. 4, 2016, 6:02 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For the task launched by default-executor, its sandbox directory is
> 'executor_directory/tasks/task_id/'. This patch generates the
> corresponding sandbox directory in Web UI for the task according to its
> executor type.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent_executor.html 
> 8b83ed5acc2ecd56f20b1571878ec9f6794efbd2 
>   src/webui/master/static/framework.html 
> bc3c56adf22030e6cd1dcd8a2c945d44ff79aa4b 
>   src/webui/master/static/home.html 179cb15140f85811df0ea0a87620dd3c90dd30c7 
>   src/webui/master/static/js/app.js 400a4286d9358c699453326d87a0bd49dbb105a7 
>   src/webui/master/static/js/controllers.js 
> 29a5a1c8754cc2fb934854750d6dfb04f1eaeae4 
> 
> Diff: https://reviews.apache.org/r/52471/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

[Vinod Kone]

> On Oct. 3, 2016, 10:28 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 4339
> > 
> >
> > Mutating TaskInfo should be avoided as much as possible.
> > 
> > I don't follow why we need this. 
> > 
> > For the command executor, we were able to find it's corresponding 
> > executor without having to do this, why do we need it for task group tasks?
> 
> haosdent huang wrote:
> For tasks launched by the command executor, it's executor id are same 
> with the task id. 
> https://github.com/apache/mesos/blob/1.0.1/src/slave/slave.cpp#L3778 So in 
> Web UI, it would work that if we set the executor id to the task id when the 
> executor id is empty. 
> https://github.com/apache/mesos/blob/1.0.1/src/webui/master/static/js/controllers.js#L122
> 
> But for the tasks launched by the default executor, it's executor id are 
> given by the user 
> https://github.com/apache/mesos/blob/c13644984564179979d5ef5f40c8299fadd10ee7/src/cli/execute.cpp#L461-L462
>  . And it may different with the task id. So when Web UI use the task id to 
> search the executor, it would fail.

Instead of mutating TaskInfo, lets make sure "Task.executor_id" is properly set 
for task group tasks in `protobuf::createTask()`. You also need to make sure 
default executor is added to framework and slave structs in `addTask` (this is 
actually a bug that we need to fix irrespective of WebUI fix: maybe file a 
separate ticket for that and do it in a separate review).


- Vinod


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


On Oct. 4, 2016, 5:59 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 4, 2016, 5:59 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 02a2fb29bdd8484fc90e5cb033ac29b49a141860 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 52537: Added unit test for provisioner 'RecoverNestedContainerNoParentImage'.

[Gilbert Song]

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

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


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


Repository: mesos


Description
---

Added unit test for provisioner 'RecoverNestedContainerNoParentImage'.


Diffs
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
6ef1c926d7aa942e241a24d4d3838a5f2d7c4bd1 

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


Testing
---

make check

Verified that this test would fail if we removed the fix in 
https://reviews.apache.org/r/52480/


Thanks,

Gilbert Song

Review Request 52536: Refactored the provisioner recover test.

[Gilbert Song]

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

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


Repository: mesos


Description
---

Refactored the provisioner recover test.


Diffs
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
6ef1c926d7aa942e241a24d4d3838a5f2d7c4bd1 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51108: Added additional parameters to the CLI config.

[Joseph Wu]

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




src/cli_new/bin/config.py (line 52)


Period at the end.



src/cli_new/bin/config.py (lines 78 - 82)


Given all this repeated load/validation code, you may consider doing 
something more dynamic:

```
for config_key in CONFIG_DATA.keys():
config_key_upper = config_key.upper()
if config_key_upper in globals():
if not isinstance(CONFIG_DATA[config_key], 
type(globals()[config_key_upper])):
raise CLIException("some error")

globals()[config_key_upper] = CONFIG_DATA[config_key]
else:
# Log that we're ignoring this key.
# Or bail out.
```

This replaces existing global config variables as long as the loaded JSON 
has the same type.


- Joseph Wu


On Aug. 15, 2016, 6:44 p.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51108/
> ---
> 
> (Updated Aug. 15, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Kevin Klues.
> 
> 
> Bugs: mesos-5676
> https://issues.apache.org/jira/browse/mesos-5676
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These parameters will be used by future plugins in the CLI. We can either
> edit them directly from the config file or discover them through environment
> variables. The environment variables override any edits made to the config 
> file.
> 
> Also added the ability to format command flags in the base plugin class. This 
> allows
> us to not import the config file within individual plugins to do so.
> The Key-Value pairs for formatting will be introduced with with future 
> plugins.
> 
> 
> Diffs
> -
> 
>   src/cli_new/bin/config.py PRE-CREATION 
>   src/cli_new/lib/mesos/plugins/base.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51108/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>

Re: Review Request 50910: Added a python linter to mesos-style.cpp.

[Joseph Wu]

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


Fix it, then Ship it!





support/mesos-style.py (lines 270 - 272)


It may help to change the order of reviews (no rebase necessary, just the 
"depends on" field).  

Because you add a linter in this review, that depends on the next one ( 
https://reviews.apache.org/r/50912/ ), this review fails to lint itself :)


- Joseph Wu


On Aug. 11, 2016, 2:54 p.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50910/
> ---
> 
> (Updated Aug. 11, 2016, 2:54 p.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: Mesos-6006
> https://issues.apache.org/jira/browse/Mesos-6006
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It currently doesn't run over any files in the code base, but we will
> be adding the new python CLI in a subsequent commit, which will use
> this new linter.
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 061cbe711bd9407b7341fa146f8c723eaa9fc438 
> 
> Diff: https://reviews.apache.org/r/50910/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>

Re: Review Request 50912: Added the infrastructure for a new python-based CLI.

[Joseph Wu]

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


Fix it, then Ship it!




Builds and runs as expected.


configure.ac (lines 200 - 204)


The inclusion of the autoconf/automake bits in this review gives the 
impression that we need to `make` before doing anything with the CLI.

Technically, we do not even need to `../configure --enable-new-cli` in 
order to use the CLI.

I'd prefer if the autoconf/automake things were a separate patch.



src/cli_new/README.md (lines 15 - 17)


Given that there is already a top-level `bootstrap`, you may want to refer 
to `cli_new/bootstrap` in some other way.



src/cli_new/bin/main.py (line 18)


s/mesos-cli/Mesos CLI/



src/cli_new/bin/main.py (line 32)


s/mesos/Mesos/



src/cli_new/bin/main.py (line 81)


Period at the end.



src/cli_new/bootstrap (line 41)


Period at the end.



src/cli_new/lib/mesos/exceptions.py (line 23)


Period at the end.



src/cli_new/lib/mesos/plugins/base.py (line 71)


Period at the end.



src/cli_new/lib/mesos/plugins/base.py (line 134)


Period at the end.



src/cli_new/lib/mesos/util.py (lines 73 - 77)


It would really help to explain what the three return types mean.

i.e.
```
Returns `comp_words` if the completion word is potentially in that list.
Returns an empty list if there is no possible completion.
Returns `None` if this autocomplete is already done.
```



src/cli_new/lib/mesos/util.py (line 107)


Period at the end.



src/cli_new/lib/mesos/util.py (line 113)


s/mesos/the top-level entry point/



src/cli_new/lib/mesos/util.py (line 128)


Period at the end.



src/cli_new/mesos.bash_completion (lines 16 - 20)


There are mixed tabs and spaces in this file.


- Joseph Wu


On Oct. 1, 2016, 1:09 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50912/
> ---
> 
> (Updated Oct. 1, 2016, 1:09 p.m.)
> 
> 
> Review request for mesos, Haris Choudhary and Joseph Wu.
> 
> 
> Bugs: MESOS-6008
> https://issues.apache.org/jira/browse/MESOS-6008
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the infrastructure for a new python-based CLI.
> 
> 
> Diffs
> -
> 
>   configure.ac 57482d39db1f83e92e75fca959cd6df329a1c24f 
>   docs/getting-started.md de3fba4a821ca7fa9b9e2fd692b33b40edd35b68 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/cli_new/.gitignore PRE-CREATION 
>   src/cli_new/README.md PRE-CREATION 
>   src/cli_new/activate PRE-CREATION 
>   src/cli_new/bin/config.py PRE-CREATION 
>   src/cli_new/bin/main.py PRE-CREATION 
>   src/cli_new/bin/mesos PRE-CREATION 
>   src/cli_new/bootstrap PRE-CREATION 
>   src/cli_new/deactivate PRE-CREATION 
>   src/cli_new/lib/mesos/__init__.py PRE-CREATION 
>   src/cli_new/lib/mesos/docopt.py PRE-CREATION 
>   src/cli_new/lib/mesos/exceptions.py PRE-CREATION 
>   src/cli_new/lib/mesos/plugins/__init__.py PRE-CREATION 
>   src/cli_new/lib/mesos/plugins/base.py PRE-CREATION 
>   src/cli_new/lib/mesos/util.py PRE-CREATION 
>   src/cli_new/mesos.bash_completion PRE-CREATION 
>   src/cli_new/pip-requirements.txt PRE-CREATION 
>   src/cli_new/pylint.config PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50912/diff/
> 
> 
> Testing
> ---
> 
> ../configure --enable-new-cli
> make -C src mesos
> src/mesos
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52034: Disabled error dialog in WebUI when user is unauthorized to see metrics.

[Adam B]

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


Ship it!




LGTM, although I'm no JS expert.


src/webui/master/static/js/controllers.js (lines 404 - 406)


Anything else we'd want to do instead for unauthorized users? I guess we'll 
just show empty/zero cluster metrics? That's better than the continuous error 
dialog at least.


- Adam B


On Sept. 19, 2016, 6:45 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52034/
> ---
> 
> (Updated Sept. 19, 2016, 6:45 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Disables the dialog showing a connection error in the WebUI if the
> returned error code is either _401 Unauthorized_ or _403 Forbidden_.
> 
> Showing a connection error is missleading in the latter cases, while
> it also rendered the WebUI unusable since the dialog would show
> over and over again.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/controllers.js 
> 29a5a1c8754cc2fb934854750d6dfb04f1eaeae4 
> 
> Diff: https://reviews.apache.org/r/52034/diff/
> 
> 
> Testing
> ---
> 
> manual tests and `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 52520: Exposed the executor's type in the endpoints.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 4, 2016, 6:01 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52520/
> ---
> 
> (Updated Oct. 4, 2016, 6:01 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Exposed the executor's type in the endpoints.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 538330a4c780fbc2dfcdfb31537b0e75f368e3e0 
>   src/slave/http.cpp bdafe1db9f1b22ac0761f5119e73f87b531c6a3c 
>   src/tests/default_executor_tests.cpp 
> 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 
> 
> Diff: https://reviews.apache.org/r/52520/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52042: Changed error message in pailer if user is unauthorized.

[Adam B]

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


Fix it, then Ship it!




Looks good except for the typo, but I haven't tried it personally yet.


src/webui/master/static/js/jquery.pailer.js (line 139)


s/SSS/SS/


- Adam B


On Sept. 20, 2016, 12:41 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52042/
> ---
> 
> (Updated Sept. 20, 2016, 12:41 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If attempts to use the pailer to access sandboxes and/or mesos logs fail
> because the user is unauthorized, an error telling the user he is
> unauthorized is shown to the client instead of showing the misguiding
> _Failed to initialize... retrying_. It also disables retry in case the
> client is unauthorized.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/jquery.pailer.js 
> 79c3c0dad59c2dde78913116bb08383f7a6c4d5e 
> 
> Diff: https://reviews.apache.org/r/52042/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> and manual tests with the following ACLs
> 
> ```json
> {
>   "permissive": true,
>   "access_mesos_logs" : [
> {
>   "principals" : { "values" : ["foo"] },
>   "logs" : { "type" : "NONE" }
> }
>   ],
>   "access_sandboxes" : [
> {
>   "principals" : { "values" : ["foo"] },
>   "users" : { "type" : "NONE" }
> }
>   ]
> }
> 
> ```
> 
> and credentials:
> 
> ```
> foo bar
> baz bar
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

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

[Srinivas Brahmaroutu]

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

(Updated Oct. 4, 2016, 11:45 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
---

Builds shared libraries for dynamically loading them by various tests:
libexamplemodule, libtestauthorizer, libtestisolator,
libtestresource_estimator, libtestallocator, libtestcontainer_logger,
libtestmastercontender, libtestanonymous, libtesthook,
libtestmasterdetector, libtestauthentication, libtesthttpauthenticator,
libtestqos_controller.


Diffs (updated)
-

  CMakeLists.txt ee8f565586b958929bc4e95b09827244d6d4155f 
  cmake/MesosConfigure.cmake 6650c7c12b188b08c70cfee72b3200e83e7a1cd2 
  src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 
  src/examples/CMakeLists.txt PRE-CREATION 
  src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 

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


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 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 Oct. 4, 2016, 11: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
---

Builds library liblogrotate_container_logger useful to operate
logrotate loggers.


Diffs (updated)
-

  src/slave/CMakeLists.txt 33120ace79bce449329a7cc4b7ef264d2867fc13 
  src/slave/cmake/SlaveConfigure.cmake b339239761a5de321d65b92376dae69c339bee5c 
  src/slave/container_loggers/CMakeLists.txt PRE-CREATION 

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


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

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

[Srinivas Brahmaroutu]

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

(Updated Oct. 4, 2016, 11: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
---

Builds executables dynamic-reservation-framework, test-http-framework,
test-framework, test-executor, test-http-executor, long-lived-framework,
long-lived-executor, no-executor-framework,
docker-no-executor-framework, balloon-framework, balloon-executor,
load-generator-framework, persistent-volume-framework


Diffs (updated)
-

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

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


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49921: Fixed mesos tests to run most of the tests on Unix and OSX.

[Srinivas Brahmaroutu]

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

(Updated Oct. 4, 2016, 11: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
---

This patch builds /src/mesos-tests
This patch adds all test sources, categorize them so that some are run
in environment with Java, on Unix, etc. Patch allows to run all the
tests that are enabled on a platform by simply running the mesos-tests
executable.


Diffs (updated)
-

  src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 
  src/tests/CMakeLists.txt f5d66dc63143455506d8660674fbd9eb227625ff 
  src/tests/cmake/MesosTestsConfigure.cmake 
3f543c010ae87ff04e6b45745bc49ef65b6590ff 
  src/tests/containerizer/CMakeLists.txt PRE-CREATION 

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 51027: Track allocation candidates to bound allocator.

[Jacob Janco]

> On Oct. 4, 2016, 9:47 p.m., Guangya Liu wrote:
> > Jacob, two other comments for this:
> > 
> > 1) when will you have the patch `SmallOfferFilter` ready for review?
> > 2) I think we may need another benchmark test which only do two operations: 
> > a) add multiple agents first 2) add multiple frameworks, this test can make 
> > sure we `batched` the `allocate()` request for all frameworks.

Added the fix for you: https://reviews.apache.org/r/52534


- Jacob


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


On Oct. 4, 2016, 11:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Oct. 4, 2016, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

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

[Jacob Janco]

> On Oct. 1, 2016, 12:11 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1311
> > 
> >
> > I think that here we should return `Nothing()` but not 
> > `allocation.get()`, as `allocation.get()` will wait till `allocation is 
> > ready`, but here we do not need to wait the till the `allocation become 
> > ready` but just update the `allocationCandidates` as soon as possible.

Since we're not doing anything with this future, I think it is safe to update 
allocation/allocationCandidates here and return Nothing()


- Jacob


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


On Oct. 4, 2016, 11:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Oct. 4, 2016, 11:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028 and 
> https://reviews.apache.org/r/52534
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

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

[Jacob Janco]

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

(Updated Oct. 4, 2016, 11:31 p.m.)


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


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


Repository: mesos


Description
---

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


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
2c31471ee0f5d6836393bf87ff9ecfd8df835013 
  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 

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


Testing (updated)
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028 and 
https://reviews.apache.org/r/52534


Thanks,

Jacob Janco

Review Request 52534: Dispatch filter expiration twice.

[Jacob Janco]

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

Review request for mesos.


Repository: mesos


Description
---

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


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
2c31471ee0f5d6836393bf87ff9ecfd8df835013 
  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 

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


Testing
---

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

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


Thanks,

Jacob Janco

Re: Review Request 50327: Added scripts to build sample framework executables.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50064, 50179, 50323, 50324, 50325, 50326, 50327]

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 Oct. 4, 2016, 4:53 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50327/
> ---
> 
> (Updated Oct. 4, 2016, 4:53 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 scripts to build sample framework executables.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt ee8f565586b958929bc4e95b09827244d6d4155f 
>   cmake/MesosConfigure.cmake 6650c7c12b188b08c70cfee72b3200e83e7a1cd2 
>   src/CMakeLists.txt 35eb63fe9a8e47d97512e9904bf5a714c63722a7 
>   src/examples/CMakeLists.txt PRE-CREATION 
>   src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50327/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make dynamic-reservation-framework test-http-framework 
> test-framework test-executor test-http-executor long-lived-framework 
> long-lived-executor no-executor-framework docker-no-executor-framework 
> balloon-framework balloon-executor load-generator-framework 
> persistent-volume-framework
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 46228: Create persistent volume with a supplied user.

[Greg Mann]

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




src/common/resources.cpp (line 146)


Is there a reason not to include the `principal` field in this equality 
operator?



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (line 675)


either:
s/user/the user/
or
s/user/`user`/



src/slave/slave.cpp (lines 2488 - 2502)


Looks like this bit will need some rebasing, since the volume creation code 
seems to have been factored out into a helper `syncCheckpointedResources`.



src/slave/slave.cpp (lines 2494 - 2495)


Could you enclose the code snippets in backticks? i.e.: `volume.has_disk() 
&& volume.disk().has_persistence()`


- Greg Mann


On April 14, 2016, 11:04 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46228/
> ---
> 
> (Updated April 14, 2016, 11:04 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If user is specified in Resource::DiskInfo::Persistence, set the
> ownership of the volume to that user. Tasks should have to comply to
> ownership of the volume before they can use the volume.
> If user is not specified in Resource::DiskInfo::Persistence, it is
> created with the user mesos-slave process runs as. When a task is
> launched, it updates the ownership of the persistent volume with its
> user so as to be able to write into that persistent volume.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 9fc7c48f99155750fd3c18c7c102507e2726362b 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> 01c0ad6dbb6d509e62e769365586b3d23dcb240d 
>   src/slave/slave.cpp de99e9eb5cc812b2e07deb749b98b4f4db363728 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> 
> Diff: https://reviews.apache.org/r/46228/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

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

[Guangya Liu]

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



Jacob, two other comments for this:

1) when will you have the patch `SmallOfferFilter` ready for review?
2) I think we may need another benchmark test which only do two operations: a) 
add multiple agents first 2) add multiple frameworks, this test can make sure 
we `batched` the `allocate()` request for all frameworks.


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


The `addFramework` will call `allocate()` and allocate `all agents 
resources` but not a `single agnent`, so here we should not mention 
`addFramework`, but only the following three:

1) addSlave
2) updateSlave
3) updateUnavailability?



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


s/slaveIds/`slaveIds`


- Guangya Liu


On 十月 4, 2016, 9:31 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated 十月 4, 2016, 9:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> note: SmallOfferFilter fix pending review -> will list Jira here
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

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

[Jacob Janco]

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

(Updated Oct. 4, 2016, 9:31 p.m.)


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


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
2c31471ee0f5d6836393bf87ff9ecfd8df835013 
  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 

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


Testing
---

make check

note: check without filters depends on https://reviews.apache.org/r/51028
note: SmallOfferFilter fix pending review -> will list Jira here


Thanks,

Jacob Janco

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

[Jacob Janco]

> On Sept. 28, 2016, 9:33 p.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 224
> > 
> >
> > ```
> > // If no run is queued we only update `allocationCandidates`
> > ```
> > 
> > I think here should be 
> > 
> > ```
> > We only update the `allocationCandidates` if there are pending 
> > allocation run.
> > ```

Changed to: 

// If a run is queued, we only update `allocationCandidates`.


- Jacob


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


On Sept. 28, 2016, 8:21 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> ---
> 
> (Updated Sept. 28, 2016, 8:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus 
> Ma, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 2c31471ee0f5d6836393bf87ff9ecfd8df835013 
>   src/master/allocator/mesos/hierarchical.cpp 
> 3f51f4194c1ba7c1e4f08c3dd623281ca5754d39 
> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> note: check without filters depends on https://reviews.apache.org/r/51028
> note: SmallOfferFilter fix pending review -> will list Jira here
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

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

[Srinivas Brahmaroutu]

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

(Updated Oct. 4, 2016, 9:12 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
---

Builds shared libraries for dynamically loading them by various tests:
libexamplemodule, libtestauthorizer, libtestisolator,
libtestresource_estimator, libtestallocator, libtestcontainer_logger,
libtestmastercontender, libtestanonymous, libtesthook,
libtestmasterdetector, libtestauthentication, libtesthttpauthenticator,
libtestqos_controller.


Diffs (updated)
-

  CMakeLists.txt ee8f565586b958929bc4e95b09827244d6d4155f 
  cmake/MesosConfigure.cmake 6650c7c12b188b08c70cfee72b3200e83e7a1cd2 
  src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 
  src/examples/CMakeLists.txt PRE-CREATION 
  src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 

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


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49862: Changed libraies to shared on OSX and UNIX.

[Srinivas Brahmaroutu]

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

(Updated Oct. 4, 2016, 9:10 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 fles MESOS_DEFAULT_LIBRARY_LINKAGE which is set to STATIC on
WIN32 and SHARED on OSX or Unix. This allows all libraries built
static or shared. Also minor changes made to eliminate leveldb
dependency when build slave or master. Also by setting the flag
CMAKE_POSITION_INDEPENDENT_CODE to true cmake auto sets the
flag -fPIC.


Diffs (updated)
-

  3rdparty/http-parser/CMakeLists.txt.template 
9a671973b754095e1de917f135a7deb978fb6eb6 
  cmake/MesosConfigure.cmake 6650c7c12b188b08c70cfee72b3200e83e7a1cd2 
  src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 
  src/slave/qos_controllers/CMakeLists.txt 
87c92af21c012655c201c01cd4ba5ff912555119 
  src/slave/resource_estimators/CMakeLists.txt 
17b149f734ea9dc8ac4c5dd45bdb8312faf4cc77 
  src/tests/cmake/MesosTestsConfigure.cmake 
3f543c010ae87ff04e6b45745bc49ef65b6590ff 

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


Testing
---

cmake .. && make


Thanks,

Srinivas Brahmaroutu

Re: Review Request 52433: Improved the naming of variables in HealthCheckTest.HealthStatusChange.

[Alexander Rukletsov]

> On Oct. 4, 2016, 9:07 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, lines 680-682
> > 
> >
> > Can we make it similar to the test you introduce in r/52357? They look 
> > like they do the same thing.

Ah, I see you do it in r/52434. Why have you decided to split the patches?


- Alexander


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


On Sept. 30, 2016, 5:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52433/
> ---
> 
> (Updated Sept. 30, 2016, 5:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved the naming of variables in HealthCheckTest.HealthStatusChange.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52433/diff/
> 
> 
> Testing
> ---
> 
> make check GTEST_FILTER="HealthCheckTest.*" V=0
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52433: Improved the naming of variables in HealthCheckTest.HealthStatusChange.

[Alexander Rukletsov]

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




src/tests/health_check_tests.cpp (lines 680 - 682)


Can we make it similar to the test you introduce in r/52357? They look like 
they do the same thing.


- Alexander Rukletsov


On Sept. 30, 2016, 5:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52433/
> ---
> 
> (Updated Sept. 30, 2016, 5:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved the naming of variables in HealthCheckTest.HealthStatusChange.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52433/diff/
> 
> 
> Testing
> ---
> 
> make check GTEST_FILTER="HealthCheckTest.*" V=0
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52432: Reenabled the 'HealthCheckTest.GracePeriod' test.

[Alexander Rukletsov]

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




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


Why do you check twice for the same thing? Do you want to check for 
TASK_RUNNING?


- Alexander Rukletsov


On Sept. 30, 2016, 6:12 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52432/
> ---
> 
> (Updated Sept. 30, 2016, 6:12 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test was disabled in 2014 because it was considered flaky. I
> updated it so that it runs quicker and ran it 1000 times without a
> single failure.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52432/diff/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --verbose --gtest_filter="HealthCheckTest.GracePeriod" 
> --break-on-error --gtest_repeat=1000
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52357: Improved handling of health check failures within the grace period.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Oct. 4, 2016, 1:18 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52357/
> ---
> 
> (Updated Oct. 4, 2016, 1:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-6170
> https://issues.apache.org/jira/browse/MESOS-6170
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The health check library would ignore all failures until after the end
> of the grace period.
> 
> This behaviour was misleading. With the changes in this commit, once a
> health check succeeds for the first time, the grace period rule for
> failures is not be applied any more.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> ea93132f2a5d4828c75005f102eddc4c3131599d 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52357/diff/
> 
> 
> Testing
> ---
> 
> Manual tests using Marathon.
> Added a new unit test and verified that it fails without the change, but 
> succeeds with it.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52431: Reduced boilerplate from health check tests.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Sept. 30, 2016, 5:15 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52431/
> ---
> 
> (Updated Sept. 30, 2016, 5:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reduced boilerplate from health check tests.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52431/diff/
> 
> 
> Testing
> ---
> 
> make check GTEST_FILTER="HealthCheckTest.*" V=0
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52516: Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51774, 52515, 52516]

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 Oct. 4, 2016, 2:44 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52516/
> ---
> 
> (Updated Oct. 4, 2016, 2:44 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-6102
> https://issues.apache.org/jira/browse/MESOS-6102
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 
> 
> Diff: https://reviews.apache.org/r/52516/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52462: Ported ReviewBot script to Windows.

[Joseph Wu]

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

(Updated Oct. 4, 2016, 1:29 p.m.)


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


Repository: mesos


Description
---

Changes which script ReviewBot uses to apply reviews and 
what script ReviewBot runs if on Windows.


Diffs
-

  support/verify_reviews.py f14951b753ffbc1a939d1991d7bb901741efe388 

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


Testing (updated)
---

On Windows:
python support/verify_reviews.py a b 1
// ^ Fake username and password, so as to not post any reviews.

Tested successful build and unsuccessful build (introduced a syntax error in 
libprocess).

---

TODO:
Wait for this: https://issues.apache.org/jira/browse/INFRA-12681
Then test run: https://builds.apache.org/job/Mesos-Windows-Reviewbot/


Thanks,

Joseph Wu

Re: Review Request 51605: Added "mesos-tcp-connect" binary.

[haosdent huang]

> On Sept. 2, 2016, 4:53 p.m., haosdent huang wrote:
> > src/Makefile.am, line 1377
> > 
> >
> > I think the name `mesos-tcp-connect` may be not clear enough. Should we 
> > add something like `health-check` into its name to indicate it is used for 
> > health check?
> 
> Avinash sridharan wrote:
> I agree with @haosdent on this. Would like to see `health-check` in the 
> suffix of the binary name.
> 
> Alexander Rukletsov wrote:
> Why? The binary establishes a TCP connection, which is not necessarily 
> health check related?

> Why? The binary establishes a TCP connection, which is not necessarily health 
> check related?

Make sense, we could use it in other places not only health check.


- haosdent


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


On Oct. 4, 2016, 1:54 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51605/
> ---
> 
> (Updated Oct. 4, 2016, 1:54 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Benjamin Mahler, Gastón Kleiman, 
> and haosdent huang.
> 
> 
> Bugs: MESOS-6119
> https://issues.apache.org/jira/browse/MESOS-6119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To remove dependency on `bash` for TCP health checks, introduce
> a separate light-weight binary (without libmesos dependency) for
> probing TCP connections.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 
>   src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
>   src/health-check/CMakeLists.txt PRE-CREATION 
>   src/health-check/tcp_connect.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51605/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/51607/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 51606: Libprocess: Added target for "mesos-tcp-connect" binary.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On Oct. 4, 2016, 1:55 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51606/
> ---
> 
> (Updated Oct. 4, 2016, 1:55 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and haosdent huang.
> 
> 
> Bugs: MESOS-6119
> https://issues.apache.org/jira/browse/MESOS-6119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
> 3ef9040e9fe37058886aea0480582886c255ccb8 
> 
> Diff: https://reviews.apache.org/r/51606/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/51607/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 51606: Libprocess: Added target for "mesos-tcp-connect" binary.

[haosdent huang]

> On Sept. 6, 2016, 3:31 p.m., haosdent huang wrote:
> > 3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake, lines 155-159
> > 
> >
> > I think we need to put this after `MESOS_DOCKER_EXECUTOR` to keep the 
> > order
> 
> Alexander Rukletsov wrote:
> I though we maintain alphabetical order, no?

Sorry I must nap when write the above comment.


- haosdent


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


On Oct. 4, 2016, 1:55 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51606/
> ---
> 
> (Updated Oct. 4, 2016, 1:55 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and haosdent huang.
> 
> 
> Bugs: MESOS-6119
> https://issues.apache.org/jira/browse/MESOS-6119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
> 3ef9040e9fe37058886aea0480582886c255ccb8 
> 
> Diff: https://reviews.apache.org/r/51606/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/51607/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 52471: Fixed the wrong sandbox directory of the tasks in Web UI.

[haosdent huang]

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

(Updated Oct. 4, 2016, 6:02 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Fix @vinodkone's comments.


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


Repository: mesos


Description (updated)
---

For the task launched by default-executor, its sandbox directory is
'executor_directory/tasks/task_id/'. This patch generates the
corresponding sandbox directory in Web UI for the task according to its
executor type.


Diffs (updated)
-

  src/webui/master/static/agent_executor.html 
8b83ed5acc2ecd56f20b1571878ec9f6794efbd2 
  src/webui/master/static/framework.html 
bc3c56adf22030e6cd1dcd8a2c945d44ff79aa4b 
  src/webui/master/static/home.html 179cb15140f85811df0ea0a87620dd3c90dd30c7 
  src/webui/master/static/js/app.js 400a4286d9358c699453326d87a0bd49dbb105a7 
  src/webui/master/static/js/controllers.js 
29a5a1c8754cc2fb934854750d6dfb04f1eaeae4 

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


Testing
---


Thanks,

haosdent huang

Review Request 52520: Exposed the executor's type in the endpoints.

[haosdent huang]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Exposed the executor's type in the endpoints.


Diffs
-

  src/common/http.cpp 538330a4c780fbc2dfcdfb31537b0e75f368e3e0 
  src/slave/http.cpp bdafe1db9f1b22ac0761f5119e73f87b531c6a3c 
  src/tests/default_executor_tests.cpp 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

[haosdent huang]

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

(Updated Oct. 4, 2016, 5:59 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This fixed the navigate error in Web UI because Web UI uses the
executor id of the task to search the corresponding sandbox directory.
Web UI uses the task id as the executor id if the executor id of the
task is empty when searching the sandbox directory. It works fine when
tasks are launched by `CommandExecutor` because the executor id of the
task is equal to the task id in this case. However, when tasks are
launched by `DefaultExecutor`, the executor id of the task is defined
in the framework side and may different with the task id. So we need to
fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
the Web UI uses incorrect executor id to search sandbox directory.


Diffs (updated)
-

  src/master/master.cpp 02a2fb29bdd8484fc90e5cb033ac29b49a141860 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Oct. 4, 2016, 5:54 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ad2070246b7fdb90aa6ed02326b5a7eb95365997 

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


Testing (updated)
---

make check on linux and macos


Thanks,

Megha Sharma

Re: Review Request 50327: Added scripts to build sample framework executables.

[Srinivas Brahmaroutu]

> On Oct. 3, 2016, 11:05 p.m., Joseph Wu wrote:
> > src/examples/CMakeLists.txt, lines 84-92
> > 
> >
> > Why are these necessary?  I could build a couple of the example 
> > frameworks after deleting these.

You are right, we can compile all example modules without additional libs or 
includes.


- Srinivas


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


On Oct. 4, 2016, 4:53 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50327/
> ---
> 
> (Updated Oct. 4, 2016, 4:53 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 scripts to build sample framework executables.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt ee8f565586b958929bc4e95b09827244d6d4155f 
>   cmake/MesosConfigure.cmake 6650c7c12b188b08c70cfee72b3200e83e7a1cd2 
>   src/CMakeLists.txt 35eb63fe9a8e47d97512e9904bf5a714c63722a7 
>   src/examples/CMakeLists.txt PRE-CREATION 
>   src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50327/diff/
> 
> 
> Testing
> ---
> 
> cmake .. && make dynamic-reservation-framework test-http-framework 
> test-framework test-executor test-http-executor long-lived-framework 
> long-lived-executor no-executor-framework docker-no-executor-framework 
> balloon-framework balloon-executor load-generator-framework 
> persistent-volume-framework
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 50327: Added scripts to build sample framework executables.

[Srinivas Brahmaroutu]

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

(Updated Oct. 4, 2016, 4:53 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 scripts to build sample framework executables.


Diffs (updated)
-

  CMakeLists.txt ee8f565586b958929bc4e95b09827244d6d4155f 
  cmake/MesosConfigure.cmake 6650c7c12b188b08c70cfee72b3200e83e7a1cd2 
  src/CMakeLists.txt 35eb63fe9a8e47d97512e9904bf5a714c63722a7 
  src/examples/CMakeLists.txt PRE-CREATION 
  src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 

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


Testing
---

cmake .. && make dynamic-reservation-framework test-http-framework 
test-framework test-executor test-http-executor long-lived-framework 
long-lived-executor no-executor-framework docker-no-executor-framework 
balloon-framework balloon-executor load-generator-framework 
persistent-volume-framework


Thanks,

Srinivas Brahmaroutu

Re: Review Request 52434: Improved handling of tmp file creation in health check test.

[Gastón Kleiman]

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

(Updated Oct. 4, 2016, 4:14 p.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


Changes
---

Revert the last revision, since it made the tests stop passing.


Repository: mesos


Description
---

The tmp file used in the HealthStatusChange test is now created inside
the tmp directory created for the test suite. This ensures that it will
be cleaned up on tear down.


Diffs (updated)
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---

make check GTEST_FILTER="HealthCheckTest.*" V=0


Thanks,

Gastón Kleiman

Re: Review Request 51946: Updated test for BadACLNoPrincipal and BadACLDropCreateAndDestroy.

[Greg Mann]

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


Ship it!




Thanks Guangya!

- Greg Mann


On Oct. 3, 2016, 9:53 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51946/
> ---
> 
> (Updated Oct. 3, 2016, 9:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Greg Mann, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6181
> https://issues.apache.org/jira/browse/MESOS-6181
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If destroy volume failed, we should get the latest offer to make
> sure that the latest offer contain the volume resource.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 2ab44d11d159162dfcac9d0791b651ed059b8164 
> 
> Diff: https://reviews.apache.org/r/51946/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh --gtest_filter="*PersistentVolumeTest.BadACL*/*"  
> --gtest_repeat=20
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 51299: Fixed memory leak in master during framework teardown.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Oct. 3, 2016, 8:54 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51299/
> ---
> 
> (Updated Oct. 3, 2016, 8:54 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Greg Mann, and Artem Harutyunyan.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `process::http::Pipe` class leaks its underlying `shared_ptr`
> due to how the master (accidentally) causes the `shared_ptr` to hold
> a self-reference.
> 
> When the master receives a `SUBSCRIBE` call from an V1 HTTP API
> framework, the master creates a `process::http::Pipe` to manage the
> reading and writing in separate locations in the code.  For 
> synchronization, the read/write ends of the HTTP connection share
> some state (via `shared_ptr`).
> 
> The master introduces a self-reference via this line in
> `Master::addFramework`:
> ```
>   http.closed()
> .onAny(defer(self(), &Self::exited, framework->id(), http));
> ```
> 
> `http.closed()` returns a future managed by the read-end of the `Pipe`.
> `http` holds a copy of the write-end of the `Pipe`, which in turn has
> a copy of the `shared_ptr`.  Because `http` is passed into the future's 
> continuation, a copy of `http` will live inside the read-end's future 
> until the either (a) the continuation is executed or (b) the future 
> is destructed.
> 
> Due to how we manage the future, neither (a) nor (b) are performed:
> (a) When the read-end of the `Pipe` is closed, we only complete the
> future if the write-end of the `Pipe` is still open.  During
> framework teardown, the write-end is closed first.
> (b) The future in question lives inside a `Promise`, which lives
> inside the `shared_ptr`.  Because the self-reference exists, the
> `shared_ptr` is never destructed; which means the `Promise` and
> future are never destructed either.
> 
> ---
> 
> This patch fixes the leak by making sure the continuation is always
> executed (a) or discarded.  It does so by discarding the related 
> future when the write-end of the `Pipe` is already closed.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 404196bb198c1ff958b55d72fb29c5fe92dba429 
>   3rdparty/libprocess/src/http.cpp 298bd460afdeccd18e201d8a505b961fd4cf3d3e 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 2538f56c88f1c6074a0d41182a2242a6fdd105f4 
> 
> Diff: https://reviews.apache.org/r/51299/diff/
> 
> 
> Testing
> ---
> 
> Found this leak in collaboration with Greg :)
> 
> Ran valgrind before applying this patch:
> ```
> LD_RUN_PATH=/path/to/mesos/build/src/.libs 
> LD_LIBRARY_PATH=/path/to/mesos/build/src/.libs valgrind --leak-check=full 
> src/.libs/mesos-tests --gtest_filter="*SchedulerTest.Teardown*"
> ```
> 
> Observed the following leak (among many false positives):
> ```
> 1,881 (216 direct, 1,665 indirect) bytes in 1 blocks are definitely lost in 
> loss record 2,405 of 2,507
>at 0x4C2A105: operator new(unsigned long) (in 
> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>by 0xF259A9: process::http::Pipe::Pipe() (http.hpp:414)
>by 0x5D629A1: 
> mesos::internal::master::Master::Http::scheduler(process::http::Request 
> const&, Option const&) const (http.cpp:796)
>by 0x5DB5806: operator() (master.cpp:858)
>...
> ```
> 
> Applied the patch and re-ran valgrind.  Confirmed that leak is gone.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 51752: Add event for `FRAMEWORK_REMOVED`.

[Zhitao Li]

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

(Updated Oct. 4, 2016, 3:23 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


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


Repository: mesos


Description
---

Add event for `FRAMEWORK_REMOVED`.


Diffs
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 
  src/common/protobuf_utils.hpp 3324838f0a54491024b3e26ddf38afbfad31b8e4 
  src/common/protobuf_utils.cpp 8c4a7264080385789157703b05b42716fb0563b3 
  src/master/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 
  src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 50271: Created an isolator for Linux capabilities.

[Benjamin Bannier]

> On Oct. 2, 2016, 2:22 a.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, lines 665-682
> > 
> >
> > I am wondering if we can parameterize this as well?

Yes we can :D

Updated the RR to this effect.


- Benjamin


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


On Oct. 4, 2016, 5:03 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50271/
> ---
> 
> (Updated Oct. 4, 2016, 5:03 p.m.)
> 
> 
> Review request for mesos, Jay Guo and Jie Yu.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This isolator evaluates agent allowed capabilities and passes net
> capabilities on to `mesos-containerizer` which enforces the
> capabilities.
> 
> Capability information is passed via a new field in
> `ContainerLaunchInfo`.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 86d1859854b44f237ac2ca1ef74982b543c08d25 
>   src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 
>   src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e6bd9f7a8284d220be157a3db2da094e6b1b6d33 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.hpp 
> a9b6ee2f0da2c1c84c6a0642adbce20a9f0218dc 
>   src/slave/containerizer/mesos/launch.cpp 
> 7dd10d5030260fbfdf7396a7c05a52b7c3d983e8 
>   src/tests/containerizer/capabilities_isolator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50271/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> `make` (OS X, clang-4.0 w/o optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 50271: Created an isolator for Linux capabilities.

[Benjamin Bannier]

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

(Updated Oct. 4, 2016, 5:03 p.m.)


Review request for mesos, Jay Guo and Jie Yu.


Changes
---

Parametrize test for task with image as well.


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


Repository: mesos


Description
---

This isolator evaluates agent allowed capabilities and passes net
capabilities on to `mesos-containerizer` which enforces the
capabilities.

Capability information is passed via a new field in
`ContainerLaunchInfo`.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
86d1859854b44f237ac2ca1ef74982b543c08d25 
  src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 
  src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
  src/slave/containerizer/mesos/containerizer.cpp 
e6bd9f7a8284d220be157a3db2da094e6b1b6d33 
  src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
  src/slave/containerizer/mesos/launch.hpp 
a9b6ee2f0da2c1c84c6a0642adbce20a9f0218dc 
  src/slave/containerizer/mesos/launch.cpp 
7dd10d5030260fbfdf7396a7c05a52b7c3d983e8 
  src/tests/containerizer/capabilities_isolator_tests.cpp PRE-CREATION 

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


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)
`make` (OS X, clang-4.0 w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 52357: Improved handling of health check failures within the grace period.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52357]

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 Oct. 4, 2016, 1:18 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52357/
> ---
> 
> (Updated Oct. 4, 2016, 1:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-6170
> https://issues.apache.org/jira/browse/MESOS-6170
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The health check library would ignore all failures until after the end
> of the grace period.
> 
> This behaviour was misleading. With the changes in this commit, once a
> health check succeeds for the first time, the grace period rule for
> failures is not be applied any more.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> ea93132f2a5d4828c75005f102eddc4c3131599d 
>   src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 
> 
> Diff: https://reviews.apache.org/r/52357/diff/
> 
> 
> Testing
> ---
> 
> Manual tests using Marathon.
> Added a new unit test and verified that it fails without the change, but 
> succeeds with it.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52515: Implement `AGENT_ADDED` and `AGENT_REMOVED` events.

[Zhitao Li]

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

(Updated Oct. 4, 2016, 2:45 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


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


Repository: mesos


Description
---

Implement `AGENT_ADDED` and `AGENT_REMOVED` events.


Diffs
-

  src/common/protobuf_utils.hpp 0c6405f6cfc5a69fa835428fa330d8d63778f469 
  src/common/protobuf_utils.cpp 1f088684c566f89c85dd72eaece295c5e3d50b23 
  src/master/master.cpp 756ab546851952bc1de24b1f469d232237b1c01c 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 51774: Add `AGENT_ADDED` and `AGENT_REMOVED` master events.

[Zhitao Li]

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

(Updated Oct. 4, 2016, 2:45 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


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


Repository: mesos


Description
---

Add `AGENT_ADDED` and `AGENT_REMOVED` master events.


Diffs
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 

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


Testing
---

New unit test.


Thanks,

Zhitao Li

Re: Review Request 52516: Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.

[Zhitao Li]

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

(Updated Oct. 4, 2016, 2:44 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


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


Repository: mesos


Description
---

Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.


Diffs
-

  src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 51752: Add event for `FRAMEWORK_REMOVED`.

[Zhitao Li]

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

(Updated Oct. 4, 2016, 2:44 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


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


Repository: mesos


Description
---

Add event for `FRAMEWORK_REMOVED`.


Diffs
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 
  src/common/protobuf_utils.hpp 3324838f0a54491024b3e26ddf38afbfad31b8e4 
  src/common/protobuf_utils.cpp 8c4a7264080385789157703b05b42716fb0563b3 
  src/master/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 
  src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 51700: Add `FrameworkAdded` event to master event stream.

[Zhitao Li]

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

(Updated Oct. 4, 2016, 2:44 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


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


Repository: mesos


Description
---

Changes included:
- Moving `model()` helper function to common/protobuf_utils;
- Definition of event and response;
- Sending event from master;
- new test.


Diffs
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 
  src/common/protobuf_utils.hpp 3324838f0a54491024b3e26ddf38afbfad31b8e4 
  src/common/protobuf_utils.cpp 8c4a7264080385789157703b05b42716fb0563b3 
  src/master/http.cpp 52dd80b856cf2317c0b73ba54bf501696786088d 
  src/master/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 
  src/tests/api_tests.cpp e440d1b44fb0b40dd1f68209c71e5ca0cd19f4fb 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 52516: Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.

[Zhitao Li]

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

(Updated Oct. 4, 2016, 2:43 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


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


Repository: mesos


Description
---

Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.


Diffs
-

  src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 51774: Add `AGENT_ADDED` and `AGENT_REMOVED` master events.

[Zhitao Li]

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

(Updated Oct. 4, 2016, 2:43 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


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


Repository: mesos


Description
---

Add `AGENT_ADDED` and `AGENT_REMOVED` master events.


Diffs
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 

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


Testing
---

New unit test.


Thanks,

Zhitao Li

Review Request 52516: Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.

[Zhitao Li]

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

Review request for mesos.


Repository: mesos


Description
---

Add test for `AGENT_ADDED` and `AGENT_REMOVED` events.


Diffs
-

  src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 

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


Testing
---


Thanks,

Zhitao Li

Review Request 52515: Implement `AGENT_ADDED` and `AGENT_REMOVED` events.

[Zhitao Li]

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

Review request for mesos.


Repository: mesos


Description
---

Implement `AGENT_ADDED` and `AGENT_REMOVED` events.


Diffs
-

  src/common/protobuf_utils.hpp 0c6405f6cfc5a69fa835428fa330d8d63778f469 
  src/common/protobuf_utils.cpp 1f088684c566f89c85dd72eaece295c5e3d50b23 
  src/master/master.cpp 756ab546851952bc1de24b1f469d232237b1c01c 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 51774: Add `AGENT_ADDED` and `AGENT_REMOVED` master events.

[Zhitao Li]

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

(Updated Oct. 4, 2016, 2:41 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


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


Repository: mesos


Description
---

Add `AGENT_ADDED` and `AGENT_REMOVED` master events.


Diffs
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 

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


Testing
---

New unit test.


Thanks,

Zhitao Li

Re: Review Request 51774: Add `AGENT_ADDED` and `AGENT_REMOVED` master events.

[Zhitao Li]

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

(Updated Oct. 4, 2016, 2:41 p.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, haosdent huang, and 
Vinod Kone.


Changes
---

rebase off dependency with protobuf change only.


Summary (updated)
-

Add `AGENT_ADDED` and `AGENT_REMOVED` master events.


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


Repository: mesos


Description (updated)
---

Add `AGENT_ADDED` and `AGENT_REMOVED` master events.


Diffs (updated)
-

  include/mesos/master/master.proto 1ea79fd7ecd9c2b7e94718fc8c7779d1051583db 
  include/mesos/v1/master/master.proto 58dc65006658f47bb752ee1c0c1520ee3cc03bd7 

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


Testing
---

New unit test.


Thanks,

Zhitao Li

Re: Review Request 51607: Used mesos-tcp-connect binary in TCP health checks.

[Alexander Rukletsov]

> On Sept. 6, 2016, 6:54 p.m., Avinash sridharan wrote:
> > src/health-check/health_checker.cpp, line 117
> > 
> >
> > Shouldn't we check is this directory exists before creating this 
> > `HealthCheckerProcess`?
> 
> Alexander Rukletsov wrote:
> Probably not, we don't do it in `launchTaskPosix()`, moreover this flag 
> is required for executors.

After a second thought, this should be a check then.


- Alexander


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


On Oct. 4, 2016, 1:57 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51607/
> ---
> 
> (Updated Oct. 4, 2016, 1:57 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Benjamin Mahler, Gastón Kleiman, 
> and haosdent huang.
> 
> 
> Bugs: MESOS-6119
> https://issues.apache.org/jira/browse/MESOS-6119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp ab3f0473fdc9105d1c425f0dbe7b81c566d541e8 
>   src/health-check/health_checker.hpp 
> 392b4d5bd1e5831994b9366c1eb5a2911e19860f 
>   src/health-check/health_checker.cpp 
> ea93132f2a5d4828c75005f102eddc4c3131599d 
>   src/launcher/executor.cpp 8a1051b886a0c95d19ff370e5c77d9c4033c8b61 
> 
> Diff: https://reviews.apache.org/r/51607/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.11.6
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 51609: Updated formatting in HealthChecker for consistency.

[Alexander Rukletsov]

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

(Updated Oct. 4, 2016, 1:58 p.m.)


Review request for mesos, Gastón Kleiman and haosdent huang.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/health-check/health_checker.cpp ea93132f2a5d4828c75005f102eddc4c3131599d 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 51608: Extracted "curl" binary into HTTP_CHECK_COMMAND constant.

[Alexander Rukletsov]

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

(Updated Oct. 4, 2016, 1:58 p.m.)


Review request for mesos, Gastón Kleiman and haosdent huang.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/health-check/health_checker.cpp ea93132f2a5d4828c75005f102eddc4c3131599d 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 51607: Used mesos-tcp-connect binary in TCP health checks.

[Alexander Rukletsov]

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

(Updated Oct. 4, 2016, 1:57 p.m.)


Review request for mesos, Avinash sridharan, Benjamin Mahler, Gastón Kleiman, 
and haosdent huang.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/docker/executor.cpp ab3f0473fdc9105d1c425f0dbe7b81c566d541e8 
  src/health-check/health_checker.hpp 392b4d5bd1e5831994b9366c1eb5a2911e19860f 
  src/health-check/health_checker.cpp ea93132f2a5d4828c75005f102eddc4c3131599d 
  src/launcher/executor.cpp 8a1051b886a0c95d19ff370e5c77d9c4033c8b61 

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


Testing
---

make check on Mac OS 10.11.6


Thanks,

Alexander Rukletsov

Re: Review Request 51606: Libprocess: Added target for "mesos-tcp-connect" binary.

[Alexander Rukletsov]

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

(Updated Oct. 4, 2016, 1:55 p.m.)


Review request for mesos, Gastón Kleiman and haosdent huang.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
3ef9040e9fe37058886aea0480582886c255ccb8 

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


Testing
---

See https://reviews.apache.org/r/51607/


Thanks,

Alexander Rukletsov

Re: Review Request 51605: Added "mesos-tcp-connect" binary.

[Alexander Rukletsov]

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

(Updated Oct. 4, 2016, 1:54 p.m.)


Review request for mesos, Avinash sridharan, Benjamin Mahler, Gastón Kleiman, 
and haosdent huang.


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


Repository: mesos


Description
---

To remove dependency on `bash` for TCP health checks, introduce
a separate light-weight binary (without libmesos dependency) for
probing TCP connections.


Diffs (updated)
-

  src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 
  src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
  src/health-check/CMakeLists.txt PRE-CREATION 
  src/health-check/tcp_connect.cpp PRE-CREATION 

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


Testing
---

See https://reviews.apache.org/r/51607/


Thanks,

Alexander Rukletsov

Re: Review Request 52434: Improved handling of tmp file creation in health check test.

[Gastón Kleiman]

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

(Updated Oct. 4, 2016, 1:40 p.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


Changes
---

Use a static path for the tmp file.


Repository: mesos


Description
---

The tmp file used in the HealthStatusChange test is now created inside
the tmp directory created for the test suite. This ensures that it will
be cleaned up on tear down.


Diffs (updated)
-

  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---

make check GTEST_FILTER="HealthCheckTest.*" V=0


Thanks,

Gastón Kleiman

Re: Review Request 52357: Improved handling of health check failures within the grace period.

[Gastón Kleiman]

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

(Updated Oct. 4, 2016, 1:18 p.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


Changes
---

Use a static name for the tmp file created by the new test in the suite's 
temporary directory.


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


Repository: mesos


Description
---

The health check library would ignore all failures until after the end
of the grace period.

This behaviour was misleading. With the changes in this commit, once a
health check succeeds for the first time, the grace period rule for
failures is not be applied any more.


Diffs (updated)
-

  src/health-check/health_checker.cpp ea93132f2a5d4828c75005f102eddc4c3131599d 
  src/tests/health_check_tests.cpp f4a63e2f5def022bf4469920861e6a5af229af96 

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


Testing
---

Manual tests using Marathon.
Added a new unit test and verified that it fails without the change, but 
succeeds with it.


Thanks,

Gastón Kleiman

Re: Review Request 50271: Created an isolator for Linux capabilities.

[Benjamin Bannier]

> On Oct. 2, 2016, 2:22 a.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, line 378
> > 
> >
> > I suggest we take `Option>` here so that the caller 
> > can do:
> > ```
> > TestConfig(
> > {NET_RAW, SYS_ADMIN},
> > {...},
> > ...
> > )
> > ```

I have changed this to now take `Option>` since that's what 
`capabilities::convert` accepts.


> On Oct. 2, 2016, 2:22 a.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, line 381
> > 
> >
> > I would simply this to be a bool: if the task should finish normally or 
> > not.
> > 
> > In the test, I would watch for terminal status update and see if it's 
> > TASK_FINISHED or not.

With the test in its current form this cannot be a `bool` since a task might 
fail in different ways (failure to start, or return with a failure when run). 
Depending on the number of task status updates we expect we do `EXPECT` 
differently.

In its current form we can be both explicit about when we expect a task to 
fail, and also avoid introducing an extra parameter storing the number of task 
status updates we expect. Can we leave it like is?


> On Oct. 2, 2016, 2:22 a.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, lines 573-578
> > 
> >
> > This can be
> > ```
> > TestConfig({}, None(), false)
> > ```

See above.


> On Oct. 2, 2016, 2:22 a.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, lines 657-659
> > 
> >
> > Should we fix those?

In fact I already did (https://reviews.apache.org/r/51931/), but failed to 
remove the `FIXME`.


> On Oct. 2, 2016, 2:22 a.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, line 380
> > 
> >
> > I would remove this and leave a TODO.

We currently have a test for the base case relying on this (the first one in 
`NoCapabilitiesConfigured`). Is your suggestion to remove that test case?


- Benjamin


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


On Oct. 4, 2016, 3:08 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50271/
> ---
> 
> (Updated Oct. 4, 2016, 3:08 p.m.)
> 
> 
> Review request for mesos, Jay Guo and Jie Yu.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This isolator evaluates agent allowed capabilities and passes net
> capabilities on to `mesos-containerizer` which enforces the
> capabilities.
> 
> Capability information is passed via a new field in
> `ContainerLaunchInfo`.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 86d1859854b44f237ac2ca1ef74982b543c08d25 
>   src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 
>   src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e6bd9f7a8284d220be157a3db2da094e6b1b6d33 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.hpp 
> a9b6ee2f0da2c1c84c6a0642adbce20a9f0218dc 
>   src/slave/containerizer/mesos/launch.cpp 
> 7dd10d5030260fbfdf7396a7c05a52b7c3d983e8 
>   src/tests/containerizer/capabilities_isolator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50271/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> `make` (OS X, clang-4.0 w/o optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52081: Reorganized includes in containerizer.

[Benjamin Bannier]

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

(Updated Oct. 4, 2016, 3:08 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


Repository: mesos


Description
---

Reorganized includes in containerizer.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
e6bd9f7a8284d220be157a3db2da094e6b1b6d33 

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


Testing
---

make check


Thanks,

Benjamin Bannier

Re: Review Request 50271: Created an isolator for Linux capabilities.

[Benjamin Bannier]

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

(Updated Oct. 4, 2016, 3:08 p.m.)


Review request for mesos, Jay Guo and Jie Yu.


Changes
---

Addressed on round of comments from Jie.


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


Repository: mesos


Description
---

This isolator evaluates agent allowed capabilities and passes net
capabilities on to `mesos-containerizer` which enforces the
capabilities.

Capability information is passed via a new field in
`ContainerLaunchInfo`.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
86d1859854b44f237ac2ca1ef74982b543c08d25 
  src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 
  src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
  src/slave/containerizer/mesos/containerizer.cpp 
e6bd9f7a8284d220be157a3db2da094e6b1b6d33 
  src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
  src/slave/containerizer/mesos/launch.hpp 
a9b6ee2f0da2c1c84c6a0642adbce20a9f0218dc 
  src/slave/containerizer/mesos/launch.cpp 
7dd10d5030260fbfdf7396a7c05a52b7c3d983e8 
  src/tests/containerizer/capabilities_isolator_tests.cpp PRE-CREATION 

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


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)
`make` (OS X, clang-4.0 w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 52486: Decoupled QueuedTasks and QueuedTaskGroups on the agent.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52486]

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 Oct. 4, 2016, 5:44 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52486/
> ---
> 
> (Updated Oct. 4, 2016, 5:44 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6298
> https://issues.apache.org/jira/browse/MESOS-6298
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Decoupled QueuedTasks and QueuedTaskGroups on the agent.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp bdafe1db9f1b22ac0761f5119e73f87b531c6a3c 
>   src/slave/slave.cpp fba089506850eec87f67a46694a1ef645cdf4ad5 
> 
> Diff: https://reviews.apache.org/r/52486/diff/
> 
> 
> Testing
> ---
> 
> make -j`nproc` check
> 
> Manually ran "sleep 1000" and "echo hello" with mesos-execute for both single 
> task and task group.
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 51605: Added "mesos-tcp-connect" binary.

[Alexander Rukletsov]

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

(Updated Oct. 4, 2016, 1:04 p.m.)


Review request for mesos, Avinash sridharan, Benjamin Mahler, Gastón Kleiman, 
and haosdent huang.


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


Repository: mesos


Description
---

To remove dependency on `bash` for TCP health checks, introduce
a separate light-weight binary (without libmesos dependency) for
probing TCP connections.


Diffs (updated)
-

  src/CMakeLists.txt ba49d81335fd47a8ced334e282eadb00657bf2c2 
  src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
  src/health-check/CMakeLists.txt PRE-CREATION 
  src/health-check/tcp_connect.cpp PRE-CREATION 

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


Testing
---

See https://reviews.apache.org/r/51607/


Thanks,

Alexander Rukletsov

Re: Review Request 51606: Libprocess: Added target for "mesos-tcp-connect" binary.

[Alexander Rukletsov]

> On Sept. 29, 2016, 4:35 p.m., Benjamin Bannier wrote:
> > Why does this need to be an extra commit? It appears it should just be 
> > folded into https://reviews.apache.org/r/51605/ which adds this target for 
> > the automake setup.
> 
> Alexander Rukletsov wrote:
> Because it's in libprocess.
> 
> Benjamin Bannier wrote:
> Of course, sorry for not looking at the original file more carefully.
> 
> This does follow the existing pattern, but I am not sure what this 
> pattern accomplishes, e.g., why do these mesos-specific target names need to 
> be defined under libprocess, while all their sources and users are in a 
> directory above? This not only is inconsistent with what we do in the 
> automake world (OK, not not a perfect setup either), but also appears to be 
> contrary to our approach of self-contained 3rdparty components.
> 
> Could you please file a tech debt cleanup ticket to fix this?

Good point, Benjamin. Filed https://issues.apache.org/jira/browse/MESOS-6309


- Alexander


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


On Sept. 2, 2016, 4:42 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51606/
> ---
> 
> (Updated Sept. 2, 2016, 4:42 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and haosdent huang.
> 
> 
> Bugs: MESOS-6119
> https://issues.apache.org/jira/browse/MESOS-6119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake 
> f7cba60c9ef7c7ed27819dcf4939c0c51f80d49e 
> 
> Diff: https://reviews.apache.org/r/51606/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/51607/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 52499: Removed a redundant space from Appc fetcher.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52499]

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 Oct. 4, 2016, 1:26 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52499/
> ---
> 
> (Updated Oct. 4, 2016, 1:26 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a redundant space from Appc fetcher.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp 
> 907e761856e8996b72e4231de27fa15e884d52e3 
> 
> Diff: https://reviews.apache.org/r/52499/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 51879: Autodetect value of resource when not specified in static resources.

[Guangya Liu]

> On 九月 28, 2016, 10:37 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 587-618
> > 
> >
> > No need for the helpers. Just the following is sufficient.
> > 
> > ```
> > Resource resource;
> > resource.set_name(name);
> > resource.set_role(role);
> > 
> > // Return a Resource with missing value.
> > if (_value.isNone()) {
> >   return resource;
> > }
> > 
> > Value _value = result.get();
> > 
> > if (_value.type() == Value::SCALAR) {
> >   resource.set_type(Value::SCALAR);
> >   resource.mutable_scalar()->CopyFrom(_value.scalar());
> > } else if (_value.type() == Value::RANGES) {
> >   resource.set_type(Value::RANGES);
> >   resource.mutable_ranges()->CopyFrom(_value.ranges());
> > } else if (_value.type() == Value::SET) {
> >   resource.set_type(Value::SET);
> >   resource.mutable_set()->CopyFrom(_value.set());
> > } else {
> >   return Error(
> >   "Bad type for resource " + name + " value " + value +
> >   " type " + Value::Type_Name(_value.type()));
> > }
> > ```

Seems we still need the `type` for resource when `_value.isNone()`, so the 
logic could be:

```
Resource resource;
resource.set_name(name);
resource.set_role(role);

// Return a Resource with missing value.
if (_value.isNone()) {
  Try _type = Resources::valueType(name);
  if (_type.isError()) {
return Error(_type.error());
  }
  
  resource.set_type(_type.get());
  return resource;
}

Value _value = result.get();

if (_value.type() == Value::SCALAR) {
  resource.set_type(Value::SCALAR);
  resource.mutable_scalar()->CopyFrom(_value.scalar());
} else if (_value.type() == Value::RANGES) {
  resource.set_type(Value::RANGES);
  resource.mutable_ranges()->CopyFrom(_value.ranges());
} else if (_value.type() == Value::SET) {
  resource.set_type(Value::SET);
  resource.mutable_set()->CopyFrom(_value.set());
} else {
  return Error(
  "Bad type for resource " + name + " value " + value +
  " type " + Value::Type_Name(_value.type()));
}
```


- Guangya


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


On 十月 3, 2016, 11:46 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> ---
> 
> (Updated 十月 3, 2016, 11:46 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When static resources indicate resources with a positive size, we use
> that for the resources on the agent. However, --resources can include
> resources with no size, which indicates that mesos agent determine the
> size of those resources from the agent and uses that information. For
> disk resources, this is not allowed for PATH disks since PATH disks
> can be carved into smaller chunks and we cannot assume that the whole
> physical disk is available to the PATH disk.
> 
> With this change, JSON or textual representation for disk resources
> that specify scalar value of 0 would not result in an error, but those
> resources will not be accounted for until a valid size is determined
> for such resources. A scalar value of -1 in JSON or textual formats
> still results in an invalid resource.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.hpp 
> b2eabfebef99ccebef427d144bb816adc0175ecf 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
> 2e722691475c84afae14009014ea70cc0fdd0e65 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51879/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52071: Updated docs to handle resources with no size in agent flags.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51999, 52002, 51879, 51880, 52071]

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 Oct. 3, 2016, 11:46 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52071/
> ---
> 
> (Updated Oct. 3, 2016, 11:46 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a resource with no size is specified in `--resources` startup
> flag of mesos agent, the size is auto detected by the agent. This
> is enabled for all known resource types (except gpus).
> For disk resources, this is done for both root disks as well as
> MOUNT disks, but is not allowed for PATH disks since PATH disks can
> be carved into smaller volumes.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa 
> 
> Diff: https://reviews.apache.org/r/52071/diff/
> 
> 
> Testing
> ---
> 
> Documentation change only. All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>