Re: Review Request 58202: Tweaked an incorrect comment in allocator test.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On April 5, 2017, 8:56 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58202/
> ---
> 
> (Updated April 5, 2017, 8:56 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6940
> https://issues.apache.org/jira/browse/MESOS-6940
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tweaked an incorrect comment in allocator test.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> e343dc37bd7136f0f6dd5dbc22a25cabe715038d 
> 
> 
> Diff: https://reviews.apache.org/r/58202/diff/1/
> 
> 
> Testing
> ---
> 
> no functional change
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58202: Tweaked an incorrect comment in allocator test.

[Benjamin Mahler]

> On June 26, 2017, 7 a.m., Benjamin Mahler wrote:
> > Ship It!

I changed the summary to: "Fixed an incorrect comment about MULTI_ROLE in the 
allocator tests."


- Benjamin


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


On April 5, 2017, 8:56 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58202/
> ---
> 
> (Updated April 5, 2017, 8:56 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6940
> https://issues.apache.org/jira/browse/MESOS-6940
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tweaked an incorrect comment in allocator test.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> e343dc37bd7136f0f6dd5dbc22a25cabe715038d 
> 
> 
> Diff: https://reviews.apache.org/r/58202/diff/1/
> 
> 
> Testing
> ---
> 
> no functional change
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58552: Resolved a TODO of MULTI_ROLE.

[Jay Guo]

> On June 26, 2017, 2:55 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Line 2838 (original), 2838-2839 (patched)
> > 
> >
> > Rather than the comment, we should just call this function 
> > allocatedResources so that there's no need for the comment.
> > 
> > I'll remove the comment in the commit, do you want to send another 
> > patch to rename it to `allocatedResources()` for clarity?

https://reviews.apache.org/r/60423/


- Jay


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


On April 20, 2017, 11 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58552/
> ---
> 
> (Updated April 20, 2017, 11 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> After the completion of MULTI_ROLE support, allocation_info is always
> populated, some redundant logic for resources of old format are not
> necessary anymore, therefore removed.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
> 
> 
> Diff: https://reviews.apache.org/r/58552/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Review Request 60423: Renamed method `resources` to `allocatedResources` in master::Role.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

This method collects resources allocated to the particular role.
It's more precise to rename it as such.


Diffs
-

  src/master/http.cpp 4dd43fd7c3fb986f4eed78bce574b6d3af156b67 
  src/master/master.hpp 9dd6a530c373516dc81bfd51ee6e95f25588148f 


Diff: https://reviews.apache.org/r/60423/diff/1/


Testing
---

no functional change


Thanks,

Jay Guo

Re: Review Request 60233: Linted support/post-reviews.py.

[Benjamin Bannier]

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


Fix it, then Ship it!





support/post-reviews.py
Line 20 (original), 20 (patched)


Can we add a one-line PEP257-conformant summary?



support/post-reviews.py
Lines 37 (patched)


Let's quote this docstring with `r"""` since we have backslashes here.



support/post-reviews.py
Lines 59 (patched)


`Execute a process and leave.`, see PEP257.



support/post-reviews.py
Line 48 (original), 69 (patched)


Do we need to name the exception here? It seems the original `raise` was 
sufficient.



support/post-reviews.py
Line 55 (original), 76 (patched)


This pattern seems not straight-forward to me, could we just use a 
multiline string here, e.g.,

need_login = 'Please log in to the Review Board' \
 ' server at reviews.apache.org.'



support/post-reviews.py
Lines 94 (patched)


Can we add a PEP257-conformant summary line here, e.g.,

Execute main function.



support/post-reviews.py
Line 282 (original), 316 (patched)


Let's just continue the line with `\`, e.g.,

if rbt_command in post_review and \
   rbt_version >= LooseVersion('RBTools 0.6'):



support/post-reviews.py
Lines 321 (patched)


Continue with `\`, e.g.,

if rbt_version >= LooseVersion('RBTools 0.6.1') and \
   parent_review_request_id:


- Benjamin Bannier


On June 20, 2017, 12:15 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60233/
> ---
> 
> (Updated June 20, 2017, 12:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/post-reviews.py 410fb3371a3c46adbfd68c7584ffd6cf3b3010d1 
> 
> 
> Diff: https://reviews.apache.org/r/60233/diff/1/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60234: Linted support/push-commits.py.

[Benjamin Bannier]

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


Fix it, then Ship it!





support/push-commits.py
Lines 20 (patched)


Could we add a one-line, PEP-257-conformant summary line here?



support/push-commits.py
Line 28 (original), 46 (patched)


Since we are cleaning up, could we make this PEP257-conformant (this could 
also make it fit on a single line), e.g.,

"""Return the list of reviews found in the commits in the revision 
range."""



support/push-commits.py
Line 55 (original), 73 (patched)


The old wording seemed better to me, but I am not a native speaker. Also, 
PEP-257 suggests `s/Marks/Mark/`.



support/push-commits.py
Line 65 (original), 83 (patched)


Since we are cleaning up, can we make this PEP257-conformant, e.g.,

"""Return a dictionary of options parsed from command line arguments."""



support/push-commits.py
Lines 101 (patched)


Can we add a one-line, PEP257-conformant summary here?


- Benjamin Bannier


On June 20, 2017, 12:15 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60234/
> ---
> 
> (Updated June 20, 2017, 12:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/push-commits.py 4835d67393aaf3a38a1da90ccc7c89a952bf9270 
> 
> 
> Diff: https://reviews.apache.org/r/60234/diff/1/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60235: Linted support/test-upgrade.py.

[Benjamin Bannier]

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


Fix it, then Ship it!





support/test-upgrade.py
Lines 34 (patched)


Could we add a one-line, PEP257-conformant summary here, e.g.,

"""
Helper class to keep track of process lifecycles.

This class allows to i.e., start processes, capture their output,
and checking their liveness during delays/sleep.
"""

Also, add a blank line before declaring the first method.



support/test-upgrade.py
Lines 49 (patched)


Let's add a PEP257-conformant summary line, e.g.,

"""
Poll the process for the specified number of seconds.

We return the process's return value if it ends during that
time. Returns `True` if the process is still running after
that time period.
"""



support/test-upgrade.py
Line 48 (original), 69 (patched)


Add an empty line before declaring the first method.



support/test-upgrade.py
Line 60 (original), 79 (patched)


Add an empty line before declaring the first method.



support/test-upgrade.py
Line 75 (original), 91 (patched)


Add an empty line before declaring the first method.



support/test-upgrade.py
Lines 102-103 (original), 116-118 (patched)


Let's fit this onto a single line, e.g.,

"""Convenience function to get the Mesos version from built 
executables."""



support/test-upgrade.py
Line 119 (original), 134 (patched)


Let's add a one-line, PEP257-conformant summary, e.g.,

Main function.



support/test-upgrade.py
Line 138 (original), 158 (patched)


I think explicitly converting this `Boolean`s to `Boolean`s is pretty 
verbose, how about

if not prev_version or not next_version:



support/test-upgrade.py
Line 176 (original), 196 (patched)


Let's rely on the thruthiness of `sleep`'s return value, e.g.,

if not prev_master.sleep(0.5):



support/test-upgrade.py
Line 183 (original), 203 (patched)


Let's rely on the thruthiness of `sleep`'s return value, e.g.,

if not prev_agent.sleep(0.5):


- Benjamin Bannier


On June 20, 2017, 12:16 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60235/
> ---
> 
> (Updated June 20, 2017, 12:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 
> 
> 
> Diff: https://reviews.apache.org/r/60235/diff/1/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60353: Allowed dashes in Python modules.

[Benjamin Bannier]

> On June 22, 2017, 10:01 a.m., Armand Grillet wrote:
> > src/cli_new/pylint.config
> > Lines 16 (patched)
> > 
> >
> > I do not think that having a pylint.config for all our Python 
> > directories in src/cli_new is the most logical solution. We could move 
> > pylint.config to support/ in this review request or add a new PyLint 
> > configuration, including a new pylint.config, for these support scripts. It 
> > will require a change in the `run_lint` method of `PyLinter` to use 
> > different settings when linting the directories.
> > 
> > I would go for the second solution due to some errors currently 
> > returned by PyLint when linting files such as support/post-reviews.py, e.g. 
> > `E: 53, 0: No name 'version' in module 'distutils' (no-name-in-module)` 
> > that is likely due to the `virtualenv_dir` used by PyLint (the one used for 
> > `cli_new`). We are likely need at least two separate settings depending on 
> > what we lint: a standard Python module or a list of scripts.

+1.


- Benjamin


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


On June 22, 2017, 2:57 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60353/
> ---
> 
> (Updated June 22, 2017, 2:57 a.m.)
> 
> 
> Review request for mesos, Armand Grillet and Benjamin Bannier.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> None of our support scripts are intended to be used as modules, so
> the usage of dashes in the filenames are acceptable.  PyLint however
> considers dashes in module names (same as the filename) to be an error.
> 
> This commit also adds the `support` directory to the list of linted
> sources.
> 
> 
> Diffs
> -
> 
>   src/cli_new/pylint.config 63fbb94fbf47cccd1053b4c1eafac18b88dbc818 
>   support/mesos-style.py 48d816f72df65d77364769f812fc06afcd6b5aec 
> 
> 
> Diff: https://reviews.apache.org/r/60353/diff/1/
> 
> 
> Testing
> ---
> 
> This shouldn't be commited yet as a few support scripts don't pass the linter 
> yet.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 60425: Added MESOS-7581 to the 1.3.1 CHANGELOG.

[Benjamin Bannier]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Added MESOS-7581 to the 1.3.1 CHANGELOG.


Diffs
-

  CHANGELOG 7536717b85bb188d8a463b36ce75e4e1cecc445d 


Diff: https://reviews.apache.org/r/60425/diff/1/


Testing
---


Thanks,

Benjamin Bannier

Review Request 60426: Marked 1.2.2 as WIP in CHANGELOG.

[Benjamin Bannier]

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

Review request for mesos and Adam B.


Repository: mesos


Description
---

Marked 1.2.2 as WIP in CHANGELOG.


Diffs
-

  CHANGELOG 7536717b85bb188d8a463b36ce75e4e1cecc445d 


Diff: https://reviews.apache.org/r/60426/diff/1/


Testing
---


Thanks,

Benjamin Bannier

Review Request 60428: Added MESOS-7581 to the 1.1.3 CHANGELOG.

[Benjamin Bannier]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Added MESOS-7581 to the 1.1.3 CHANGELOG.


Diffs
-

  CHANGELOG 7536717b85bb188d8a463b36ce75e4e1cecc445d 


Diff: https://reviews.apache.org/r/60428/diff/1/


Testing
---


Thanks,

Benjamin Bannier

Review Request 60427: Added MESOS-7581 to the 1.2.2 CHANGELOG.

[Benjamin Bannier]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Added MESOS-7581 to the 1.2.2 CHANGELOG.


Diffs
-

  CHANGELOG 7536717b85bb188d8a463b36ce75e4e1cecc445d 


Diff: https://reviews.apache.org/r/60427/diff/1/


Testing
---


Thanks,

Benjamin Bannier

Re: Review Request 59294: Optionally scale egress bandwidth with CPU.

[Ilya Pronin]

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




src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 423-427 (original), 424-428 (patched)


I think this flag's semantics needs additional explanation. If it is set, 
but contains an empty JSON, then an existing HTB class is removed.



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 576 (patched)


Style: For functions the opening brace must be on a separate line.



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 617 (patched)


Above there's a check that verifies that `rate.isSome()`.



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 604-606 (original), 642-644 (patched)


The comment is a bit misleading. We're removing the whole qdisc here, not 
just a class.



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 2209-2210 (original), 2301-2302 (patched)


Do we expect the helper to hang? Is this a safeguard for the new code 
deployment?



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Line 2231 (original), 2318 (patched)


Spelling: s/stdout>/stdout/.



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp
Lines 4315-4317 (patched)


If we set `ceil` low, it will prevent heavy CPU users' from bursting. 
Wouldn't it be more flexible to make `egress_ceil_limit` flag additive instead 
of absolute?


- Ilya Pronin


On June 21, 2017, 9:48 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59294/
> ---
> 
> (Updated June 21, 2017, 9:48 p.m.)
> 
> 
> Review request for mesos, Dmitry Zhuk, Ilya Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-7508
> https://issues.apache.org/jira/browse/MESOS-7508
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support to isolators/port_mapping for optionally scaling egress bandwidth 
> with CPU and with minimum and maximum limits.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e2502acc3542d64c2b2b13f8c0363a3c8372e20e 
>   src/slave/containerizer/mesos/isolators/network/helper.cpp 
> 4ed879dca42f85fc9dd7638e763822cf10fa8405 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
> 9d38289c7161d5e931053b587d115684ccc44c94 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> cd008aaebcd42554a9a81d2b059269546f59c966 
>   src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c 
>   src/slave/flags.cpp c84aa6724170bba46bbe8410b71d42a1626e 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 16e015a8ac53a4aa5336b60c40228720b5f6910a 
> 
> 
> Diff: https://reviews.apache.org/r/59294/diff/4/
> 
> 
> Testing
> ---
> 
> # added a new test 
> $ make check
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 60008: Fixed bug causing FUTURE_DISPATCH to react on irrelevant dispatch (WIP).

[Alexander Rukletsov]

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




3rdparty/libprocess/include/process/dispatch.hpp
Lines 68 (patched)


Why don't you use `Option<>`?



3rdparty/libprocess/include/process/dispatch.hpp
Lines 149 (patched)


`canonicalizePointer`?



3rdparty/libprocess/include/process/event.hpp
Lines 178-179 (patched)


Please a blank line here, i.e.,
```
// Any society that would give up a little liberty to gain
// a little security will deserve neither and lose both.
//
// TODO(abudnik): Add a source link for the quote above.
```



3rdparty/libprocess/src/tests/process_tests.cpp
Line 189 (original), 190-192 (patched)


We strive to add a descriptive comment for each test. Even though it is not 
an easy to go and add comments to existing tests, we do want to have a proper 
description for every newly added test.



3rdparty/libprocess/src/tests/process_tests.cpp
Lines 204-205 (patched)


Why do you need this statement?



3rdparty/libprocess/src/tests/process_tests.cpp
Lines 208-209 (patched)


Additionally to (or actually instead of) `EXPECT` here, `AWAIT_READY(f);` 
would carry your intent better, I think. I would actually get rid of call 
expectations all together and simply focus on future states.


- Alexander Rukletsov


On June 16, 2017, 5:34 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60008/
> ---
> 
> (Updated June 16, 2017, 5:34 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-5886
> https://issues.apache.org/jira/browse/MESOS-5886
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> FUTURE_DISPATCH uses DispatchMatcher to figure out whether a processed
> DispatchEvent is the same the user is waiting for. Currently, we
> compare std::type_info of function pointers, which is not enough:
> different class methods with same signatures will be matched (see
> MESOS-5886 for an example).
> This patch adds value of pointer-to-member function in addition to
> std::type_info in DispatchEvent to uniquely identify class methods.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/dispatch.hpp 
> 3a0793888dc0df5e3ec31b06f47cd920c71e0db9 
>   3rdparty/libprocess/include/process/event.hpp 
> 8afe6266eb0dc5a17af35d79efb6bfdf9e6a0ee9 
>   3rdparty/libprocess/include/process/gmock.hpp 
> e9af943b39436f365fe687301febb5c7fbefffc4 
>   3rdparty/libprocess/src/process.cpp 
> 4ff7448d171f39dbb8cbb81dd9bed136ad43d62d 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 38d787a083a5eb31e922d283f4b4bed2bd62eb0a 
> 
> 
> Diff: https://reviews.apache.org/r/60008/diff/1/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
> 
> NOTE: Test GroupTest.ConnectTimer is broken, because it uses FUTURE_DISPATCH 
> on a wrong method (GroupProcess::expired),
> which has the same signature as a private method (GroupProcess::timedout). 
> The later method is actually called,
> thus causing the error after applying this patch.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60236: Linted support/verify-reviews.py.

[Armand Grillet]

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

(Updated June 26, 2017, 3:28 p.m.)


Review request for mesos, Benjamin Bannier and Joseph Wu.


Changes
---

Improvements using pep257.


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


Repository: mesos


Description
---

This will allow us to use PyLint on the
entire support directory in the future.


Diffs (updated)
-

  support/verify-reviews.py 391bef5c15a7399f037e54600d1b13c9bd261811 


Diff: https://reviews.apache.org/r/60236/diff/3/

Changes: https://reviews.apache.org/r/60236/diff/2-3/


Testing
---

Added `support` to `source_dirs` in the PyLinter defined
in `mesos-style.py`. Linted until only acceptable errors
where displayed (e.g. due to the name of the file containing
a dash instead of an underscore).


Thanks,

Armand Grillet

Re: Review Request 59746: Separated discarded and failed cases for container launch.

[Alexander Rukletsov]

> On June 2, 2017, 4:37 p.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Line 5147 (original), 5147 (patched)
> > 
> >
> > Can you explain to me in what scenario, the `future` will be in 
> > DISCARDED state? who discard the promise associated with this future?
> 
> Alexander Rukletsov wrote:
> Sure. Consider docker containerizer.
> 
> 1) During container launch, docker containerizer calls `pull()`: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/containerizer/docker.cpp#L1238
> 2) The container enters `PULLING` state: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/containerizer/docker.cpp#L435
> 3) While the image is being pulled by docker, future 
> `containers_[containerId]->pull` is returned from `pull()`: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/containerizer/docker.cpp#L446
> 4) This future is part of the `.then` chain returned from `_launch()`: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/containerizer/docker.cpp#L1269
> 5) Now while docker is pulling, `destroy()` is called, which discards the 
> "pulling future": 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/containerizer/docker.cpp#L2126-L2128
> 6) But discarding that future is propagated up the chain: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/3rdparty/libprocess/include/process/future.hpp#L1410-L1411
> 7) Which triggers the `onAny` callback attached to launch: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/slave.cpp#L2800-L2810
> 8) Which in turn gives us discarded future treated as launch error: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/slave.cpp#L5147-L5152
> 
> Jie Yu wrote:
> 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/containerizer/docker.cpp#L2126-L2128
> 
> This discards the future, but not necessarily transition the future to 
> DISCARDED state. That's the reason we have `hasDiscard` and `isDiscarded` 
> methods for Future becaue they means different things. Can you point to me 
> where the promise associated with this future is actually being transitioned 
> into DISCARDED state?
> 
> Alexander Rukletsov wrote:
> Sure. In this case, we discard pulling in case client discarded the 
> future: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/docker/docker.cpp#L1512
> 
> Additionally, I've manually reproduced the issue 
> (https://issues.apache.org/jira/browse/MESOS-7601)
> ```
> ./src/mesos-execute --master=192.99.40.208:5050 --containerizer=docker 
> --docker_image=ubuntu:16.04 --name=pull-test --command="sleep 1000"
> ```
> aborted right after the start when docker was pulling the image yielded 
> the following verbose agent log:
> ```
> I0621 12:59:22.271728 28980 fetcher.cpp:324] Starting to fetch URIs for 
> container: e2227d2f-fb6e-4fba-b6b6-528d2da7b276, directory: 
> /tmp/a/slaves/f123fa01-fe6e-45f4-b0a9-022b0fc3ce26-S0/frameworks/f123fa01-fe6e-45f4-b0a9-022b0fc3ce26-0003/executors/pull-test/runs/e2227d2f-fb6e-4fba-b6b6-528d2da7b276
> I0621 12:59:22.272665 28989 docker.cpp:1352] Running docker -H 
> unix:///var/run/docker.sock inspect ubuntu:16.04
> I0621 12:59:22.420902 28990 docker.cpp:1426] Running docker -H 
> unix:///var/run/docker.sock pull ubuntu:16.04
> I0621 12:59:23.070950 28980 slave.cpp:3130] Asked to shut down framework 
> f123fa01-fe6e-45f4-b0a9-022b0fc3ce26-0003 by master@192.99.40.208:5050
> I0621 12:59:23.071007 28980 slave.cpp:3155] Shutting down framework 
> f123fa01-fe6e-45f4-b0a9-022b0fc3ce26-0003
> I0621 12:59:23.071146 28980 slave.cpp:5625] Shutting down executor 
> 'pull-test' of framework f123fa01-fe6e-45f4-b0a9-022b0fc3ce26-0003
> W0621 12:59:23.071171 28980 slave.hpp:732] Unable to send event to 
> executor 'pull-test' of framework f123fa01-fe6e-45f4-b0a9-022b0fc3ce26-0003: 
> unknown connection type
> I0621 12:59:28.072532 28984 slave.cpp:5698] Killing executor 'pull-test' 
> of framework f123fa01-fe6e-45f4-b0a9-022b0fc3ce26-0003
> I0621 12:59:28.072849 28985 docker.cpp:2125] Destroying container 
> e2227d2f-fb6e-4fba-b6b6-528d2da7b276 in PULLING state
> I0621 12:59:28.073074 28985 docker.cpp:149] 'docker -H 
> unix:///var/run/docker.sock pull ubuntu:16.04' is being discarded
> E0621 12:59:28.150388 28981 slave.cpp:5183] Container 
> 'e2227d2f-fb6e-4fba-b6b6-528d2da7b276' for executor 'pull-test' of framework 
> f123fa01-fe6e-45f4-b0a9-022b0fc3ce26-0003 failed to start: future discarded
> E0621 12:59:28.150698 28978 slave.cpp:5290] Termination of executor 
> 'p

