Review Request 71286: Added container transition times to the logs.

[James Peach]

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

Review request for mesos and Gilbert Song.


Repository: mesos


Description
---

It's very useful to know directly how long a container spent in each
state. Capture a timestamp at each state transition time and log the
elapsed time in the corresponding state.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
271d6326e9739634b9e2b5edece3ace20f3a150c 
  src/slave/containerizer/mesos/containerizer.cpp 
75e7cdac499692be6838aa122578129d9cfa724c 


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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 71284: Fixed a typo in the master.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71284]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Aug. 14, 2019, 12:43 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71284/
> ---
> 
> (Updated Aug. 14, 2019, 12:43 a.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a typo in the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 783e4a38ad9e36cb38d5e78b6b942033bc806a0a 
>   src/master/master.cpp 31c7c97abecb92591369b417e2ef38a99cc09bac 
>   src/tests/master_tests.cpp 429521d14cec1c496cd8be12ce3fb3737fd8cf99 
> 
> 
> Diff: https://reviews.apache.org/r/71284/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 71244: Included task group's resources in the ExecutorInfo.

[Qian Zhang]

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

(Updated Aug. 14, 2019, 3:52 p.m.)


Review request for mesos, Andrei Budnik and Gilbert Song.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Included task group's resources in the ExecutorInfo.


Diffs (updated)
-

  src/slave/slave.cpp 74eb45744d6603b91676e812ed008a7b1ab39a49 


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

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


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 71244: Included task group's resources in the ExecutorInfo.

[Qian Zhang]

> On Aug. 9, 2019, 1:23 a.m., Vinod Kone wrote:
> > Can you add a test for this?

Sure, I posted a test here: https://reviews.apache.org/r/71287/


- Qian


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


On Aug. 14, 2019, 3:52 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71244/
> ---
> 
> (Updated Aug. 14, 2019, 3:52 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Gilbert Song.
> 
> 
> Bugs: MESOS-9925
> https://issues.apache.org/jira/browse/MESOS-9925
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Included task group's resources in the ExecutorInfo.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 74eb45744d6603b91676e812ed008a7b1ab39a49 
> 
> 
> Diff: https://reviews.apache.org/r/71244/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 71287: Added a test `SlaveTest.DefaultExecutorResources`.

[Qian Zhang]

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

Review request for mesos, Andrei Budnik and Gilbert Song.


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


Repository: mesos


Description
---

Added a test `SlaveTest.DefaultExecutorResources`.


Diffs
-

  src/tests/slave_tests.cpp 1a926029a9f81eb0c43e9cddd73c318557760c27 


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


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 71286: Added container transition times to the logs.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [71286]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:14.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose 
--disable-libtool-wrappers --disable-parallel-test-execution' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh 2>&1 | tee 
build_71286"]

Error:
..
ude -I../include/mesos -D__STDC_FORMAT_MACROS -I../3rdparty/boost-1.65.0 
-I../3rdparty/concurrentqueue-7b69a8f -I../3rdparty/elfio-3.2 
-I../3rdparty/glog-0.4.0/src -I../3rdparty/grpc-1.10.0/include 
-I../3rdparty/leveldb-1.19/include -I../3rdparty/libarchive-3.3.2/libarchive/ 
-I../../3rdparty/libprocess/include -I../3rdparty/nvml-352.79 
-I../3rdparty/picojson-1.3.0 -I../3rdparty/protobuf-3.5.0/src 
-I../3rdparty/rapidjson-1.1.0/include -I../../3rdparty/stout/include 
-I../3rdparty/zookeeper-3.4.8/src/c/include 
-I../3rdparty/zookeeper-3.4.8/src/c/generated -I/usr/include/subversion-1 
-I/usr/include/apr-1 -I/usr/include/apr-1.0 -pthread -Wall -Wsign-compare 
-Wformat-security -fstack-protector -fPIC -g1 -O0 -Wno-unused-local-typedefs 
-std=c++11 -c ../../src/resource_provider/local.cpp  -fPIC -DPIC -o 
resource_provider/.libs/libmesos_no_3rdparty_la-local.o
/bin/bash ../libtool  --tag=CXX   --mode=compile g++ -DPACKAGE_NAME=\"mesos\" 
-DPACKAGE_TARNAME=\"mesos\" -DPACKAGE_VERSION=\"1.9.0\" 
-DPACKAGE_STRING=\"mesos\ 1.9.0\" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" 
-DPACKAGE=\"mesos\" -DVERSION=\"1.9.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 
-DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 
-DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 
-DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 
-DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 
-DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 -DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 
-DMESOS_HAS_JAVA=1 -DHAVE_LIBSASL2=1 -DHAVE_SVN_VERSION_H=1 
-DHAVE_LIBSVN_SUBR_1=1 -DHAVE_SVN_DELTA_H=1 -DHAVE_LIBSVN_DELTA_1=1 
-DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 -DHAVE_PYTHON=\"2.7\" -DMESOS_HAS_PYTHON=1 -I. 
-I../../src   -Werror -DLIBDIR=\"/mesos/mesos-1.9.0/_inst/lib\" 
-DPKGLIBEXECDIR=\"/mesos/mesos-1.9.0/_inst/libexec/mesos\" 
-DPKGDATADIR=\"/mesos/mesos-1.9.0/_
 inst/share/mesos\" 
-DPKGMODULEDIR=\"/mesos/mesos-1.9.0/_inst/lib/mesos/modules\" -I../../include 
-I../include -I../include/mesos -D__STDC_FORMAT_MACROS 
-I../3rdparty/boost-1.65.0 -I../3rdparty/concurrentqueue-7b69a8f 
-I../3rdparty/elfio-3.2 -I../3rdparty/glog-0.4.0/src 
-I../3rdparty/grpc-1.10.0/include -I../3rdparty/leveldb-1.19/include 
-I../3rdparty/libarchive-3.3.2/libarchive/ -I../../3rdparty/libprocess/include  
-I../3rdparty/nvml-352.79 -I../3rdparty/picojson-1.3.0 
-I../3rdparty/protobuf-3.5.0/src -I../3rdparty/rapidjson-1.1.0/include 
-I../../3rdparty/stout/include -I../3rdparty/zookeeper-3.4.8/src/c/include 
-I../3rdparty/zookeeper-3.4.8/src/c/generated -I/usr/include/subversion-1 
-I/usr/include/apr-1 -I/usr/include/apr-1.0 -pthread -Wall 
-Wsign-compare -Wformat-security -fstack-protector -fPIC -fPIE -g1 -O0 
-Wno-unused-local-typedefs -std=c++11 -c -o 
resource_provider/libmesos_no_3rdparty_la-manager.lo `test -f 
'resource_provider/manager.cpp' || echo '../../src/'`resourc
 e_provider/manager.cpp
libtool: compile:  g++ -DPACKAGE_NAME=\"mesos\" -DPACKAGE_TARNAME=\"mesos\" 
-DPACKAGE_VERSION=\"1.9.0\" "-DPACKAGE_STRING=\"mesos 1.9.0\"" 
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DPACKAGE=\"mesos\" 
-DVERSION=\"1.9.0\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 
-DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 
-DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 
-DLT_OBJDIR=\".libs/\" -DHAVE_CXX11=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 
-DHAVE_PTHREAD=1 -DHAVE_OPENSSL_SSL_H=1 -DHAVE_FTS_H=1 -DHAVE_APR_POOLS_H=1 
-DHAVE_LIBAPR_1=1 -DHAVE_LIBCURL=1 -DMESOS_HAS_JAVA=1 -DHAVE_LIBSASL2=1 
-DHAVE_SVN_VERSION_H=1 -DHAVE_LIBSVN_SUBR_1=1 -DHAVE_SVN_DELTA_H=1 
-DHAVE_LIBSVN_DELTA_1=1 -DHAVE_ZLIB_H=1 -DHAVE_LIBZ=1 -DHAVE_PYTHON=\"2.7\" 
-DMESOS_HAS_PYTHON=1 -I. -I../../src -Werror 
-DLIBDIR=\"/mesos/mesos-1.9.0/_inst/lib\" 
-DPKGLIBEXECDIR=\"/mesos/mesos-1.9.0/_inst/libexec/mesos\" 
-DPKGDATADIR=\"/mesos/mesos-1.9.0/_inst/share/mesos\" -DPKGMODULED
 IR=\"/mesos/mesos-1.9.0/_inst/lib/mesos/modules\" -I../../include -I../include 
-I../include/mesos -D__STDC_FORMAT_MACROS -I../3rdparty/boost-1.65.0 
-I../3rdparty/concurrentqueue-7b69a8f -I../3rdparty/elfio-3.2 
-I../3rdparty/glog-0.4.0/src -I../3rdparty/grpc-1.10.0/include 
-I../3rdparty/leveldb-1.19/include -I../3rdparty/libarchive-3.3.2/libarchive/ 
-I../../3rdparty/libprocess/include -I../3rdparty/nvml-352.79 
-I../3rdparty/picojson-1.3.0 

Re: Review Request 70096: Moved cpplint configuration into dedicated file.

[Benno Evers]

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



Can you maybe clarify in the commit description what the advantage of having 
this in a separate file is?

This is going to clutter the source dir with a very visible, screaming-case 
`CPPLINT.cfg` file, so all other things being equal it seems like passing these 
settings via command-line instead would be preferrable?

- Benno Evers


On March 3, 2019, 1:01 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70096/
> ---
> 
> (Updated March 3, 2019, 1:01 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved cpplint configuration into dedicated file.
> 
> 
> Diffs
> -
> 
>   bootstrap 7be2cc95c7ace55d710315427f38284cc5b2af4c 
>   support/CPPLINT.cfg PRE-CREATION 
>   support/mesos-style.py b14820a1efe32c9c6e093b6f9cea93cb55ec97e4 
> 
> 
> Diff: https://reviews.apache.org/r/70096/diff/2/
> 
> 
> Testing
> ---
> 
> * confirmed that `./support/mesos-style.py src/executor/executor.cpp` still 
> does what is expected
> * no new warnings when running over the whole codebase
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70096: Moved cpplint configuration into dedicated file.

[Benjamin Bannier]

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

(Updated Aug. 14, 2019, 1:25 p.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description (updated)
---

With this change we not only reduce the amount of code in
`support/mesos-style.py` in favor of a configuration supported by
upstream, but we also make it easier to interoperate with editor
integrations for cpplint.


Diffs (updated)
-

  bootstrap 7be2cc95c7ace55d710315427f38284cc5b2af4c 
  support/CPPLINT.cfg PRE-CREATION 
  support/mesos-style.py cd490bd3deeb8477295bbad010f36d984ce17d27 


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

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


Testing
---

* confirmed that `./support/mesos-style.py src/executor/executor.cpp` still 
does what is expected
* no new warnings when running over the whole codebase


Thanks,

Benjamin Bannier

Re: Review Request 71205: Switch commit hooks to pre-commit.

[Benjamin Bannier]

> On Aug. 14, 2019, 1:04 p.m., Benno Evers wrote:
> > bootstrap
> > Line 55 (original), 55 (patched)
> > 
> >
> > Copying my comment from slack here so the discussion isn't split over 
> > too many places:
> > 
> > As we discussed privately yesterday, I think installing this in 
> > bootstrap is a bit problematic because that is also part of the source 
> > tarball, used by non-developers to build mesos w/o even having a git 
> > repository. Additionally, I think I'd be not particularly amused if I 
> > cloned a random open-source project and the first thing it does is install 
> > a bunch of stuff in my home directory.
> > 
> > I'm not sure if it's possible to implement, but imho the ideal workflow 
> > would be something like this:
> > 
> > ```
> > $ git commit -m "Foo the bar."
> > WARNING: Your commit touched a `.cpp` file, but `cpplint` is not 
> > installed on your system. It is highly recommended to install it to avoid 
> > embarrassing mistakes.
> > 
> > You can also run `pre-commit ` to automatically install a usable 
> > version of `cpplint` in you home directory.
> > ```

Totally agree on the mix of building and dev setup in `bootstrap` is 
problematic. Let me break out an `autogen.sh` to be used for setting up a build 
env, and `boostrap` also setting up a dev env.

We already download packages from the internet from `mesos-style.py` when e.g., 
setting up the virtualenv to run node linters. There is however no caching so 
one needs to potentially download this again and again after clearing the build 
dir; with `pre-commit` files are cached in `$HOME/.cache` and even for 
different linter versions or configs with the lifetime managed explicitly with 
`pre-commit`. I feel this is a superior, more controllable approach than 
pulling stuff with weird heuristics (`mesos-style.py` hardcodes a few of these 
and they tend to perform more work than necessary).

The only dependency of the linters now becomes `pre-commit` (which needs to be 
clearly documented) and potentially some Python interpreters or sim. Linters 
like e.g., eslint are automatically set up and do not need to be installed by 
users (in fact we want to control the exact versions of these tools, so 
`pre-commit` enforcing that is great). `cpplint` is a linter present in the 
source tree and does not need to be installed.


- Benjamin


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


On July 30, 2019, 11:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71205/
> ---
> 
> (Updated July 30, 2019, 11:01 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9360
> https://issues.apache.org/jira/browse/MESOS-9360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch switches commit hooks to be orchestrated by the pre-commit
> tool mirroring the previous linters invoked through git commit
> hooks (orchestrated by `support/mesos-style.py` or standalone hooks).
> 
> Using pre-commit removes the burden of maintaining
> `support/mesos-style.py`, making sure that hooks have the expected
> environment (e.g., Python version, Node installed). Additionally,
> upstream provides a number of additional linters which are not hard to
> add to Mesos' hooks.
> 
> 
> Diffs
> -
> 
>   .pre-commit-config.yaml PRE-CREATION 
>   bootstrap 7be2cc95c7ace55d710315427f38284cc5b2af4c 
> 
> 
> Diff: https://reviews.apache.org/r/71205/diff/1/
> 
> 
> Testing
> ---
> 
> * used successfully for a couple of months
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71204: Added gitlint config.

[Benjamin Bannier]

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

(Updated Aug. 14, 2019, 1:24 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Link config with bootstrap


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


Repository: mesos


Description
---

This patch adds a config for the gitlint tool which is slated to replace
a custom commit-msg hook once we switch our hook infrastructure to the
pre-commit tool.


Diffs (updated)
-

  bootstrap 7be2cc95c7ace55d710315427f38284cc5b2af4c 
  support/gitlint PRE-CREATION 


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

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


Testing
---

n/a


Thanks,

Benjamin Bannier

Re: Review Request 71269: Added a role tree class in the allocator.

[Meng Zhu]

> On Aug. 13, 2019, 7:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 198-199 (patched)
> > 
> >
> > The comment isn't adding value here IMO
> > 
> > s/getR/r/
> > 
> > Actually, couldn't this be just touching the member variable? It seems 
> > strange that you can only touch the root in a const way but can touch the 
> > Roles from `get()` in a non-const way?

yeah, I think we can just return a non-const root now. The Role class after 
some refactoring, now is pretty const already.


> On Aug. 13, 2019, 7:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 207-210 (patched)
> > 
> >
> > There seems to be a bit of subtlety here that is missed in the comment?
> > 
> > If I add "a/b", the ancestor role "a" along the path will be created. 
> > Then, if I add "a", the comment suggests that it already exists which is a 
> > problem.

Updated the comment to "The given role must not already exist.".


> On Aug. 13, 2019, 7:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 212-215 (patched)
> > 
> >
> > Ditto here, it seems at the overall explanation of the RoleTree 
> > abstraction we need to explain the "." subtlety, along with an example role 
> > tree to make it clear.

I am not sure what you mean by the "." subtlety. Unlike sorter, there is no 
need for any virtual node ".". I think the tree fits in the normal expectation. 
But drew an example nevertheless.


> On Aug. 13, 2019, 7:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 218-224 (patched)
> > 
> >
> > It's quite strange that these take maps with the role (seems like an 
> > artifact of the way the allocator managed reservations previously?), the 
> > following interface seems more expected:
> > 
> > ```
> >   void trackReservations(const Resources& reservations);
> >   void untrackReservations(const Resources& reservations);
> > ```
> > 
> > The implementation would examine the resources to know which roles the 
> > reservations are for. The caller can call it multiple times for different 
> > roles if they like, or pass in Resources that have a mix of reservations.
> > 
> > As it stands, it seems pretty brittle and unnecessary to assume that 
> > the Resources objects obey each role key.

Sg. posted a separate patch: r/71291


> On Aug. 13, 2019, 7:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 231 (patched)
> > 
> >
> > This can be `hashmap` which would avoid the 
> > potential leak of forgetting `delete`, and avoid the extra memory 
> > allocation.

hmm, would prefer keep the map just as a simple lookup table. Feels somewhat 
convoluted if also using it to manage the life cycle.
The simple destructors should be enough to guard against any leaks.
Also, the "get" call would need to return a coy of `Role` instead of a pointer, 
not quite what we want.


> On Aug. 13, 2019, 7:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2658-2664 (original), 2799-2805 (patched)
> > 
> >
> > Per the above comment, maybe this becomes:
> > 
> > ```
> >   Resources oldReservations = oldTotal.reserved();
> >   Resources newReservations = total.reserved();
> > 
> >   if (oldReservations != newReservations) {
> > roleTree.untrackReservations(oldReservations);
> > roleTree.trackReservations(newReservations);
> >   }
> > ```
> > 
> > Or more simply:
> > 
> > ```
> >   if (oldTotal.reserved() != total.reserved()) {
> > roleTree.untrackReservations(oldTotal.reserved());
> > roleTree.trackReservations(total.reserved());
> >   }
> > ```
> > 
> > Or don't even bother equality checking!
> > 
> > ```
> > roleTree.untrackReservations(oldTotal.reserved());
> > roleTree.trackReservations(total.reserved());
> > ```

Sg. r/71291


- Meng


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


On Aug. 14, 2019, 2:45 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71269/
> 

Re: Review Request 71269: Added a role tree class in the allocator.

[Meng Zhu]

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

(Updated Aug. 14, 2019, 2:45 p.m.)


Review request for mesos, Andrei Sekretenko and Benjamin Mahler.


Changes
---

Addressed Ben's comment.


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


Repository: mesos


Description
---

The role concept in Mesos fits into a tree structure naturally.
However, the role state in the allocator are currenstored
in a hashmap. This is less efficient and harder to use and reason.

This patch introduced a `RoleTree` structure in the allocator
and organizes all the roles in to a tree. This should simplify
the code logic and opens further refactor and optimization
opportunities.

In addition, the master code also lacks a proper tree structure
for tracking roles. We should leverage the same role tree code
here to simplify that as well.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
8be8dcee04e8fc5b97f730b2f058d14c81678788 
  src/master/allocator/mesos/hierarchical.cpp 
580d35a3b71c1f7e851fa0504c6b9f037c05c378 


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

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


Testing
---

make check

Benchmaring using optimized build shows no noticable performance change.

Benchmark: `BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota`

## Before:

Added 30 agents in 1.252621ms
Added 30 frameworks in 7.747202ms
Benchmark setup: 30 agents, 30 roles, 30 frameworks, with drf sorter
Made 36 allocations in 12.791427ms

Added 300 agents in 9.765295ms
Added 300 frameworks in 273.978325ms
Benchmark setup: 300 agents, 300 roles, 300 frameworks, with drf sorter
Made 350 allocations in 424.603736ms

Added 3000 agents in 79.646516ms
Added 3000 frameworks in 19.17449514secs
Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with drf sorter
Made 3500 allocations in 31.687207066secs

## After:

Added 30 agents in 1.376619ms
Added 30 frameworks in 7.647487ms
Benchmark setup: 30 agents, 30 roles, 30 frameworks, with drf sorter
Made 36 allocations in 11.638116ms

Added 300 agents in 9.506101ms
Added 300 frameworks in 263.008198ms
Benchmark setup: 300 agents, 300 roles, 300 frameworks, with drf sorter
Made 350 allocations in 418.962396ms

Added 3000 agents in 81.553527ms
Added 3000 frameworks in 19.201708305secs
Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with drf sorter
Made 3500 allocations in 31.583417136secs


Thanks,

Meng Zhu

Re: Review Request 71285: Fixed recovery of agent resources and operations after crash.

[Greg Mann]

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

(Updated Aug. 14, 2019, 10:35 p.m.)


Review request for mesos, Gastón Kleiman, James Peach, and Joseph Wu.


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


Repository: mesos


Description
---

Fixes an issue where the agent may incorrectly send an
OPERATION_FINISHED update for a failed offer operation
following agent failover and recovery.

The agent previously relied on the difference between the
set of checkpointed operations and the set of operation
IDs recovered from the operation status update manager to
apply any operations which had not been applied due to an
ill-timed agent failover.

However, this approach did not work in the case where a
persistent volume failed to be successfully created by
`syncCheckpointedResources()`. In order to handle this
case, this patch changes the agent code to continue with
the old approach of a two-phase-commit of persistent
volumes to disk, where the agent will fail to complete
recovery if `syncCheckpointedResources()` cannot be
executed successfully after failover.


Diffs
-

  src/slave/paths.hpp e077587fd02bd8e35fee7ce12ae436e3dca25e47 
  src/slave/paths.cpp 28a7cf9f9c70fb31eeefe2e823cd7e19ffcf126a 
  src/slave/slave.cpp 74eb45744d6603b91676e812ed008a7b1ab39a49 
  src/slave/state.cpp cd3fac72dd57da21ed5ac46b17066531af26d42a 


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


Testing (updated)
---

Tested manually by doing the following:

1) Run a master
2) Run an agent with statically reserved resources for use in the following step
3) Run the persistent volume example framework to create a volume on the agent, 
which leads to checkpointing of resources
4) Use `chattr -R +i /agent/volumes/dir` to make the agent's persistent volume 
directory immutable
5) Run the persistent volume example framework again; it will fail and the 
agent will crash
6) Restart the agent; confirm that it continues to crash
7) Use `chattr -R -i /agent/volumes/dir` to remove the immutable attribute
8) Restart the agent; confirm that it recovers successfully, with the 
persistent volume created on disk
9) Run the persistent volume example framework again; it should succeed