Re: Review Request 60233: Linted support/post-reviews.py.

[Armand Grillet]

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

(Updated June 26, 2017, 3:46 p.m.)


Review request for mesos, Benjamin Bannier and Joseph Wu.


Changes
---

Resolved issues.


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


Repository: mesos


Description
---

This will allow us to use PyLint on the
entire support directory in the future.


Diffs (updated)
-

  support/post-reviews.py 410fb3371a3c46adbfd68c7584ffd6cf3b3010d1 


Diff: https://reviews.apache.org/r/60233/diff/2/

Changes: https://reviews.apache.org/r/60233/diff/1-2/


Testing
---

Added `support` to `source_dirs` in the PyLinter defined
in `mesos-style.py`. Linted until only acceptable errors
where displayed (e.g. due to the name of the file containing
a dash instead of an underscore).


Thanks,

Armand Grillet

Re: Review Request 60233: Linted support/post-reviews.py.

[Armand Grillet]

> On June 26, 2017, 9:40 a.m., Benjamin Bannier wrote:
> > support/post-reviews.py
> > Line 48 (original), 69 (patched)
> > 
> >
> > Do we need to name the exception here? It seems the original `raise` 
> > was sufficient.

I cannot just do an `except:` above or PyLint returns `W: 66, 4: No exception 
type(s) specified (bare-except)`. I have changed the code to have a `except 
Exception as _:` followed by just a `raise`.


- Armand


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


On June 26, 2017, 3:46 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60233/
> ---
> 
> (Updated June 26, 2017, 3:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/post-reviews.py 410fb3371a3c46adbfd68c7584ffd6cf3b3010d1 
> 
> 
> Diff: https://reviews.apache.org/r/60233/diff/2/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60235: Linted support/test-upgrade.py.

[Armand Grillet]

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

(Updated June 26, 2017, 3:48 p.m.)


Review request for mesos, Benjamin Bannier and Joseph Wu.


Changes
---

Resolved issues and new function `test_case` to make the code simpler.


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


Repository: mesos


Description
---

This will allow us to use PyLint on the
entire support directory in the future.


Diffs (updated)
-

  support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 


Diff: https://reviews.apache.org/r/60235/diff/2/

Changes: https://reviews.apache.org/r/60235/diff/1-2/


Testing
---

Added `support` to `source_dirs` in the PyLinter defined
in `mesos-style.py`. Linted until only acceptable errors
where displayed (e.g. due to the name of the file containing
a dash instead of an underscore).


Thanks,

Armand Grillet

Re: Review Request 60234: Linted support/push-commits.py.

[Armand Grillet]

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

(Updated June 26, 2017, 3:49 p.m.)


Review request for mesos, Benjamin Bannier and Joseph Wu.


Changes
---

Resolved issues.


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


Repository: mesos


Description
---

This will allow us to use PyLint on the
entire support directory in the future.


Diffs (updated)
-

  support/push-commits.py 4835d67393aaf3a38a1da90ccc7c89a952bf9270 


Diff: https://reviews.apache.org/r/60234/diff/2/

Changes: https://reviews.apache.org/r/60234/diff/1-2/


Testing
---

Added `support` to `source_dirs` in the PyLinter defined
in `mesos-style.py`. Linted until only acceptable errors
where displayed (e.g. due to the name of the file containing
a dash instead of an underscore).


Thanks,

Armand Grillet

Re: Review Request 60426: Marked 1.2.2 as WIP in CHANGELOG.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On June 26, 2017, 3:42 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60426/
> ---
> 
> (Updated June 26, 2017, 3:42 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Marked 1.2.2 as WIP in CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 7536717b85bb188d8a463b36ce75e4e1cecc445d 
> 
> 
> Diff: https://reviews.apache.org/r/60426/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 60233: Linted support/post-reviews.py.

[Benjamin Bannier]

> On June 26, 2017, 11:40 a.m., Benjamin Bannier wrote:
> > support/post-reviews.py
> > Line 48 (original), 69 (patched)
> > 
> >
> > Do we need to name the exception here? It seems the original `raise` 
> > was sufficient.
> 
> Armand Grillet wrote:
> I cannot just do an `except:` above or PyLint returns `W: 66, 4: No 
> exception type(s) specified (bare-except)`. I have changed the code to have a 
> `except Exception as _:` followed by just a `raise`.

I didn't mean to catch all exceptions, we should still be specific, but 
wouldn't need to name the exception, e.g.,

try:
  foo()
except Exception:
  raise


- Benjamin


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


On June 26, 2017, 5:46 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60233/
> ---
> 
> (Updated June 26, 2017, 5:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/post-reviews.py 410fb3371a3c46adbfd68c7584ffd6cf3b3010d1 
> 
> 
> Diff: https://reviews.apache.org/r/60233/diff/2/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

[Andrei Budnik]

> On June 23, 2017, 4:06 p.m., haosdent huang wrote:
> > src/webui/master/static/agent.html
> > Lines 176 (patched)
> > 
> >
> > Should we use `*` here to keep consistent with other parts?

There is related discussion in the ticket, so it looks like we have agreed on 
calling it "Unreserved".


> On June 23, 2017, 4:06 p.m., haosdent huang wrote:
> > src/webui/master/static/agent.html
> > Lines 184 (patched)
> > 
> >
> > Is `|| 0` necessary here?

I think yes, because we might miss information about a specific role in 
state.reserved_resources_allocated. E.g., we have both statically reserved 
resources for both "ads" and "dyn" roles, but only one role is actually 
allocated. Let's assume that resources from "dyn" role are used to launch some 
task, while "ads" role isn't used, hence no item for "ads" in 
state.reserved_resources_allocated exists. In the latter case 0 is used as a 
default value.


- Andrei


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


On June 22, 2017, 3:26 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60370/
> ---
> 
> (Updated June 22, 2017, 3:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated agent webui page to display allocated resources per each role.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 71e5e702e5e64e6f46c84791247aa5156c046ed9 
>   src/webui/master/static/js/controllers.js 
> 67bfd030649dd21840c16188a4964f814aa232d7 
> 
> 
> Diff: https://reviews.apache.org/r/60370/diff/1/
> 
> 
> Testing
> ---
> 
> See screenshot.
> 
> 
> File Attachments
> 
> 
> agent.html
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/22/66cc61b4-04c8-451e-a434-58f556397724__Resource_Reservations.png
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Review Request 60438: Updated endpoint help generator script to work inside Docker.

[Vinod Kone]

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

Review request for mesos, Benjamin Mahler and haosdent huang.


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


Repository: mesos


Description
---

Changed agent flags to make this script work inside Docker container.
This is needed because this script will be run as part of website
publishing process which runs on ASF CI inside Docker.


Diffs
-

  support/generate-endpoint-help.py 6eb2d41b35c5f064839c29a0ba8326c590a33213 


Diff: https://reviews.apache.org/r/60438/diff/1/


Testing
---

Tested by running a CI job pointing to a branch containing this patch.


Thanks,

Vinod Kone

Review Request 60439: Added scripts to automate website publishing.

[Vinod Kone]

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

Review request for mesos and haosdent huang.


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


Repository: mesos


Description
---

These scripts are expected to be run by ASF CI on any commit to
the mesos repo. The directory layout is similar to tidybot CI job.


Diffs
-

  support/jenkins/websitebot.sh PRE-CREATION 
  support/mesos-website.sh PRE-CREATION 
  support/mesos-website/Dockerfile PRE-CREATION 
  support/mesos-website/build.sh PRE-CREATION 
  support/mesos-website/entrypoint.sh PRE-CREATION 


Diff: https://reviews.apache.org/r/60439/diff/1/


Testing
---

Tested by running a CI job pointing to a branch containing this patch.


Thanks,

Vinod Kone

Review Request 60440: Updated local development workflow of mesos website.

[Vinod Kone]

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

Review request for mesos and haosdent huang.


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


Repository: mesos


Description
---

Made the layout and scripts consistent with CI based automatic
publishing of the website.


Diffs
-

  site/Dockerfile 8ba0be0c28e924f7a2b89e6e5a3237deb3751a41 
  site/README.md ebd3e6a0fea7ae0fe3b28719bcab28ee8f7c356c 
  site/build.sh 11f15e15621c4d3db1472e88911787b9b3100f97 
  site/entrypoint.sh PRE-CREATION 
  site/mesos-website-dev.sh PRE-CREATION 


Diff: https://reviews.apache.org/r/60440/diff/1/


Testing
---

Tested by running the script locally.


Thanks,

Vinod Kone

Re: Review Request 60397: WIP: Check perf version compatibility in tests with disabled coredumps.

[Andrei Budnik]

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

(Updated June 26, 2017, 8:07 p.m.)


Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.


Summary (updated)
-

WIP: Check perf version compatibility in tests with disabled coredumps.


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


Repository: mesos


Description
---

For autotools build, the docker-build script performs a 'distcheck'
build. This type of build warns if any unexpected files are left in
the build directory after an uninstall, mainly to detect broken
uninstall Makefile rules. The return status of the build container is
the result of the distcheck.
This fixes an issue where in some dockerized configurations
invocations of 'perf' segfaulted (producing a core file as a
side-effect), where the failure case was already anticipated and
handled by the caller.


Diffs
-

  src/tests/environment.cpp a7262cd97361b55ff31238341657e764df6c9ea5 


Diff: https://reviews.apache.org/r/60397/diff/1/


Testing
---

1. make check (mac os x 10.12, fedora 25)
2. internal CI

Needs to be confirmed by Apache CI, e.g., reviewbot.


Thanks,

Andrei Budnik

Re: Review Request 60397: WIP: Check perf version compatibility in tests with disabled coredumps.

[Andrei Budnik]

> On June 23, 2017, 11:49 p.m., James Peach wrote:
> > Great idea! What do you think about setting the core rlimit to 0 in the the 
> > `subprocess` clone lambda in `perf::execute()`?

According to discussion in the related ticket, crashes are caused by calling 
abort() on failures in os::subprocess().


- Andrei


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


On June 26, 2017, 8:07 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> ---
> 
> (Updated June 26, 2017, 8:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
> 
> 
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.cpp a7262cd97361b55ff31238341657e764df6c9ea5 
> 
> 
> Diff: https://reviews.apache.org/r/60397/diff/1/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
> 
> Needs to be confirmed by Apache CI, e.g., reviewbot.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 58394: Setup new directory for python http client lib in src/python.

[Eric Chung]

> On May 2, 2017, 11:11 p.m., Kevin Klues wrote:
> > src/python/lib/mesos/__init__.py
> > Lines 1 (patched)
> > 
> >
> > I would exclude this from this commit.
> > It is not used anywhere, therefore it should not be included until the 
> > commit where it is used.
> 
> Eric Chung wrote:
> this makes it clear that this dir is a python package, in addition to 
> making git check the dir in. i would like to keep it unless there is a very 
> strong reason not to.
> 
> Kevin Klues wrote:
> I just meant to remove the line for the absolute_import. I understand the 
> need for the file in general.

gotcha. i'll remove the import statement


- Eric


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


On May 11, 2017, 6:34 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58394/
> ---
> 
> (Updated May 11, 2017, 6:34 p.m.)
> 
> 
> Review request for mesos, Jason Lai, Joseph Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-7310
> https://issues.apache.org/jira/browse/MESOS-7310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/58394
> 
> 
> Diffs
> -
> 
>   src/cli_new/bootstrap 6d62e9adf1d543ed00a3a2cf2484edf1c33ee443 
>   src/python/.gitignore PRE-CREATION 
>   src/python/lib/mesos/__init__.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58394/diff/2/
> 
> 
> Testing
> ---
> 
> Under src/cli_new:
> 1\. ./bootstrap
> 2\. . ./activate
> 3\. python
> 4\. >>> import mesos
> 5\. >>> mesos.\_\_path\_\_
> 6\. verify that the path printed out is indeed at src/python/lib/mesos
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Review Request 60448: Forward declare CheckerProcess.

[James Peach]

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

Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

Rather than including "checker_process.hpp" in header files,
forward declare CheckerProcess. This results in fewer visible
symbols and hopefully some small improvement in build times.


Diffs
-

  src/checks/checker.hpp 6f65b4709a3420fc7f4ed885425cdc8192cbcfbf 
  src/checks/health_checker.hpp dab366db45b11c6d300e9c8b82ce06565cec85a1 
  src/checks/health_checker.cpp 26b2dafa1ee6dedee1c2217bcb3c3dcd727be5ef 


Diff: https://reviews.apache.org/r/60448/diff/1/


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 60448: Forward declare CheckerProcess.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On June 26, 2017, 10:18 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60448/
> ---
> 
> (Updated June 26, 2017, 10:18 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than including "checker_process.hpp" in header files,
> forward declare CheckerProcess. This results in fewer visible
> symbols and hopefully some small improvement in build times.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp 6f65b4709a3420fc7f4ed885425cdc8192cbcfbf 
>   src/checks/health_checker.hpp dab366db45b11c6d300e9c8b82ce06565cec85a1 
>   src/checks/health_checker.cpp 26b2dafa1ee6dedee1c2217bcb3c3dcd727be5ef 
> 
> 
> Diff: https://reviews.apache.org/r/60448/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60169: Introduced a new agent flag 'cgroups_auto'.

[Gilbert Song]

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

(Updated June 26, 2017, 3:36 p.m.)


Review request for mesos, haosdent huang, Hao Yixin, Jason Lai, Jie Yu, and 
Vinod Kone.


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


Repository: mesos


Description
---

This new agent flag 'cgroups_auto' defaults to be 'true'.
It means by default, all local supported cgroup subsystems
will be turned on if they are implemented in mesos.


Diffs
-

  docs/configuration.md 0eb696a949003ff11831aed5e4f4ab384cf9992e 
  src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c 
  src/slave/flags.cpp c84aa6724170bba46bbe8410b71d42a1626e 


Diff: https://reviews.apache.org/r/60169/diff/1/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 60170: Supported auto cgroups in unified cgroup isolator.

[Gilbert Song]

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

(Updated June 26, 2017, 3:37 p.m.)


Review request for mesos, haosdent huang, Hao Yixin, Jason Lai, Jie Yu, and 
Vinod Kone.


Summary (updated)
-

Supported auto cgroups in unified cgroup isolator.


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


Repository: mesos


Description (updated)
---

This patch introduced a new agent isolation option 'cgroups', which
represents all local machine supported cgroup subsystems will be
turned on automatically.

Note that if this 'cgroups' option is specified in isolation, all
other specified cgroup subsystems will be included.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
d7a95b2d49fd2ae743e2bffc25f15c4ce4b96f39 


Diff: https://reviews.apache.org/r/60170/diff/2/

Changes: https://reviews.apache.org/r/60170/diff/1-2/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 60170: Supported auto cgroups in unified cgroup isolator.

[Gilbert Song]

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

(Updated June 26, 2017, 3:39 p.m.)


Review request for mesos, haosdent huang, Hao Yixin, Jason Lai, Jie Yu, and 
Vinod Kone.


Changes
---

Removed the agent flag option and used --isolation="cgroups" to represent auto 
cgroups.


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


Repository: mesos


Description
---

This patch introduced a new agent isolation option 'cgroups', which
represents all local machine supported cgroup subsystems will be
turned on automatically.

Note that if this 'cgroups' option is specified in isolation, all
other specified cgroup subsystems will be included.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
d7a95b2d49fd2ae743e2bffc25f15c4ce4b96f39 


Diff: https://reviews.apache.org/r/60170/diff/3/

Changes: https://reviews.apache.org/r/60170/diff/2-3/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

[Jason Lai]

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

(Updated June 26, 2017, 10:50 p.m.)


Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, 
Kunal Thakar, and Zhitao Li.


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


Repository: mesos


Description
---

Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.


Diffs
-

  include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 


Diff: https://reviews.apache.org/r/54693/diff/3/


Testing
---


Thanks,

Jason Lai

Re: Review Request 60428: Added MESOS-7581 to the 1.1.3 CHANGELOG.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [60425, 60427, 60428]

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 June 26, 2017, 10:43 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60428/
> ---
> 
> (Updated June 26, 2017, 10:43 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added MESOS-7581 to the 1.1.3 CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 7536717b85bb188d8a463b36ce75e4e1cecc445d 
> 
> 
> Diff: https://reviews.apache.org/r/60428/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 60170: Supported auto cgroups in unified cgroup isolator.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On June 26, 2017, 10:39 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60170/
> ---
> 
> (Updated June 26, 2017, 10:39 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Hao Yixin, Jason Lai, Jie Yu, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7691
> https://issues.apache.org/jira/browse/MESOS-7691
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduced a new agent isolation option 'cgroups', which
> represents all local machine supported cgroup subsystems will be
> turned on automatically.
> 
> Note that if this 'cgroups' option is specified in isolation, all
> other specified cgroup subsystems will be included.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> d7a95b2d49fd2ae743e2bffc25f15c4ce4b96f39 
> 
> 
> Diff: https://reviews.apache.org/r/60170/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60170: Supported auto cgroups in unified cgroup isolator.

[Hao Yixin]

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


Ship it!




Ship It!

- Hao Yixin


On 六月 26, 2017, 10:39 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60170/
> ---
> 
> (Updated 六月 26, 2017, 10:39 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Hao Yixin, Jason Lai, Jie Yu, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7691
> https://issues.apache.org/jira/browse/MESOS-7691
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduced a new agent isolation option 'cgroups', which
> represents all local machine supported cgroup subsystems will be
> turned on automatically.
> 
> Note that if this 'cgroups' option is specified in isolation, all
> other specified cgroup subsystems will be included.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> d7a95b2d49fd2ae743e2bffc25f15c4ce4b96f39 
> 
> 
> Diff: https://reviews.apache.org/r/60170/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60170: Supported auto cgroups in unified cgroup isolator.

[Hao Yixin]

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




src/slave/containerizer/mesos/containerizer.cpp
Lines 323 (patched)


I think that "cgroups/*" is more semantically consistent.


- Hao Yixin


On 六月 26, 2017, 10:39 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60170/
> ---
> 
> (Updated 六月 26, 2017, 10:39 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Hao Yixin, Jason Lai, Jie Yu, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7691
> https://issues.apache.org/jira/browse/MESOS-7691
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduced a new agent isolation option 'cgroups', which
> represents all local machine supported cgroup subsystems will be
> turned on automatically.
> 
> Note that if this 'cgroups' option is specified in isolation, all
> other specified cgroup subsystems will be included.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> d7a95b2d49fd2ae743e2bffc25f15c4ce4b96f39 
> 
> 
> Diff: https://reviews.apache.org/r/60170/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60397: WIP: Check perf version compatibility in tests with disabled coredumps.

[Benjamin Mahler]

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




src/tests/environment.cpp
Line 542 (original), 542-548 (patched)


Shouldn't this just be covered by `perf::suppported`?

Would also be great to include some additional context here, like a pointer 
to the ticket or mentioning that this is more likely if running within a docker 
image?


- Benjamin Mahler


On June 26, 2017, 8:07 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> ---
> 
> (Updated June 26, 2017, 8:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
> 
> 
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.cpp a7262cd97361b55ff31238341657e764df6c9ea5 
> 
> 
> Diff: https://reviews.apache.org/r/60397/diff/1/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
> 
> Needs to be confirmed by Apache CI, e.g., reviewbot.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>