Thanks,

Greg Mann

Re: Review Request 71269: Added a role tree class in the allocator.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [71269, 71258, 71257, 71255, 71254]

Error:
2019-08-14 21:47:16 URL:https://reviews.apache.org/r/71254/diff/raw/ 
[5967/5967] -> "71254.patch" [1]
error: patch failed: src/master/allocator/mesos/sorter/drf/sorter.cpp:17
error: src/master/allocator/mesos/sorter/drf/sorter.cpp: patch does not apply
error: patch failed: src/master/allocator/mesos/sorter/random/sorter.cpp:17
error: src/master/allocator/mesos/sorter/random/sorter.cpp: patch does not apply

- Mesos Reviewbot


On Aug. 14, 2019, 9:45 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71269/
> ---
> 
> (Updated Aug. 14, 2019, 9:45 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9917
> https://issues.apache.org/jira/browse/MESOS-9917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The role concept in Mesos fits into a tree structure naturally.
> However, the role state in the allocator are currenstored
> in a hashmap. This is less efficient and harder to use and reason.
> 
> This patch introduced a `RoleTree` structure in the allocator
> and organizes all the roles in to a tree. This should simplify
> the code logic and opens further refactor and optimization
> opportunities.
> 
> In addition, the master code also lacks a proper tree structure
> for tracking roles. We should leverage the same role tree code
> here to simplify that as well.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 8be8dcee04e8fc5b97f730b2f058d14c81678788 
>   src/master/allocator/mesos/hierarchical.cpp 
> 580d35a3b71c1f7e851fa0504c6b9f037c05c378 
> 
> 
> Diff: https://reviews.apache.org/r/71269/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> Benchmaring using optimized build shows no noticable performance change.
> 
> Benchmark: `BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota`
> 
> ## Before:
> 
> Added 30 agents in 1.252621ms
> Added 30 frameworks in 7.747202ms
> Benchmark setup: 30 agents, 30 roles, 30 frameworks, with drf sorter
> Made 36 allocations in 12.791427ms
> 
> Added 300 agents in 9.765295ms
> Added 300 frameworks in 273.978325ms
> Benchmark setup: 300 agents, 300 roles, 300 frameworks, with drf sorter
> Made 350 allocations in 424.603736ms
> 
> Added 3000 agents in 79.646516ms
> Added 3000 frameworks in 19.17449514secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with drf sorter
> Made 3500 allocations in 31.687207066secs
> 
> ## After:
> 
> Added 30 agents in 1.376619ms
> Added 30 frameworks in 7.647487ms
> Benchmark setup: 30 agents, 30 roles, 30 frameworks, with drf sorter
> Made 36 allocations in 11.638116ms
> 
> Added 300 agents in 9.506101ms
> Added 300 frameworks in 263.008198ms
> Benchmark setup: 300 agents, 300 roles, 300 frameworks, with drf sorter
> Made 350 allocations in 418.962396ms
> 
> Added 3000 agents in 81.553527ms
> Added 3000 frameworks in 19.201708305secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with drf sorter
> Made 3500 allocations in 31.583417136secs
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 71291: Refactored (un)trackReservations() in the allocator.

[Meng Zhu]

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

Review request for mesos, Andrei Sekretenko and Benjamin Mahler.


Repository: mesos


Description
---

Refactored (un)trackReservations() in the allocator.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
8be8dcee04e8fc5b97f730b2f058d14c81678788 
  src/master/allocator/mesos/hierarchical.cpp 
580d35a3b71c1f7e851fa0504c6b9f037c05c378 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 71291: Refactored (un)trackReservations() in the allocator.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [71291, 71269, 71258, 71257, 71255, 71254]

Error:
2019-08-14 23:18:59 URL:https://reviews.apache.org/r/71254/diff/raw/ 
[5967/5967] -> "71254.patch" [1]
error: patch failed: src/master/allocator/mesos/sorter/drf/sorter.cpp:17
error: src/master/allocator/mesos/sorter/drf/sorter.cpp: patch does not apply
error: patch failed: src/master/allocator/mesos/sorter/random/sorter.cpp:17
error: src/master/allocator/mesos/sorter/random/sorter.cpp: patch does not apply

- Mesos Reviewbot


On Aug. 14, 2019, 9:46 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71291/
> ---
> 
> (Updated Aug. 14, 2019, 9:46 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored (un)trackReservations() in the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 8be8dcee04e8fc5b97f730b2f058d14c81678788 
>   src/master/allocator/mesos/hierarchical.cpp 
> 580d35a3b71c1f7e851fa0504c6b9f037c05c378 
> 
> 
> Diff: https://reviews.apache.org/r/71291/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 71286: Added container transition times to the logs.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71286]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Aug. 14, 2019, 6:12 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71286/
> ---
> 
> (Updated Aug. 14, 2019, 6:12 a.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's very useful to know directly how long a container spent in each
> state. Capture a timestamp at each state transition time and log the
> elapsed time in the corresponding state.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 271d6326e9739634b9e2b5edece3ace20f3a150c 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e7cdac499692be6838aa122578129d9cfa724c 
> 
> 
> Diff: https://reviews.apache.org/r/71286/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 71285: Fixed recovery of agent resources and operations after crash.

[James Peach]

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




src/slave/slave.cpp
Lines 4430 (patched)


I think that this comment could be improved. We can see that it is 
renaming, so it would be better to discuss why we do the rename now and what it 
means.



src/slave/slave.cpp
Lines 4439 (patched)


Add the error to the exit message?



src/slave/state.cpp
Lines 819 (patched)


`state::read` can already return `None()`, so what do you think about 
letting it return `None()` when the file to read isn't present?

I think that this would improve the code in a number of places, but it's a 
large enough change that a separate review would be better.


- James Peach


On Aug. 14, 2019, 10:35 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71285/
> ---
> 
> (Updated Aug. 14, 2019, 10:35 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-9875
> https://issues.apache.org/jira/browse/MESOS-9875
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixes an issue where the agent may incorrectly send an
> OPERATION_FINISHED update for a failed offer operation
> following agent failover and recovery.
> 
> The agent previously relied on the difference between the
> set of checkpointed operations and the set of operation
> IDs recovered from the operation status update manager to
> apply any operations which had not been applied due to an
> ill-timed agent failover.
> 
> However, this approach did not work in the case where a
> persistent volume failed to be successfully created by
> `syncCheckpointedResources()`. In order to handle this
> case, this patch changes the agent code to continue with
> the old approach of a two-phase-commit of persistent
> volumes to disk, where the agent will fail to complete
> recovery if `syncCheckpointedResources()` cannot be
> executed successfully after failover.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.hpp e077587fd02bd8e35fee7ce12ae436e3dca25e47 
>   src/slave/paths.cpp 28a7cf9f9c70fb31eeefe2e823cd7e19ffcf126a 
>   src/slave/slave.cpp 74eb45744d6603b91676e812ed008a7b1ab39a49 
>   src/slave/state.cpp cd3fac72dd57da21ed5ac46b17066531af26d42a 
> 
> 
> Diff: https://reviews.apache.org/r/71285/diff/1/
> 
> 
> Testing
> ---
> 
> Tested manually by doing the following:
> 
> 1) Run a master
> 2) Run an agent with statically reserved resources for use in the following 
> step
> 3) Run the persistent volume example framework to create a volume on the 
> agent, which leads to checkpointing of resources
> 4) Use `chattr -R +i /agent/volumes/dir` to make the agent's persistent 
> volume directory immutable
> 5) Run the persistent volume example framework again; it will fail and the 
> agent will crash
> 6) Restart the agent; confirm that it continues to crash
> 7) Use `chattr -R -i /agent/volumes/dir` to remove the immutable attribute
> 8) Restart the agent; confirm that it recovers successfully, with the 
> persistent volume created on disk
> 9) Run the persistent volume example framework again; it should succeed
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 71285: Fixed recovery of agent resources and operations after crash.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71285]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Aug. 14, 2019, 12:53 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71285/
> ---
> 
> (Updated Aug. 14, 2019, 12:53 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-9875
> https://issues.apache.org/jira/browse/MESOS-9875
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixes an issue where the agent may incorrectly send an
> OPERATION_FINISHED update for a failed offer operation
> following agent failover and recovery.
> 
> The agent previously relied on the difference between the
> set of checkpointed operations and the set of operation
> IDs recovered from the operation status update manager to
> apply any operations which had not been applied due to an
> ill-timed agent failover.
> 
> However, this approach did not work in the case where a
> persistent volume failed to be successfully created by
> `syncCheckpointedResources()`. In order to handle this
> case, this patch changes the agent code to continue with
> the old approach of a two-phase-commit of persistent
> volumes to disk, where the agent will fail to complete
> recovery if `syncCheckpointedResources()` cannot be
> executed successfully after failover.
> 
> 
> Diffs
> -
> 
>   src/slave/paths.hpp e077587fd02bd8e35fee7ce12ae436e3dca25e47 
>   src/slave/paths.cpp 28a7cf9f9c70fb31eeefe2e823cd7e19ffcf126a 
>   src/slave/slave.cpp 74eb45744d6603b91676e812ed008a7b1ab39a49 
>   src/slave/state.cpp cd3fac72dd57da21ed5ac46b17066531af26d42a 
> 
> 
> Diff: https://reviews.apache.org/r/71285/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 71205: Switch commit hooks to pre-commit.

[Benno Evers]

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




bootstrap
Line 55 (original), 55 (patched)


Copying my comment from slack here so the discussion isn't split over too 
many places:

As we discussed privately yesterday, I think installing this in bootstrap 
is a bit problematic because that is also part of the source tarball, used by 
non-developers to build mesos w/o even having a git repository. Additionally, I 
think I'd be not particularly amused if I cloned a random open-source project 
and the first thing it does is install a bunch of stuff in my home directory.

I'm not sure if it's possible to implement, but imho the ideal workflow 
would be something like this:

```
$ git commit -m "Foo the bar."
WARNING: Your commit touched a `.cpp` file, but `cpplint` is not installed 
on your system. It is highly recommended to install it to avoid embarrassing 
mistakes.

You can also run `pre-commit ` to automatically install a usable 
version of `cpplint` in you home directory.
```


- Benno Evers


On July 30, 2019, 9:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71205/
> ---
> 
> (Updated July 30, 2019, 9:01 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9360
> https://issues.apache.org/jira/browse/MESOS-9360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch switches commit hooks to be orchestrated by the pre-commit
> tool mirroring the previous linters invoked through git commit
> hooks (orchestrated by `support/mesos-style.py` or standalone hooks).
> 
> Using pre-commit removes the burden of maintaining
> `support/mesos-style.py`, making sure that hooks have the expected
> environment (e.g., Python version, Node installed). Additionally,
> upstream provides a number of additional linters which are not hard to
> add to Mesos' hooks.
> 
> 
> Diffs
> -
> 
>   .pre-commit-config.yaml PRE-CREATION 
>   bootstrap 7be2cc95c7ace55d710315427f38284cc5b2af4c 
> 
> 
> Diff: https://reviews.apache.org/r/71205/diff/1/
> 
> 
> Testing
> ---
> 
> * used successfully for a couple of months
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71206: Removed old mesos-style and references.

[Benno Evers]

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



After applying this series for testing purposes, I ran into a problem where 
`mesos-style.py` was still referenced from a git hook, breaking my ability to 
commit without non-trivial investigation of the problem. (and I had the 
advantage of *knowing* that I just applied this patch series, if we commit it 
to master some other developers will be caught by suprise, no matter how 
heavily it is advertised.)

I'd suggest instead of removing `mesos-style.py` outright, we replace it with a 
simple bash script that just prints something like `WARNING: 'mesos-style.py' 
is not supported anymore and does nothing, please fix whatever was calling this 
script. See MESOS-9630.`

- Benno Evers


On July 30, 2019, 9:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71206/
> ---
> 
> (Updated July 30, 2019, 9:01 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9360
> https://issues.apache.org/jira/browse/MESOS-9360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removes references to `support/mesos-style.py` which was
> replaced with a pre-commit setup in a previous commit. We also remove
> the tool itself.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md 8a48afe780f23736c9b7abeb7337977521cecfa5 
>   support/build-virtualenv 7dc03b054f7663979e4eb4b11ad51d759b7f1ad3 
>   support/hooks/commit-msg a0c218deee3fb4b7594fe39b76c1025045ba0725 
>   support/hooks/post-rewrite 1ab14abf711d1923a7ae69beb33581317009a94a 
>   support/hooks/pre-commit 519567bf5f20a74b273c8d8514577fe4342dc45d 
>   support/mesos-style.py cd490bd3deeb8477295bbad010f36d984ce17d27 
> 
> 
> Diff: https://reviews.apache.org/r/71206/diff/1/
> 
> 
> Testing
> ---
> 
> n/a
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71209: Enabled a number of additional pre-commit checks.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70096, 71203, 71204, 71205, 71206, 71207, 71208, 71209]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On July 30, 2019, 9:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71209/
> ---
> 
> (Updated July 30, 2019, 9:01 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9360
> https://issues.apache.org/jira/browse/MESOS-9360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled a number of additional pre-commit checks.
> 
> 
> Diffs
> -
> 
>   .pre-commit-config.yaml PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71209/diff/1/
> 
> 
> Testing
> ---
> 
> * used for development for a couple of months
> * reports no issues in the current source tree as indentified issues were 
> fixed
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

[GitHub] [mesos] lava commented on a change in pull request #341: Remove unnecessary logging.

[GitBox]

lava commented on a change in pull request #341: Remove unnecessary logging.
URL: https://github.com/apache/mesos/pull/341#discussion_r313886115
 
 

 ##
 File path: src/slave/slave.cpp
 ##
 @@ -7157,11 +7157,6 @@ void Slave::_checkDiskUsage(const Future& usage)
 LOG(ERROR) << "Failed to get disk usage: "
<< (usage.isFailed() ? usage.failure() : "future discarded");
   } else {
-executorDirectoryMaxAllowedAge = age(usage.get());
 
 Review comment:
   Here `executorDirectoryMaxAllowedAge` is actually a member variable of 
`Slave` that is used for generating the value of the metric, so removing the 
assignment will probably break this. (Imho this is also a fault in our C++ 
style guide, having no visual indication of member variables makes it too easy 
to make this kind of mistake)


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Review Request 71289: Fixed out-of-order processing of requests in composing containerizer.

[Andrei Budnik]

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

Review request for mesos, Gilbert Song, Greg Mann, and Qian Zhang.


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


Repository: mesos


Description
---

Previously, the composing containerizer could return
"Container not found" failure before all preceding requests
have been processed by the underlying containerizer. It could
lead to subtle errors on a client side, which expects that all
requests are processed strictly in the order they arrived.
This patch introduces DESTROYED state and forces all requests
to a DESTROYED container to wait until the underlying containerizer
finishes processing of requests sent before the container transitioned
to the DESTROYED state.


Diffs
-

  src/slave/containerizer/composing.cpp 
d854794fc4775fb8a05efc233d488a64b9ef620a 


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


Testing
---


Thanks,

Andrei Budnik

Review Request 71137: Added a test to ensure composing containerizer preserves request order.

[Andrei Budnik]

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

Review request for mesos, Gilbert Song, Greg Mann, and Qian Zhang.


Summary (updated)
-

Added a test to ensure composing containerizer preserves request order.


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


Repository: mesos


Description (updated)
---

Added a test to ensure composing containerizer preserves request order.


Diffs (updated)
-

  src/tests/containerizer/composing_containerizer_tests.cpp 
4236bd4ea97f5d2cf91601de34b89e63c85a6fa9 
  src/tests/containerizer/mock_containerizer.hpp 
8700257a9347199ed6e32b8b6b2a58787d68af03 
  src/tests/slave_tests.cpp 1a926029a9f81eb0c43e9cddd73c318557760c27 


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


Testing (updated)
---

1. manual testing described in MESOS-9887
2. internal CI


Thanks,

Andrei Budnik

Re: Review Request 71284: Fixed a typo in the master.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Aug. 13, 2019, 5:43 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71284/
> ---
> 
> (Updated Aug. 13, 2019, 5:43 p.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a typo in the master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 783e4a38ad9e36cb38d5e78b6b942033bc806a0a 
>   src/master/master.cpp 31c7c97abecb92591369b417e2ef38a99cc09bac 
>   src/tests/master_tests.cpp 429521d14cec1c496cd8be12ce3fb3737fd8cf99 
> 
> 
> Diff: https://reviews.apache.org/r/71284/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 71137: Added a test to ensure composing containerizer preserves request order.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71289, 71137]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Aug. 14, 2019, 4:12 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71137/
> ---
> 
> (Updated Aug. 14, 2019, 4:12 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Qian Zhang.
> 
> 
> Bugs: MESOS-9887
> https://issues.apache.org/jira/browse/MESOS-9887
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to ensure composing containerizer preserves request order.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/composing_containerizer_tests.cpp 
> 4236bd4ea97f5d2cf91601de34b89e63c85a6fa9 
>   src/tests/containerizer/mock_containerizer.hpp 
> 8700257a9347199ed6e32b8b6b2a58787d68af03 
>   src/tests/slave_tests.cpp 1a926029a9f81eb0c43e9cddd73c318557760c27 
> 
> 
> Diff: https://reviews.apache.org/r/71137/diff/1/
> 
> 
> Testing
> ---
> 
> 1. manual testing described in MESOS-9887
> 2. internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

[GitHub] [mesos] bgalek commented on issue #341: Remove unnecessary logging.

[GitBox]

bgalek commented on issue #341: Remove unnecessary logging.
URL: https://github.com/apache/mesos/pull/341#issuecomment-521279359
 
 
   @vinodkone nope, just thought it was duplication of metric


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [mesos] vinodkone commented on issue #341: Remove unnecessary logging.

[GitBox]

vinodkone commented on issue #341: Remove unnecessary logging.
URL: https://github.com/apache/mesos/pull/341#issuecomment-521271763
 
 
   This log line has been valuable to see disk usage over time from logs when 
we encountered disk full issues. A metric is not a substitute because we would 
need a metrics stack with history to get the same data. Also, this is the only 
log line that I know of in the agent that periodically prints something which 
is valuable because one can easily tell from the logs whether the main actor 
was stuck or not when debugging issues. 
   
   @bgalek Has this logging been causing any issues for you in production?


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: Review Request 71287: Added a test `SlaveTest.DefaultExecutorResources`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71244, 71287]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Aug. 14, 2019, 12:54 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71287/
> ---
> 
> (Updated Aug. 14, 2019, 12:54 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Gilbert Song.
> 
> 
> Bugs: MESOS-9925
> https://issues.apache.org/jira/browse/MESOS-9925
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `SlaveTest.DefaultExecutorResources`.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 1a926029a9f81eb0c43e9cddd73c318557760c27 
> 
> 
> Diff: https://reviews.apache.org/r/71287/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>