Review Request 44421: Added support for "overlay" keyword.

[Guangya Liu]

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

Review request for mesos, haosdent huang, Jie Yu, and Shuai Lin.


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


Repository: mesos


Description
---

The "overlayfs" was renamed to "overlay" in kernel 4.2, for overlay
support check function, it should check both "overlay" and "overlayfs"
in "/proc/filesystems".


Diffs
-

  src/linux/fs.cpp dbf94759b6382bfafd3f3b8b4c2047af36a53ad5 
  src/slave/containerizer/mesos/provisioner/backend.cpp 
55652540e35f9c451ad85cfead575a788aa3eba1 
  src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
5cc0f8b5a8cd4c945023f874056a8184113186c5 
  src/tests/environment.cpp ee1bbe6b4e3dda1e27b63d71a08ef0d1d254741a 

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


Testing
---

root@mesos002:~/src/mesos/m1/mesos/build# uname -r
4.2.3-040203-generic
root@mesos002:~/src/mesos/m1/mesos/build# lsmod  | grep over
overlay45056  1 
root@mesos002:~/src/mesos/m1/mesos/build# cat /proc/filesystems | grep over
nodev   overlay
root@mesos002:~/src/

make
make check
./bin/mesos-tests.sh 
--gtest_filter="OverlayBackendTest.ROOT_OVERLAYFS_OverlayFSBackend" --verbose


Thanks,

Guangya Liu

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44408]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 5, 2016, 4:17 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 5, 2016, 4:17 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44414: Added documentation about container image support.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44414]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 5, 2016, 2:20 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44414/
> ---
> 
> (Updated March 5, 2016, 2:20 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, Neil Conway, Timothy 
> Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-4873
> https://issues.apache.org/jira/browse/MESOS-4873
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation about container image support.
> 
> 
> Diffs
> -
> 
>   docs/container-image.md PRE-CREATION 
>   docs/mesos-provisioner.md 1b19406cb93bbc3f5330eaf9d29b1be98a674136 
> 
> Diff: https://reviews.apache.org/r/44414/diff/
> 
> 
> Testing
> ---
> 
> Tested the formatting in Mou
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 43615: Update test suite to use the reworked MesosTest helpers.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43613, 43614, 43629, 43630, 43615]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 5, 2016, 12:14 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43615/
> ---
> 
> (Updated March 5, 2016, 12:14 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Includes the following changes:
> 
> * Added the `` header where appropriate.
> * Added the namespace `using process::Owned;` where appropriate.
> * Generally replaced `Try` with `Owned`.  And 
> `Try` with `Owned`.
> * Added the (now required) `MasterDetector` argument to all slaves.  Before, 
> this was fetched from the first master in `Cluster`.
> * Removed `Shutdown();` from all tests.
> * Replaced `Stop(...)` with the appropriate master/slave destruction calls.
> * Wrap various slave objects in `Owned` (i.e. containerizers, isolators, 
> launchers, etc).
> * Replace `CHECK` in tests with `ASSERT`.
> 
> 
> Diffs
> -
> 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/command_executor_tests.cpp 
> 0d2fcf6d4b8d9a925eb6748e6bd33cf279b8f7f8 
>   src/tests/container_logger_tests.cpp 
> 00f4129e46aa9268fbb66da25b34e61004fa87b2 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6aecd912fc84b72d2b64f7a842891fddcbc469ac 
>   src/tests/containerizer/external_containerizer_test.cpp 
> 8e1dbe306a088eb16cd3b9c6174b95fad5685da4 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> e72239a55724f1aeeec5362cc370c93dbeca7164 
>   src/tests/containerizer/isolator_tests.cpp 
> 342037ce0a5f8caa4e3cf1550b8f9a7cc328acf9 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 03879d99c371f296f8d9904666911b34209c114d 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 983a6be160aefe5a32acb6111bb3c85230ec 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 5b685bfd842d0d98e8ea5ec5ddea8d2cd893dd81 
>   src/tests/credentials_tests.cpp 7edcc857e0f6f8e80e265deeec59d6349d392224 
>   src/tests/disk_quota_tests.cpp 413e562026a4fc9779f616e921ae2fa2ca51e012 
>   src/tests/exception_tests.cpp 6b71316d545e97f14a45daa14d0fd95204befd3b 
>   src/tests/executor_http_api_tests.cpp 
> 2fc0893f5f5e80a783296fb31b30abe86d92df1b 
>   src/tests/fault_tolerance_tests.cpp 
> f2b8dba809518cf716b2b5a7a6a8a5fe62e57646 
>   src/tests/gc_tests.cpp 61a8abb9581dc4602b197a88a677b19386969cbf 
>   src/tests/health_check_tests.cpp 59ef31970af2d255abe169dfbc2e6e0314d29e9a 
>   src/tests/hook_tests.cpp 88ad7806bf9f3194e434743c35199a896edea92c 
>   src/tests/master_allocator_tests.cpp 
> cba7c36471f93b678d94e1da0251a28a893696b1 
>   src/tests/master_authorization_tests.cpp 
> 29c89fb11da792c3e71eb880a19657ea225b3cc8 
>   src/tests/master_contender_detector_tests.cpp 
> 255ab8119a04b55bb4f1b61dee19c4be64499376 
>   src/tests/master_quota_tests.cpp 4fabc1473ec3e048afe7171abbb8d6e49e863847 
>   src/tests/master_slave_reconciliation_tests.cpp 
> d41178eb41df519073fc0890c5716bbc9fed6ad2 
>   src/tests/master_tests.cpp 36595772b34bcb8d37dbc74d247bdf4614f10150 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
>   src/tests/metrics_tests.cpp 419d275e0b32817388120222bd433ee6f4835efd 
>   src/tests/monitor_tests.cpp 869c9e032817e8859a968232d4a61556a3d53d45 
>   src/tests/oversubscription_tests.cpp 
> e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
>   src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 81185a161498394020a27f1f5bf747bac5425f43 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
>   src/tests/rate_limiting_tests.cpp 352a5e7a58b500c25c7c8a421047c4e79434e38e 
>   src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 
>   src/tests/registrar_zookeeper_tests.cpp 
> 3df9779ee5d076e16f6a538326693a36f986b6d0 
>   src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
>   src/tests/reservation_endpoints_tests.cpp 
> f95ae7a32c3809d150adf1e9e515a3b527e61699 
>   src/tests/reservation_tests.cpp d7f9de6f2bce061316916260f356efdb96ecd482 
>   

Re: Review Request 44260: Moved metrics of the hierarchical allocator to its own file.

[Ben Mahler]

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


Fix it, then Ship it!




I like the idea of making this more consistent with the metrics within 
`master/` and `slave/`, thanks!

A couple of minor things here but otherwise looks great!


src/master/allocator/mesos/hierarchical.hpp (line 284)


Hm.. I don't know that the comment here adds any clarity for the reader. At 
least for me, it's not clear what it means for a dispatch event to be 
"waiting", this is just the number of dispatches in the event queue. "wait" 
seems to imply some form of blocking.

It seems pretty clear to me from the way the code is written here and the 
metric name, no?

Also, keep in mind that this very name is used in a number of places, so 
the documentation should ideally be consistent.



src/master/allocator/mesos/metrics.hpp (lines 1 - 47)


The directory layout / file naming here is a little odd, because this is 
coupled to the hierarchical allocator, but the following layout doesn't express 
this:

`allocator/mesos/allocator.hpp`
`allocator/mesos/metrics.hpp`
`allocator/mesos/hierarchical.hpp`

Ideally the hierarchical allocator and metrics are more clearly coupled, 
since there is the allocator wrapper also contained here. For now it's ok since 
we can move things around here (not part of the public interfaces).



src/master/allocator/mesos/metrics.cpp (lines 17 - 21)


Any reason for the unusual ordering?


- Ben Mahler


On March 3, 2016, 4:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44260/
> ---
> 
> (Updated March 3, 2016, 4:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4718
> https://issues.apache.org/jira/browse/MESOS-4718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved metrics of the hierarchical allocator to its own file.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am b30cc25f61856d6417437547baaa0bb338a30d63 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44260/diff/
> 
> 
> Testing
> ---
> 
> `make distcheck` on OS X.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Yong Tang]

> On March 5, 2016, 2:31 a.m., Joseph Wu wrote:
> > src/tests/persistent_volume_tests.cpp, lines 283-284
> > 
> >
> > One more small refinement:
> > 
> > Now that the master does not use this variable, you should move it 
> > above the `MesosSchedulerDriver driver(...);`.
> > 
> > Ditto for all modified tests.

Thanks Joseph. Just updated the Review Request. Let me know if there are any 
other issues.


- Yong


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


On March 5, 2016, 4:17 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 5, 2016, 4:17 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Yong Tang]

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

(Updated March 5, 2016, 4:17 a.m.)


Review request for mesos and Joseph Wu.


Changes
---

Move the initialization of FrameworkInfo above MesosSchedulerDriver as 
FrameworkInfo is not needed for CreateMasterFlag.


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


Repository: mesos


Description
---

This fix removes the setting up of ACLs in PersistentVolumeTests
as it is no longer needed any more with implicit roles (MESOS-4868).


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 

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


Testing
---

make check (in Ubuntu 14.04)


Thanks,

Yong Tang

Re: Review Request 44087: Moved logic to assign process to freezer hierarchy into parentHook.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44247, 44087]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 3, 2016, 3:39 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44087/
> ---
> 
> (Updated March 3, 2016, 3:39 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved logic to assign process to freezer hierarchy into parentHook.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 9c80cfb621ef2e28aabfb2649846892964d2d4f3 
> 
> Diff: https://reviews.apache.org/r/44087/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (on Linux)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Guangya Liu]

> On 三月 5, 2016, 1:25 a.m., Joseph Wu wrote:
> > src/tests/persistent_volume_tests.cpp, line 89
> > 
> >
> > This entire helper is not needed anymore (due to implicit roles).
> 
> Guangya Liu wrote:
> @Jopseph, why also remove acls here? The implicit roles will only create 
> roles on demand but what about acl?
> 
> Joseph Wu wrote:
> By default, ACLs are "permissive".  So if a given permission does not 
> exist, Mesos will allow it.  
> 
> For the majority of the tests that use this helper, they are only using 
> it to set initialize the `role` so that a framework can register.  The 
> ACL-specific tests set up ACLs separately.

Good to know, thanks @Joesph.


- Guangya


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


On 三月 5, 2016, 2:24 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated 三月 5, 2016, 2:24 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44414: Added documentation about container image support.

[Guangya Liu]

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




docs/container-image.md (line 11)


s/containerizers/__containerizers__



docs/container-image.md (lines 44 - 46)


What about highlight Docker, AppC, OCF here?
s/Docker/__Docker__
s/AppC/__AppC__
s/OCF/__OCF__



docs/container-image.md (lines 67 - 77)


Does it make sense to enable slave add those isolations based on 
containizer technology? If using `Docker`, then add `docker/runtime` 
dynamically if the `isolation` flag does not include `docker/runtime`? This can 
make the configuration simple.



docs/container-image.md (line 83)


default value is `mesos`, do we need to specify it again?



docs/container-image.md (lines 124 - 130)


Does it make sense to guide end user/framework developer refer to 
https://github.com/apache/mesos/blob/master/src/cli/execute.cpp for an example? 
I see that there are someone asking this question in dev list, it would be 
great if we can add the reference code here.



docs/container-image.md (line 152)


s/start the Mesos agent/start the Mesos agent with docker as image provider



docs/container-image.md (line 161)


s/Mesos CLI/Mesos CLI `mesos-execute`



docs/container-image.md (lines 263 - 264)


s/*and* where more recent kernel
features such as overlayfs are not available.//

The overlay was already supportted.



docs/container-image.md (line 283)


I uploaded a patch here: https://reviews.apache.org/r/44391/

Will rebase this patch as mesos-provisioner.md is now deleted.


- Guangya Liu


On 三月 5, 2016, 2:20 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44414/
> ---
> 
> (Updated 三月 5, 2016, 2:20 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, Neil Conway, Timothy 
> Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-4873
> https://issues.apache.org/jira/browse/MESOS-4873
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation about container image support.
> 
> 
> Diffs
> -
> 
>   docs/container-image.md PRE-CREATION 
>   docs/mesos-provisioner.md 1b19406cb93bbc3f5330eaf9d29b1be98a674136 
> 
> Diff: https://reviews.apache.org/r/44414/diff/
> 
> 
> Testing
> ---
> 
> Tested the formatting in Mou
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 44407: Fixed a typo in a log message in an example framework.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44407]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 4, 2016, 9:55 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44407/
> ---
> 
> (Updated March 4, 2016, 9:55 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a typo in a log message in an example framework.
> 
> 
> Diffs
> -
> 
>   src/examples/long_lived_framework.cpp 
> c4c3aa68dc3e6e001f9a746ea5151b8ad958856f 
> 
> Diff: https://reviews.apache.org/r/44407/diff/
> 
> 
> Testing
> ---
> 
> Visual inspection of log message.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43880: Added allocator metrics for total and allocated scalar resources.

[Alexander Rojas]

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



Most of my comments are based on an email @alex-mesos sent today, in which he 
suggests removing metrics related code from the allocator and into the 
`Metrics` class, which is an idea I strongly support. If we go that way, I 
would double down into that and move _all_ related code into metrics.


src/master/allocator/mesos/hierarchical.cpp (lines 415 - 420)


It just occurred to me, since @alex-mesos mentioned moving code related to 
metrics to the `Metrics` class, and they are closely related, why not do the 
`foreach` there in a method `Metrics::createGaugesForScalars(const 
std::set& names)`



src/master/allocator/mesos/hierarchical.cpp (lines 528 - 533)


ditto



src/master/allocator/mesos/hierarchical.cpp (lines 1705 - 1730)


I wonder if we can move this code to the `Metrics` class. It can be in the 
code that actually calls this method… Unless we plan to use this functions for 
other purposes than updating the gauges.



src/master/allocator/mesos/metrics.hpp (line 43)


A reference perhaps? I don't understand why is a const pointer better in 
this case.


Over

- Alexander Rojas


On March 4, 2016, 5:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated March 4, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4720
> https://issues.apache.org/jira/browse/MESOS-4720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43880/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 44366: Added GPUs as an explicit resource.

[Kevin Klues]

> On March 4, 2016, 12:27 p.m., Klaus Ma wrote:
> > src/slave/containerizer/containerizer.cpp, line 105
> > 
> >
> > `` for --nvidia_gpus; not only this line, please also update them in 
> > the following comments.

Sorry, I don't think I understand.  What are you proposing here?


> On March 4, 2016, 12:27 p.m., Klaus Ma wrote:
> > src/slave/containerizer/containerizer.cpp, line 115
> > 
> >
> > I'd like to say "`--navidia_gpus` must be set when specifying `gpu` 
> > resources."

Are you just suggesting to make the Error string shorter?


- Kevin


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


On March 4, 2016, 1:11 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 4, 2016, 1:11 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Rob Todd.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently we can either infer the number of GPUs to offer as resources
> from the --nvidia_gpus flag, or pass the amount explicity as a "gpus"
> resource. In the future, we will generalize this via autodiscovery of
> gpus and support for gpus types other than Nvidia.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/44366/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Joseph Wu]

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



BTW, thanks for doing this :)


src/tests/persistent_volume_tests.cpp (lines 262 - 263)


One more small refinement:

Now that the master does not use this variable, you should move it above 
the `MesosSchedulerDriver driver(...);`.

Ditto for all modified tests.


- Joseph Wu


On March 4, 2016, 6:24 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 4, 2016, 6:24 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Joseph Wu]

> On March 4, 2016, 5:25 p.m., Joseph Wu wrote:
> > src/tests/persistent_volume_tests.cpp, line 89
> > 
> >
> > This entire helper is not needed anymore (due to implicit roles).
> 
> Guangya Liu wrote:
> @Jopseph, why also remove acls here? The implicit roles will only create 
> roles on demand but what about acl?

By default, ACLs are "permissive".  So if a given permission does not exist, 
Mesos will allow it.  

For the majority of the tests that use this helper, they are only using it to 
set initialize the `role` so that a framework can register.  The ACL-specific 
tests set up ACLs separately.


- Joseph


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


On March 4, 2016, 6:24 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 4, 2016, 6:24 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Yong Tang]

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

(Updated March 5, 2016, 2:24 a.m.)


Review request for mesos and Joseph Wu.


Changes
---

Removed entired MasterFlags() helper as it is not needed.


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


Repository: mesos


Description
---

This fix removes the setting up of ACLs in PersistentVolumeTests
as it is no longer needed any more with implicit roles (MESOS-4868).


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 

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


Testing
---

make check (in Ubuntu 14.04)


Thanks,

Yong Tang

Review Request 44414: Added documentation about container image support.

[Jie Yu]

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

Review request for mesos, Gilbert Song, Jojy Varghese, Neil Conway, Timothy 
Chen, and Vinod Kone.


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


Repository: mesos


Description
---

Added documentation about container image support.


Diffs
-

  docs/container-image.md PRE-CREATION 
  docs/mesos-provisioner.md 1b19406cb93bbc3f5330eaf9d29b1be98a674136 

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


Testing
---

Tested the formatting in Mou


Thanks,

Jie Yu

Re: Review Request 43879: Added allocator metrics for number of allocations made.

[Alexander Rojas]

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


Ship it!




Ship It!

- Alexander Rojas


On March 3, 2016, 5:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43879/
> ---
> 
> (Updated March 3, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4718
> https://issues.apache.org/jira/browse/MESOS-4718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metrics for number of allocations made.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43879/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 44260: Moved metrics of the hierarchical allocator to its own file.

[Alexander Rojas]

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


Ship it!





src/master/allocator/mesos/metrics.hpp (line 26)


I'm not so sure about this `internal` namespace in an already `internal` 
namespace, still, apparently allocator likes to do so.


- Alexander Rojas


On March 3, 2016, 5:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44260/
> ---
> 
> (Updated March 3, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4718
> https://issues.apache.org/jira/browse/MESOS-4718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved metrics of the hierarchical allocator to its own file.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am b30cc25f61856d6417437547baaa0bb338a30d63 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44260/diff/
> 
> 
> Testing
> ---
> 
> `make distcheck` on OS X.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 44380: Change IOTest.BufferedRead to write to the temporary directory.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44380]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 4, 2016, 6:56 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44380/
> ---
> 
> (Updated March 4, 2016, 6:56 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-4807
> https://issues.apache.org/jira/browse/MESOS-4807
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit changes IOTest.BufferedRead so that this specific
> test (not all IOTest) could be executed from temporary
> directories via TemporaryDirectoryTest fixture (MESOS-4807).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/io_tests.cpp 
> 2bffc7cd9c3aa204a1d1b8eb45f0bff12f49ca62 
> 
> Diff: https://reviews.apache.org/r/44380/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Guangya Liu]

> On 三月 5, 2016, 1:25 a.m., Joseph Wu wrote:
> > src/tests/persistent_volume_tests.cpp, line 89
> > 
> >
> > This entire helper is not needed anymore (due to implicit roles).

@Jopseph, why also remove acls here? The implicit roles will only create roles 
on demand but what about acl?


- Guangya


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


On 三月 4, 2016, 10:49 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated 三月 4, 2016, 10:49 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Joseph Wu]

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




src/tests/persistent_volume_tests.cpp (line 89)


This entire helper is not needed anymore (due to implicit roles).


- Joseph Wu


On March 4, 2016, 2:49 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44408/
> ---
> 
> (Updated March 4, 2016, 2:49 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-4868
> https://issues.apache.org/jira/browse/MESOS-4868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix removes the setting up of ACLs in PersistentVolumeTests
> as it is no longer needed any more with implicit roles (MESOS-4868).
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
> 
> Diff: https://reviews.apache.org/r/44408/diff/
> 
> 
> Testing
> ---
> 
> make check (in Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44258: Fixed http endpoint trigger two inverse offer calls.

[Guangya Liu]

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

(Updated March 5, 2016, 1:23 a.m.)


Review request for mesos, Anand Mazumdar, Joris Van Remoortere, and Joseph Wu.


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


Repository: mesos


Description
---

There is a bug when setting host maintain with http endpoint: 
https://github.com/apache/mesos/blob/master/src/master/http.cpp#L1987-L2021
The logic is as this:
1) Get all host list from maintain window and put it to updated hashmap.
2) If the machine in was in updated was also in master->machines, call master 
updateUnavailability to trigger recoverResources, updateUnavailability etc in 
allocator
3) Otherwise, clear the unavailabity time window for the machine.
4) Update each new machines in updated to call master updateUnavailability

But the logic in step 4) is getting all machines from the schedule windows but 
not the machines that is new to the cluster, this caused master get two 
updateUnavailability calls for a machine in the updated hashmap.

The fix is filter machines in updated hashmap when handling new machines.


Diffs (updated)
-

  src/master/http.cpp 8276baa538eb4d2aaf54cc1aa516bffaadacc4dd 
  src/tests/master_maintenance_tests.cpp 
3faa8136cf57276295553910319480028f433e4c 

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


Testing
---

make
make check
 ./bin/mesos-tests.sh --gtest_filter="MasterMaintenanceTest.*" --verbose


Thanks,

Guangya Liu

Re: Review Request 44258: Fixed http endpoint trigger two inverse offer calls.

[Joseph Wu]

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




src/master/http.cpp (lines 2029 - 2030)


s/due to/since/
s/trigger inverse/trigger an inverse/


- Joseph Wu


On March 4, 2016, 4:34 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44258/
> ---
> 
> (Updated March 4, 2016, 4:34 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Joris Van Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-4831
> https://issues.apache.org/jira/browse/MESOS-4831
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There is a bug when setting host maintain with http endpoint: 
> https://github.com/apache/mesos/blob/master/src/master/http.cpp#L1987-L2021
> The logic is as this:
> 1) Get all host list from maintain window and put it to updated hashmap.
> 2) If the machine in was in updated was also in master->machines, call master 
> updateUnavailability to trigger recoverResources, updateUnavailability etc in 
> allocator
> 3) Otherwise, clear the unavailabity time window for the machine.
> 4) Update each new machines in updated to call master updateUnavailability
> 
> But the logic in step 4) is getting all machines from the schedule windows 
> but not the machines that is new to the cluster, this caused master get two 
> updateUnavailability calls for a machine in the updated hashmap.
> 
> The fix is filter machines in updated hashmap when handling new machines.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8276baa538eb4d2aaf54cc1aa516bffaadacc4dd 
>   src/tests/master_maintenance_tests.cpp 
> 3faa8136cf57276295553910319480028f433e4c 
> 
> Diff: https://reviews.apache.org/r/44258/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
>  ./bin/mesos-tests.sh --gtest_filter="MasterMaintenanceTest.*" --verbose
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 44258: Fixed http endpoint trigger two inverse offer calls.

[Guangya Liu]

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

(Updated March 5, 2016, 12:34 a.m.)


Review request for mesos, Anand Mazumdar, Joris Van Remoortere, and Joseph Wu.


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


Repository: mesos


Description
---

There is a bug when setting host maintain with http endpoint: 
https://github.com/apache/mesos/blob/master/src/master/http.cpp#L1987-L2021
The logic is as this:
1) Get all host list from maintain window and put it to updated hashmap.
2) If the machine in was in updated was also in master->machines, call master 
updateUnavailability to trigger recoverResources, updateUnavailability etc in 
allocator
3) Otherwise, clear the unavailabity time window for the machine.
4) Update each new machines in updated to call master updateUnavailability

But the logic in step 4) is getting all machines from the schedule windows but 
not the machines that is new to the cluster, this caused master get two 
updateUnavailability calls for a machine in the updated hashmap.

The fix is filter machines in updated hashmap when handling new machines.


Diffs (updated)
-

  src/master/http.cpp 8276baa538eb4d2aaf54cc1aa516bffaadacc4dd 
  src/tests/master_maintenance_tests.cpp 
3faa8136cf57276295553910319480028f433e4c 

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


Testing
---

make
make check
 ./bin/mesos-tests.sh --gtest_filter="MasterMaintenanceTest.*" --verbose


Thanks,

Guangya Liu

Re: Review Request 44258: Fixed http endpoint trigger two inverse offer calls.

[Guangya Liu]

> On March 4, 2016, 8:28 p.m., Joris Van Remoortere wrote:
> > src/master/http.cpp, lines 2026-2027
> > 
> >
> > Can you explain how machines going from `UP` to `DOWN` are handled in 
> > the next loop?
> > I see logic for `UP` to `DRAINING` in the next loop.
> > 
> > Also missing a backtick after `UP`

My bad, it is from `UP` to `DRAINING`.


> On March 4, 2016, 8:28 p.m., Joris Van Remoortere wrote:
> > src/master/http.cpp, lines 2031-2033
> > 
> >
> > For some of these early exit conditions, does it make sense to add 
> > `CHECK`s (and maybe event comments) to document why we are exiting?
> > Stating *that* we are exiting less helpful to readers than *why*.
> > 
> > I think the implied invariant here (which we should call out 
> > explicitly) is that any machine should only be "touched" by 1 of the 2 
> > loops here. The exit conditions between them are meant to enforce this 
> > exclusion?

I was adding some comments to address any machine should only be "touched" by 1 
of the 2 loops here. Can you please show more for how we can add `CHECK` here? 
I did not found a good way to add `CHECK` for here but only by checking via 
some `if` conditions.


- Guangya


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


On March 4, 2016, 2:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44258/
> ---
> 
> (Updated March 4, 2016, 2:10 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Joris Van Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-4831
> https://issues.apache.org/jira/browse/MESOS-4831
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There is a bug when setting host maintain with http endpoint: 
> https://github.com/apache/mesos/blob/master/src/master/http.cpp#L1987-L2021
> The logic is as this:
> 1) Get all host list from maintain window and put it to updated hashmap.
> 2) If the machine in was in updated was also in master->machines, call master 
> updateUnavailability to trigger recoverResources, updateUnavailability etc in 
> allocator
> 3) Otherwise, clear the unavailabity time window for the machine.
> 4) Update each new machines in updated to call master updateUnavailability
> 
> But the logic in step 4) is getting all machines from the schedule windows 
> but not the machines that is new to the cluster, this caused master get two 
> updateUnavailability calls for a machine in the updated hashmap.
> 
> The fix is filter machines in updated hashmap when handling new machines.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8276baa538eb4d2aaf54cc1aa516bffaadacc4dd 
>   src/tests/master_maintenance_tests.cpp 
> 3faa8136cf57276295553910319480028f433e4c 
> 
> Diff: https://reviews.apache.org/r/44258/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
>  ./bin/mesos-tests.sh --gtest_filter="MasterMaintenanceTest.*" --verbose
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 129
> > 
> >
> > Can we also check if the plugin is executable? Since later we are going 
> > to try and execute the plugin.

This might be slightly more involved then I thought. You will have to figure 
out if the owner or group of the binary is root (since the assumption is agent 
is running with sudo). If yes then check the permission of the user and the 
group. If no then check the permission of others. os::permissions should allow 
you to check all the permissions.


- Avinash


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


On March 4, 2016, 5:07 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 5:07 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43615: Update test suite to use the reworked MesosTest helpers.

[Joseph Wu]

> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> >

I also noticed a couple of these:
```
  MesosSchedulerDriver driver(
, DEFAULT_FRAMEWORK_INFO, master.get()->pid, DEFAULT_CREDENTIAL);
```
Now fixed (there were two spaces rather than four).

---

Also went through and changed a couple of these:
```
  Future slaveReregisteredMessage =
FUTURE_PROTOBUF(
SlaveReregisteredMessage(), master.get()->pid, slave.get()->pid);
```
To:
```
  Future slaveReregisteredMessage =
FUTURE_PROTOBUF(
SlaveReregisteredMessage(), 
master.get()->pid, 
slave.get()->pid);
```


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/master_tests.cpp, line 1053
> > 
> >
> > Indentation see above.

For the above 3 issues, see the note after the next reply below.


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/master_allocator_tests.cpp, line 515
> > 
> >
> > Line break after "=" is prefered.
> > 
> > Please update other places like this, too.

Sort-of related.  I noticed some tech-debt and filed this: 
https://issues.apache.org/jira/browse/MESOS-4868 (I added newlines after 
`StartMaster` in those tests).


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/container_logger_tests.cpp, line 302
> > 
> >
> > This indentation is OK, but prefered is this:
> > 
> > Try slave = 
> >   StartSlave(detector.get(), containerizer.get(), flags);
> >   
> > Same elsewhere. Please update all of these.

Fixed!


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/master_tests.cpp, line 1101
> > 
> >
> > Insert blank line above, please.

Also added blank lines for this case in:

MesosSchedulerDriverTest, DropAckIfStopCalledBeforeAbort
MesosSchedulerDriverTest, ExplicitAcknowledgements
SchedulerDriverEventTest, Offers
SlaveTest, HTTPScheduler
SlaveTest, HTTPSchedulerLiveUpgrade


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/master_tests.cpp, line 1932
> > 
> >
> > The master needs to be stopped here, not below, right? Otherwise we 
> > might be missing part of what we want to test here.

Good catch :D

I also missed this in:

ReconciliationTest, SlaveInTransition


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/master_tests.cpp, line 1140
> > 
> >
> > It's OK to continue with the first arg on the same line in such cases.
> > 
> > Here and elsewhere.

I believe the preferred style is:
```
EXPECT_EQ(
...,
...);
```
Rather than:
```
EXPECT_EQ(...
  ...);
```
(This is based on how we indented the MasterMaintenanceTests.)


> On March 4, 2016, 6:45 a.m., Bernd Mathiske wrote:
> > src/tests/master_tests.cpp, line 3754
> > 
> >
> > This could go in line 3661.

Didn't notice that extra space (after `detector`).  Fixed that too.


- Joseph


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


On March 4, 2016, 4:14 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43615/
> ---
> 
> (Updated March 4, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Includes the following changes:
> 
> * Added the `` header where appropriate.
> * Added the namespace `using process::Owned;` where appropriate.
> * Generally replaced `Try` with `Owned`.  And 
> `Try` with `Owned`.
> * Added the (now required) `MasterDetector` argument to all slaves.  Before, 
> this was fetched from the first master in `Cluster`.
> * Removed `Shutdown();` from all tests.
> * Replaced `Stop(...)` with the appropriate master/slave destruction calls.
> * Wrap various slave objects in `Owned` (i.e. containerizers, isolators, 
> launchers, etc).
> * Replace `CHECK` in tests with `ASSERT`.
> 
> 
> Diffs
> -
> 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   

Re: Review Request 43615: Update test suite to use the reworked MesosTest helpers.

[Joseph Wu]

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

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


Review request for mesos, Bernd Mathiske and Artem Harutyunyan.


Changes
---

Rebase and address conflicts.  Changed lots of spacing.  Address Bernd's 
comments.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Includes the following changes:

* Added the `` header where appropriate.
* Added the namespace `using process::Owned;` where appropriate.
* Generally replaced `Try` with `Owned`.  And 
`Try` with `Owned`.
* Added the (now required) `MasterDetector` argument to all slaves.  Before, 
this was fetched from the first master in `Cluster`.
* Removed `Shutdown();` from all tests.
* Replaced `Stop(...)` with the appropriate master/slave destruction calls.
* Wrap various slave objects in `Owned` (i.e. containerizers, isolators, 
launchers, etc).
* Replace `CHECK` in tests with `ASSERT`.


Diffs (updated)
-

  src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
  src/tests/command_executor_tests.cpp 0d2fcf6d4b8d9a925eb6748e6bd33cf279b8f7f8 
  src/tests/container_logger_tests.cpp 00f4129e46aa9268fbb66da25b34e61004fa87b2 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6aecd912fc84b72d2b64f7a842891fddcbc469ac 
  src/tests/containerizer/external_containerizer_test.cpp 
8e1dbe306a088eb16cd3b9c6174b95fad5685da4 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
e72239a55724f1aeeec5362cc370c93dbeca7164 
  src/tests/containerizer/isolator_tests.cpp 
342037ce0a5f8caa4e3cf1550b8f9a7cc328acf9 
  src/tests/containerizer/memory_pressure_tests.cpp 
03879d99c371f296f8d9904666911b34209c114d 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
  src/tests/containerizer/port_mapping_tests.cpp 
983a6be160aefe5a32acb6111bb3c85230ec 
  src/tests/containerizer/provisioner_docker_tests.cpp 
5b685bfd842d0d98e8ea5ec5ddea8d2cd893dd81 
  src/tests/credentials_tests.cpp 7edcc857e0f6f8e80e265deeec59d6349d392224 
  src/tests/disk_quota_tests.cpp 413e562026a4fc9779f616e921ae2fa2ca51e012 
  src/tests/exception_tests.cpp 6b71316d545e97f14a45daa14d0fd95204befd3b 
  src/tests/executor_http_api_tests.cpp 
2fc0893f5f5e80a783296fb31b30abe86d92df1b 
  src/tests/fault_tolerance_tests.cpp f2b8dba809518cf716b2b5a7a6a8a5fe62e57646 
  src/tests/gc_tests.cpp 61a8abb9581dc4602b197a88a677b19386969cbf 
  src/tests/health_check_tests.cpp 59ef31970af2d255abe169dfbc2e6e0314d29e9a 
  src/tests/hook_tests.cpp 88ad7806bf9f3194e434743c35199a896edea92c 
  src/tests/master_allocator_tests.cpp cba7c36471f93b678d94e1da0251a28a893696b1 
  src/tests/master_authorization_tests.cpp 
29c89fb11da792c3e71eb880a19657ea225b3cc8 
  src/tests/master_contender_detector_tests.cpp 
255ab8119a04b55bb4f1b61dee19c4be64499376 
  src/tests/master_quota_tests.cpp 4fabc1473ec3e048afe7171abbb8d6e49e863847 
  src/tests/master_slave_reconciliation_tests.cpp 
d41178eb41df519073fc0890c5716bbc9fed6ad2 
  src/tests/master_tests.cpp 36595772b34bcb8d37dbc74d247bdf4614f10150 
  src/tests/master_validation_tests.cpp 
c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
  src/tests/metrics_tests.cpp 419d275e0b32817388120222bd433ee6f4835efd 
  src/tests/monitor_tests.cpp 869c9e032817e8859a968232d4a61556a3d53d45 
  src/tests/oversubscription_tests.cpp e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
  src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
  src/tests/persistent_volume_endpoints_tests.cpp 
81185a161498394020a27f1f5bf747bac5425f43 
  src/tests/persistent_volume_tests.cpp 
bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
  src/tests/rate_limiting_tests.cpp 352a5e7a58b500c25c7c8a421047c4e79434e38e 
  src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 
  src/tests/registrar_zookeeper_tests.cpp 
3df9779ee5d076e16f6a538326693a36f986b6d0 
  src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
  src/tests/reservation_endpoints_tests.cpp 
f95ae7a32c3809d150adf1e9e515a3b527e61699 
  src/tests/reservation_tests.cpp d7f9de6f2bce061316916260f356efdb96ecd482 
  src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 
  src/tests/scheduler_driver_tests.cpp f6dc25d82ae5f1e77fc6ede7ff2660ed0d9ea039 
  src/tests/scheduler_event_call_tests.cpp 
8c02ceeb3ec1783cb2f63f100700508e70f586e4 
  src/tests/scheduler_http_api_tests.cpp 
dfb0f51fec67a3951e396eab28eedb0dbf9493ae 
  src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 
  src/tests/status_update_manager_tests.cpp 
d64d3b8c96270478f6b681c038de77c3a9eb68fe 
  src/tests/teardown_tests.cpp 6e9e2e64f1666c2a81f5d859ee014ee14365b6b0 

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


Testing
---

sudo 

Re: Review Request 43630: Especially updated scheduler tests to use the updated MesosTest helpers.

[Joseph Wu]

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

(Updated March 4, 2016, 3:58 p.m.)


Review request for mesos, Bernd Mathiske and Artem Harutyunyan.


Changes
---

Rebase and update scheduler tests.  These used to have some special changes, 
but now look pretty much identical to the changes in the next review (Thanks 
Anand!)


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Continuation of https://reviews.apache.org/r/43615/ with re-ordering of some 
local variables due to the order of destruction.


Diffs (updated)
-

  src/tests/master_maintenance_tests.cpp 
3faa8136cf57276295553910319480028f433e4c 
  src/tests/scheduler_tests.cpp fa42fb42f2d18060a867ade547cebbdcaead07d4 

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


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 43629: Especially updated tests to use the updated MesosTest helpers.

[Joseph Wu]

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

(Updated March 4, 2016, 3:55 p.m.)


Review request for mesos, Bernd Mathiske and Artem Harutyunyan.


Changes
---

Change spacing of calls to `StartSlave` with lots of arguments.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Continuation of https://reviews.apache.org/r/43615/ with a slightly different 
pattern.


Diffs (updated)
-

  src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 
  src/tests/resource_offers_tests.cpp 0bad45dd1dabecc88fef1ab46e8ea26718070b33 
  src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 

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


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 41049: New python lib with only the executor driver.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [41049]

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

Error:
2016-03-04 23:51:33 URL:https://reviews.apache.org/r/41049/diff/raw/ 
[36629/36629] -> "41049.patch" [1]
error: patch failed: src/python/native/ext_modules.py.in:1
error: src/python/native/ext_modules.py.in: patch does not apply

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

- Mesos ReviewBot


On March 4, 2016, 7:51 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41049/
> ---
> 
> (Updated March 4, 2016, 7:51 p.m.)
> 
> 
> Review request for mesos, Till Toenshoff and Vinod Kone.
> 
> 
> Bugs: MESOS-4090
> https://issues.apache.org/jira/browse/MESOS-4090
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> New python lib with only the executor driver.
> 
> This patch produces a new python egg, mesos.executor, which contains only the 
> code needed to create a MesosExecutorDriver.  By doing so, the linker can 
> remove unused code in libmesos_no_3rdparty.a, and therefor not include any 
> external dependencies in the resulting _mesos.so.
> 
> 
> Diffs
> -
> 
>   configure.ac b30a8d30076f3068fd7d5fc8ccea1982639e998a 
>   src/Makefile.am 0409491b92c9720d60ad76fdbc2edff554fb4965 
>   src/python/executor/setup.py.in PRE-CREATION 
>   src/python/executor/src/mesos/__init__.py PRE-CREATION 
>   src/python/executor/src/mesos/executor/__init__.py PRE-CREATION 
>   src/python/executor/src/mesos/executor/module.cpp PRE-CREATION 
>   src/python/native/ext_modules.py.in 
> 4682e5eed0f7be23fb48ef628e1bebc7741431d7 
>   src/python/native/setup.py.in 49ed61293281f65d6597470ce3697326ac769032 
>   src/python/native/src/mesos/native/__init__.py 
> 226f94357cd81b700706edd68c94de461647fa1b 
>   src/python/native/src/mesos/native/mesos_executor_driver_impl.hpp  
>   src/python/native/src/mesos/native/mesos_executor_driver_impl.cpp 
> 7838a07ac12034a5eed2f459bd11e5e0a07e94de 
>   src/python/native/src/mesos/native/mesos_scheduler_driver_impl.hpp  
>   src/python/native/src/mesos/native/mesos_scheduler_driver_impl.cpp 
> f8be49bf5feb6675c3c8e6a1df75a5e079d09fcc 
>   src/python/native/src/mesos/native/module.hpp 
> 2cf7b57077d10c071bd4b8f36e2cb43041fc168d 
>   src/python/native/src/mesos/native/module.cpp 
> 6e672a21edf0e9df1d95688620ea9cc6a1b2 
>   src/python/native/src/mesos/native/proxy_executor.hpp  
>   src/python/native/src/mesos/native/proxy_executor.cpp 
> 706f417b547bb9878346902b2a2f98b1afade5ad 
>   src/python/native/src/mesos/native/proxy_scheduler.hpp  
>   src/python/native/src/mesos/native/proxy_scheduler.cpp 
> 8afb3380d99b4dc9c4f7b3d926f4a5b1d6e94c20 
>   src/python/native_common/ext_modules.py.in PRE-CREATION 
>   src/python/scheduler/setup.py.in PRE-CREATION 
>   src/python/scheduler/src/mesos/__init__.py PRE-CREATION 
>   src/python/scheduler/src/mesos/scheduler/__init__.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41049/diff/
> 
> 
> Testing
> ---
> 
> On CentOS 7, GCC 4.8.5:
> - make distcheck
> - Tested with aurora executor
> - Made sure mesos.scheduler still worked
> - Made sure mesos.native still worked
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Re: Review Request 43614: Refactor MesosTest and remove cleanup logic.

[Joseph Wu]

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

(Updated March 4, 2016, 3:43 p.m.)


Review request for mesos, Bernd Mathiske and Artem Harutyunyan.


Changes
---

A couple of comment changes (Bernd's comments & a stale comment that I noticed 
in one of the `StartMaster` helpers).


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Updates `StartMaster` and `StartSlave` test helpers to use the reworked 
`cluster` helpers.  Removes all `MesosTest` cleanup logic and as well as the 
helpers that accept a `MockExecutor` pointer.


Diffs (updated)
-

  src/tests/mesos.hpp d36840f6e23e5823332de53061bf6852330bdf0b 
  src/tests/mesos.cpp e0f641c6828833de13a0a233e39ff6dc3f343d5c 

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


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44004, 44200, 44269]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 4, 2016, 5:07 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 5:07 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 44408: Remove setting up of ACLs (unneeded) in PersistentVolumeTests.

[Yong Tang]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

This fix removes the setting up of ACLs in PersistentVolumeTests
as it is no longer needed any more with implicit roles (MESOS-4868).


Diffs
-

  src/tests/persistent_volume_tests.cpp 
bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 

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


Testing
---

make check (in Ubuntu 14.04)


Thanks,

Yong Tang

Re: Review Request 44087: Moved logic to assign process to freezer hierarchy into parentHook.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On March 3, 2016, 3:39 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44087/
> ---
> 
> (Updated March 3, 2016, 3:39 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved logic to assign process to freezer hierarchy into parentHook.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 9c80cfb621ef2e28aabfb2649846892964d2d4f3 
> 
> Diff: https://reviews.apache.org/r/44087/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (on Linux)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Review Request 44407: Fixed a typo in a log message in an example framework.

[Neil Conway]

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

Review request for mesos and Joris Van Remoortere.


Repository: mesos


Description
---

Fixed a typo in a log message in an example framework.


Diffs
-

  src/examples/long_lived_framework.cpp 
c4c3aa68dc3e6e001f9a746ea5151b8ad958856f 

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


Testing
---

Visual inspection of log message.


Thanks,

Neil Conway

Re: Review Request 44396: Updated unavailable in batch to avoid several allocate(slaveId) call in "/maintenance/schedule".

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [44396]

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

Error:
2016-03-04 21:52:11 URL:https://reviews.apache.org/r/44396/diff/raw/ 
[15995/15995] -> "44396.patch" [1]
Total errors found: 0
Checking 9 files
Error: Commit message summary (the first line) must not exceed 72 characters.

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

- Mesos ReviewBot


On March 4, 2016, 4:49 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44396/
> ---
> 
> (Updated March 4, 2016, 4:49 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-4838
> https://issues.apache.org/jira/browse/MESOS-4838
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated unavailable in batch to avoid several allocate(slaveId) call.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp a4743c5a31b18d96722a42d526bfb669d30e6e48 
>   src/master/allocator/mesos/allocator.hpp 
> 64bce0fb143b109c26f923cd97d5facb393dee9d 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/http.cpp 8276baa538eb4d2aaf54cc1aa516bffaadacc4dd 
>   src/master/master.hpp ea26670e6c6c67314406fded510e8fdd46053dc8 
>   src/master/master.cpp 57ff4a39039f573b8586bc03f873f97826b97f6f 
>   src/tests/allocator.hpp 4081193abed29b99f60254e98ef9ebc981bde859 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44396/diff/
> 
> 
> Testing
> ---
> 
> make && make check at Mac OS
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 43884: Added allocator metrics for used quotas.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44070, 44071, 44073, 44260, 43879, 43880, 43881, 44261, 
43882, 43883, 43884]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 4, 2016, 4:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> ---
> 
> (Updated March 4, 2016, 4:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
> https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metrics for used quotas.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43884/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 44258: Fixed http endpoint trigger two inverse offer calls.

[Joris Van Remoortere]

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




src/master/http.cpp (line 2016)


can we rename this to `unavailabilities`?
`updated` suggests only ones that have been modified, which may be why we 
missed this problem before.



src/master/http.cpp (lines 2026 - 2027)


Can you explain how machines going from `UP` to `DOWN` are handled in the 
next loop?
I see logic for `UP` to `DRAINING` in the next loop.

Also missing a backtick after `UP`



src/master/http.cpp (line 2028)


Comments should be in sentence form: `merge` -> `Merge`.



src/master/http.cpp (lines 2031 - 2033)


For some of these early exit conditions, does it make sense to add `CHECK`s 
(and maybe event comments) to document why we are exiting?
Stating *that* we are exiting less helpful to readers than *why*.

I think the implied invariant here (which we should call out explicitly) is 
that any machine should only be "touched" by 1 of the 2 loops here. The exit 
conditions between them are meant to enforce this exclusion?


- Joris Van Remoortere


On March 4, 2016, 2:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44258/
> ---
> 
> (Updated March 4, 2016, 2:10 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Joris Van Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-4831
> https://issues.apache.org/jira/browse/MESOS-4831
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There is a bug when setting host maintain with http endpoint: 
> https://github.com/apache/mesos/blob/master/src/master/http.cpp#L1987-L2021
> The logic is as this:
> 1) Get all host list from maintain window and put it to updated hashmap.
> 2) If the machine in was in updated was also in master->machines, call master 
> updateUnavailability to trigger recoverResources, updateUnavailability etc in 
> allocator
> 3) Otherwise, clear the unavailabity time window for the machine.
> 4) Update each new machines in updated to call master updateUnavailability
> 
> But the logic in step 4) is getting all machines from the schedule windows 
> but not the machines that is new to the cluster, this caused master get two 
> updateUnavailability calls for a machine in the updated hashmap.
> 
> The fix is filter machines in updated hashmap when handling new machines.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8276baa538eb4d2aaf54cc1aa516bffaadacc4dd 
>   src/tests/master_maintenance_tests.cpp 
> 3faa8136cf57276295553910319480028f433e4c 
> 
> Diff: https://reviews.apache.org/r/44258/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
>  ./bin/mesos-tests.sh --gtest_filter="MasterMaintenanceTest.*" --verbose
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 43293: Ignored invalid env vars.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43293]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 4, 2016, 3:34 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43293/
> ---
> 
> (Updated March 4, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4607
> https://issues.apache.org/jira/browse/MESOS-4607
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignored invalid env vars when creating docker image.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 52728707d985517e57525af7e470ccb468039373 
>   src/tests/containerizer/docker_tests.cpp 
> 7ef52ade0d3389f9e24e3c5c7dda4f8809b9d83f 
> 
> Diff: https://reviews.apache.org/r/43293/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> $ GLOG_v=2 ./bin/mesos-tests.sh  
> --gtest_filter="DockerImageTest.ParseInspectonImage" --verbose
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerImageTest
> [ RUN  ] DockerImageTest.ParseInspectonImage
> I0216 12:13:00.501356 1918300928 process.cpp:2489] Spawned process 
> files@192.168.1.102:54061
> I0216 12:13:00.501369 216694784 process.cpp:2499] Resuming 
> files@192.168.1.102:54061 at 2016-02-16 04:13:00.501399040+00:00
> I0216 12:13:00.501513 217231360 process.cpp:2499] Resuming 
> help@192.168.1.102:54061 at 2016-02-16 04:13:00.501527040+00:00
> I0216 12:13:00.505641 1918300928 docker.cpp:398] Overriding the environment 
> variable 'JAVA_VERSION' from '8u66' to '8u77'
> W0216 12:13:00.505677 1918300928 docker.cpp:391] Skipping invalid environment 
> variable 'JAVA_VERSION+8u66' for 'ContainerConfig.Env'
> I0216 12:13:00.506271 214548480 process.cpp:2499] Resuming 
> AuthenticationRouter(1)@192.168.1.102:54061 at 2016-02-16 
> 04:13:00.506306048+00:00
> I0216 12:13:00.506393 216158208 process.cpp:2499] Resuming 
> files@192.168.1.102:54061 at 2016-02-16 04:13:00.506411008+00:00
> I0216 12:13:00.506433 216158208 process.cpp:2604] Cleaning up 
> files@192.168.1.102:54061
> I0216 12:13:00.506475 215621632 process.cpp:2499] Resuming 
> help@192.168.1.102:54061 at 2016-02-16 04:13:00.506503168+00:00
> [   OK ] DockerImageTest.ParseInspectonImage (6 ms)
> [--] 1 test from DockerImageTest (6 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (18 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 44380: Change IOTest.BufferedRead to write to the temporary directory.

[Yong Tang]

> On March 4, 2016, 7:42 a.m., Benjamin Bannier wrote:
> > I think using a `TemporaryDirectoryTest` fixture is the right approach, but 
> > what I find unfortunate about your approach is that with this patch we'd 
> > create a temporary directory and incur the overhead for all tests in this 
> > suite, even ones not creating any files. If that's something we wouldn't 
> > want, what about using separate suites for tests creating files (that would 
> > be `BufferedRead` and `Redirect`), and other ones?

Hi Benjamin, thanks for the suggestion. I splits IOTest into two test suites. 
Those tests that will not write files to disk remains the same while the rests 
(BufferedRead and Redirect) have been moved to TemporaryDirectoryIOTest via 
TemporaryDirectoryTest fixture. Let me see if there are any issues and I will 
fix it.


- Yong


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


On March 4, 2016, 6:56 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44380/
> ---
> 
> (Updated March 4, 2016, 6:56 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-4807
> https://issues.apache.org/jira/browse/MESOS-4807
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit changes IOTest.BufferedRead so that this specific
> test (not all IOTest) could be executed from temporary
> directories via TemporaryDirectoryTest fixture (MESOS-4807).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/io_tests.cpp 
> 2bffc7cd9c3aa204a1d1b8eb45f0bff12f49ca62 
> 
> Diff: https://reviews.apache.org/r/44380/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 41049: New python lib with only the executor driver.

[Steve Niemitz]

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

(Updated March 4, 2016, 7:51 p.m.)


Review request for mesos, Till Toenshoff and Vinod Kone.


Changes
---

Aha, I think this email got filtered because "Groups" is not set to "mesos". 
Fixing it now. This will also let ReviewBot test this review.


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


Repository: mesos


Description
---

New python lib with only the executor driver.

This patch produces a new python egg, mesos.executor, which contains only the 
code needed to create a MesosExecutorDriver.  By doing so, the linker can 
remove unused code in libmesos_no_3rdparty.a, and therefor not include any 
external dependencies in the resulting _mesos.so.


Diffs
-

  configure.ac b30a8d30076f3068fd7d5fc8ccea1982639e998a 
  src/Makefile.am 0409491b92c9720d60ad76fdbc2edff554fb4965 
  src/python/executor/setup.py.in PRE-CREATION 
  src/python/executor/src/mesos/__init__.py PRE-CREATION 
  src/python/executor/src/mesos/executor/__init__.py PRE-CREATION 
  src/python/executor/src/mesos/executor/module.cpp PRE-CREATION 
  src/python/native/ext_modules.py.in 4682e5eed0f7be23fb48ef628e1bebc7741431d7 
  src/python/native/setup.py.in 49ed61293281f65d6597470ce3697326ac769032 
  src/python/native/src/mesos/native/__init__.py 
226f94357cd81b700706edd68c94de461647fa1b 
  src/python/native/src/mesos/native/mesos_executor_driver_impl.hpp  
  src/python/native/src/mesos/native/mesos_executor_driver_impl.cpp 
7838a07ac12034a5eed2f459bd11e5e0a07e94de 
  src/python/native/src/mesos/native/mesos_scheduler_driver_impl.hpp  
  src/python/native/src/mesos/native/mesos_scheduler_driver_impl.cpp 
f8be49bf5feb6675c3c8e6a1df75a5e079d09fcc 
  src/python/native/src/mesos/native/module.hpp 
2cf7b57077d10c071bd4b8f36e2cb43041fc168d 
  src/python/native/src/mesos/native/module.cpp 
6e672a21edf0e9df1d95688620ea9cc6a1b2 
  src/python/native/src/mesos/native/proxy_executor.hpp  
  src/python/native/src/mesos/native/proxy_executor.cpp 
706f417b547bb9878346902b2a2f98b1afade5ad 
  src/python/native/src/mesos/native/proxy_scheduler.hpp  
  src/python/native/src/mesos/native/proxy_scheduler.cpp 
8afb3380d99b4dc9c4f7b3d926f4a5b1d6e94c20 
  src/python/native_common/ext_modules.py.in PRE-CREATION 
  src/python/scheduler/setup.py.in PRE-CREATION 
  src/python/scheduler/src/mesos/__init__.py PRE-CREATION 
  src/python/scheduler/src/mesos/scheduler/__init__.py PRE-CREATION 

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


Testing
---

On CentOS 7, GCC 4.8.5:
- make distcheck
- Tested with aurora executor
- Made sure mesos.scheduler still worked
- Made sure mesos.native still worked


Thanks,

Steve Niemitz

Re: Review Request 43701: Added a command executor based on the new V1 API.

[Vinod Kone]

> On Feb. 29, 2016, 2:24 a.m., Vinod Kone wrote:
> > Thanks for working on this Qian!
> > 
> > It's really hard to tell what changes were made to the http command 
> > executor that are different from the command executor. I would suggest you 
> > to split this into multiple reviews to make reviewers' life easy.
> > 
> > 1) Add http command executor to make files. Just copy executor.cpp to 
> > http_command_executor.cpp without any changes.
> > 2) Update http_command_executor.cpp to use v1 API.
> > 3) Make changes to flags.cpp and slave.cpp.
> > 4) Update/parameterize tests (slave recovery tests?) to use http command 
> > executor.
> 
> Qian Zhang wrote:
> Sure Vinod, I will discard this patch, and upload multiple sub-patches 
> accordingly.

Hey any updates on this?


- Vinod


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


On Feb. 20, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> ---
> 
> (Updated Feb. 20, 2016, 1:44 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a command executor based on the new V1 API.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44288: Changed MasterDetector/Contender namespace.

[Anurag Singh]

> On March 3, 2016, 10:15 p.m., Joseph Wu wrote:
> > src/master/contender.hpp, lines 17-18
> > 
> >
> > This whole file seems like a pretty substantial change.  I'd recommend 
> > pulling it out into a separate review (rather than hiding it in this 
> > review).
> > 
> > Also, you'll want to consider making folders "contenders" and 
> > "detectors".  Then renaming this file "standalone.hpp".
> 
> Anurag Singh wrote:
> Ok. Although then zookeeper's classes should also go into their own files.

Looking at this again, the changes in this file are really just namespace 
changes (the only thing substantial is the removal of the MasterContender class 
definition) - I can't put them into a different commit without breaking the 
build (I'm trying to make sure individual commits don't break builds, which I 
think is a sensible goal). However, I can leave this commit as is but create a 
separate commit to create the contenders/detectors directories. Is that 
acceptable to you?


- Anurag


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


On March 3, 2016, 5:30 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44288/
> ---
> 
> (Updated March 3, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also modified users of MasterContender and MasterDetector to use this
> namespace.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
>   include/mesos/v1/scheduler.hpp a0fb73be2178171497dcce5c497ca38c3002f667 
>   src/cli/resolve.cpp 257e29034abf32491511f9a4e476b6859714829d 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/contender.hpp 3fd20f8e94daab349b76d8f5ecc87398a187a847 
>   src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
>   src/master/detector.hpp eb5d2a90b60c629150ddf04acf00f0edca1ca723 
>   src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/master/master.hpp 13c6ff153e77c527822309e787942eb463d59e7d 
>   src/master/master.cpp f242b70a0ebeff9cd3204343735b8615230a6108 
>   src/sched/sched.cpp 525255eec808c3fe5c0e38b3d1a2086bbd4eb171 
>   src/scheduler/scheduler.cpp 7ea1c2567f37a73160bca346a25bb2f0c54e71a0 
>   src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 
>   src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
>   src/tests/fault_tolerance_tests.cpp 
> f2b8dba809518cf716b2b5a7a6a8a5fe62e57646 
>   src/tests/master_allocator_tests.cpp 
> cba7c36471f93b678d94e1da0251a28a893696b1 
>   src/tests/master_authorization_tests.cpp 
> 29c89fb11da792c3e71eb880a19657ea225b3cc8 
>   src/tests/master_contender_detector_tests.cpp 
> 255ab8119a04b55bb4f1b61dee19c4be64499376 
>   src/tests/master_slave_reconciliation_tests.cpp 
> d41178eb41df519073fc0890c5716bbc9fed6ad2 
>   src/tests/master_tests.cpp 36595772b34bcb8d37dbc74d247bdf4614f10150 
>   src/tests/oversubscription_tests.cpp 
> e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
>   src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
>   src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 
>   src/tests/reservation_tests.cpp d7f9de6f2bce061316916260f356efdb96ecd482 
>   src/tests/scheduler_event_call_tests.cpp 
> 8c02ceeb3ec1783cb2f63f100700508e70f586e4 
>   src/tests/scheduler_http_api_tests.cpp 
> 428e12646d80b45daec30cfe607b97f36170fdf5 
>   src/tests/scheduler_tests.cpp 2b1693eaf1a6106f5e7d269e4e3f6c353dd6f017 
>   src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
>   src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 
> 
> Diff: https://reviews.apache.org/r/44288/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/44289/.
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

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




src/slave/containerizer/mesos/isolators/network/cni.cpp (line 45)


Remove the period at the end. We don't have terminal sentences in ERROR 
strings.



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 50)


Remove period.



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 61)


Add a `+` at the end. While not necessary, makes it more readable.


- Avinash sridharan


On March 4, 2016, 5:07 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 5:07 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 80
> > 
> >
> > Shouldn't we do a size check here as well ? Bail out if we don't have 
> > any files in this directory ?
> 
> Qian Zhang wrote:
> The check `if (netConfigs.size() == 0) {` below it has already covered 
> that case.

Yeah I see that. Was just wondering if it makes sense to do the `foreach` if 
size is 0?


- Avinash


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


On March 4, 2016, 5:07 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 5:07 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44380: Change IOTest.BufferedRead to write to the temporary directory.

[Yong Tang]

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

(Updated March 4, 2016, 6:56 p.m.)


Review request for mesos and Benjamin Bannier.


Changes
---

Split IOTest into two test suites. One is the IOTest that will not write files 
to disk, it will remains the same. Another is the TemporaryDirectoryIOTest 
fixture that includes BufferedRead and Redirect which will write files (to 
temporary directory) to disk.


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


Repository: mesos


Description (updated)
---

This commit changes IOTest.BufferedRead so that this specific
test (not all IOTest) could be executed from temporary
directories via TemporaryDirectoryTest fixture (MESOS-4807).


Diffs (updated)
-

  3rdparty/libprocess/src/tests/io_tests.cpp 
2bffc7cd9c3aa204a1d1b8eb45f0bff12f49ca62 

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


Testing
---

make check


Thanks,

Yong Tang

Re: Review Request 44391: Added document for overlayfs backend.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44390, 44391]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 4, 2016, 3:18 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44391/
> ---
> 
> (Updated March 4, 2016, 3:18 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4866
> https://issues.apache.org/jira/browse/MESOS-4866
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added document for overlayfs backend.
> 
> 
> Diffs
> -
> 
>   docs/mesos-provisioner.md 1b19406cb93bbc3f5330eaf9d29b1be98a674136 
> 
> Diff: https://reviews.apache.org/r/44391/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On March 4, 2016, 4:14 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 4, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, lines 108-115
> > 
> >
> > isn't this the same as !nameValue.isSome() ?
> > 
> > Also LOG(WARNING) should be LOG(ERROR)
> 
> Qian Zhang wrote:
> Please see 
> https://github.com/apache/mesos/blob/0.27.1/src/docker/docker.cpp#L255:L260 
> for the existing way to handle the return value of `json.get().find()`.

Well... !isSome implies isNone or isError is true which is what you are aiming 
for ? Will simplify things I guess.


> On March 4, 2016, 3:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 134
> > 
> >
> > I don't understand this comment. We just made sure the plugin does not 
> > exist? So what does the comment imply "it can
> > // still be valid as long as operator puts the CNI plugin binary
> > // that it uses under '--network_cni_plugins_dir'." ?
> > 
> > I think at this point we should return an error. If can't find an 
> > executable for a named network, the behavior will become undefined. We 
> > should bail at this point.
> 
> Qian Zhang wrote:
> My point is, if we can not find a plugin for a named network during 
> initilization, log a warning message to let operator know this issue, and 
> afterward operator can put the plugin in the plugin directory without 
> restarting agent, then the named network can still work.

Lets not rely on the operator heeding WARNING messages and fixing the problem. 
My concern is that this is a `FATAL` error since before the operator can 
rectify the error if containers are launched the behavior becomes undefined.


- Avinash


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


On March 4, 2016, 5:07 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 5:07 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43629: Especially updated tests to use the updated MesosTest helpers.

[Joseph Wu]

> On March 3, 2016, 5:55 a.m., Bernd Mathiske wrote:
> > src/tests/slave_recovery_tests.cpp, line 3461
> > 
> >
> > Why was this moved up here? Couldn't this be in line 3389/3402?
> 
> Joseph Wu wrote:
> In this case, it's because all injections of `Try 
> slave` must be stack-allocated above.  Even though we use `containerizer2` 
> much later, if we put it lower down, it would be deallocated before the 
> second `slave` (and then segfault!).
> 
> Bernd Mathiske wrote:
> I am not following yet. 
> 
> 1. How could it be deallocated before it has been allocated? How could it 
> even be referenced then? I assume "it" refers to sontainerizer2. 
> 2. The second slave would still be constructed later than containerizer2.

In the updated test, the items go on the stack like this:
```
containerizer1
containerizer2
slave
```

If I don't move `containerizer2` up, the stack will look like this:
```
containerizer1
slave
containerizer2
```
because re-constructing `slave` doesn't change `slave`'s order in the stack.


- Joseph


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


On March 2, 2016, 1:45 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43629/
> ---
> 
> (Updated March 2, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Continuation of https://reviews.apache.org/r/43615/ with a slightly different 
> pattern.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 
>   src/tests/resource_offers_tests.cpp 
> 0bad45dd1dabecc88fef1ab46e8ea26718070b33 
>   src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
> 
> Diff: https://reviews.apache.org/r/43629/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 44336: Removed numeric suffixes where appropriate in allocator tests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43105, 44331, 44332, 44333, 43138, 44334, 44335, 44336]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 4, 2016, 2:52 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44336/
> ---
> 
> (Updated March 4, 2016, 2:52 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We should not enumerate variables if the corresponding instance is the
> only one of such type in the test.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44336/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `make check`
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44288: Changed MasterDetector/Contender namespace.

[Anurag Singh]

> On March 3, 2016, 10:15 p.m., Joseph Wu wrote:
> > src/master/main.cpp, line 61
> > 
> >
> > What about this header?

Do you mean why is this header not ""? It's 
different from  - it includes the 
StandaloneMasterDetector and ZooperZooKeeperMasterDetector class definitions. 
Although, after adding the contender and detect directories, these includes 
will need to change.


- Anurag


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


On March 3, 2016, 5:30 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44288/
> ---
> 
> (Updated March 3, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also modified users of MasterContender and MasterDetector to use this
> namespace.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
>   include/mesos/v1/scheduler.hpp a0fb73be2178171497dcce5c497ca38c3002f667 
>   src/cli/resolve.cpp 257e29034abf32491511f9a4e476b6859714829d 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/contender.hpp 3fd20f8e94daab349b76d8f5ecc87398a187a847 
>   src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
>   src/master/detector.hpp eb5d2a90b60c629150ddf04acf00f0edca1ca723 
>   src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/master/master.hpp 13c6ff153e77c527822309e787942eb463d59e7d 
>   src/master/master.cpp f242b70a0ebeff9cd3204343735b8615230a6108 
>   src/sched/sched.cpp 525255eec808c3fe5c0e38b3d1a2086bbd4eb171 
>   src/scheduler/scheduler.cpp 7ea1c2567f37a73160bca346a25bb2f0c54e71a0 
>   src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 
>   src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
>   src/tests/fault_tolerance_tests.cpp 
> f2b8dba809518cf716b2b5a7a6a8a5fe62e57646 
>   src/tests/master_allocator_tests.cpp 
> cba7c36471f93b678d94e1da0251a28a893696b1 
>   src/tests/master_authorization_tests.cpp 
> 29c89fb11da792c3e71eb880a19657ea225b3cc8 
>   src/tests/master_contender_detector_tests.cpp 
> 255ab8119a04b55bb4f1b61dee19c4be64499376 
>   src/tests/master_slave_reconciliation_tests.cpp 
> d41178eb41df519073fc0890c5716bbc9fed6ad2 
>   src/tests/master_tests.cpp 36595772b34bcb8d37dbc74d247bdf4614f10150 
>   src/tests/oversubscription_tests.cpp 
> e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
>   src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
>   src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 
>   src/tests/reservation_tests.cpp d7f9de6f2bce061316916260f356efdb96ecd482 
>   src/tests/scheduler_event_call_tests.cpp 
> 8c02ceeb3ec1783cb2f63f100700508e70f586e4 
>   src/tests/scheduler_http_api_tests.cpp 
> 428e12646d80b45daec30cfe607b97f36170fdf5 
>   src/tests/scheduler_tests.cpp 2b1693eaf1a6106f5e7d269e4e3f6c353dd6f017 
>   src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
>   src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 
> 
> Diff: https://reviews.apache.org/r/44288/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/44289/.
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

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

(Updated March 5, 2016, 1:07 a.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Added the framework of 'network/cni' isolator.


Diffs (updated)
-

  src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
  src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
  src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 28
> > 
> >
> > Maybe:
> >  The isolator implements support for Container Network Interface (CNI) 
> > specification  . It provides network isolation to 
> > containers by creating a network namespace for each container, and then 
> > adding the container to the CNI network sepcified in the `NetworkInfo` for 
> > the container. It adds the container to a CNI network by using CNI plugins 
> > specified by the operator for the corresponding CNI network.

Agree.


> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 80
> > 
> >
> > Shouldn't we do a size check here as well ? Bail out if we don't have 
> > any files in this directory ?

The check `if (netConfigs.size() == 0) {` below it has already covered that 
case.


> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, lines 108-115
> > 
> >
> > isn't this the same as !nameValue.isSome() ?
> > 
> > Also LOG(WARNING) should be LOG(ERROR)

Please see 
https://github.com/apache/mesos/blob/0.27.1/src/docker/docker.cpp#L255:L260 for 
the existing way to handle the return value of `json.get().find()`.


> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 134
> > 
> >
> > I don't understand this comment. We just made sure the plugin does not 
> > exist? So what does the comment imply "it can
> > // still be valid as long as operator puts the CNI plugin binary
> > // that it uses under '--network_cni_plugins_dir'." ?
> > 
> > I think at this point we should return an error. If can't find an 
> > executable for a named network, the behavior will become undefined. We 
> > should bail at this point.

My point is, if we can not find a plugin for a named network during 
initilization, log a warning message to let operator know this issue, and 
afterward operator can put the plugin in the plugin directory without 
restarting agent, then the named network can still work.


- Qian


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


On March 4, 2016, 10:34 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 10:34 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 44396: Updated unavailable in batch to avoid several allocate(slaveId) call in "/maintenance/schedule".

[Klaus Ma]

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

Review request for mesos, Joris Van Remoortere and Joseph Wu.


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


Repository: mesos


Description
---

Updated unavailable in batch to avoid several allocate(slaveId) call.


Diffs
-

  include/mesos/master/allocator.hpp a4743c5a31b18d96722a42d526bfb669d30e6e48 
  src/master/allocator/mesos/allocator.hpp 
64bce0fb143b109c26f923cd97d5facb393dee9d 
  src/master/allocator/mesos/hierarchical.hpp 
3043888630b066505410d3b32c5b3f813cc458c1 
  src/master/allocator/mesos/hierarchical.cpp 
70291075c00a9a557529c2562dedcfc6c6c3ec32 
  src/master/http.cpp 8276baa538eb4d2aaf54cc1aa516bffaadacc4dd 
  src/master/master.hpp ea26670e6c6c67314406fded510e8fdd46053dc8 
  src/master/master.cpp 57ff4a39039f573b8586bc03f873f97826b97f6f 
  src/tests/allocator.hpp 4081193abed29b99f60254e98ef9ebc981bde859 
  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

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


Testing
---

make && make check at Mac OS


Thanks,

Klaus Ma

Re: Review Request 44288: Changed MasterDetector/Contender namespace.

[Anurag Singh]

> On March 3, 2016, 10:15 p.m., Joseph Wu wrote:
> > src/master/contender.hpp, lines 17-18
> > 
> >
> > This whole file seems like a pretty substantial change.  I'd recommend 
> > pulling it out into a separate review (rather than hiding it in this 
> > review).
> > 
> > Also, you'll want to consider making folders "contenders" and 
> > "detectors".  Then renaming this file "standalone.hpp".

Ok. Although then zookeeper's classes should also go into their own files.


- Anurag


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


On March 3, 2016, 5:30 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44288/
> ---
> 
> (Updated March 3, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also modified users of MasterContender and MasterDetector to use this
> namespace.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
>   include/mesos/v1/scheduler.hpp a0fb73be2178171497dcce5c497ca38c3002f667 
>   src/cli/resolve.cpp 257e29034abf32491511f9a4e476b6859714829d 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/contender.hpp 3fd20f8e94daab349b76d8f5ecc87398a187a847 
>   src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
>   src/master/detector.hpp eb5d2a90b60c629150ddf04acf00f0edca1ca723 
>   src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/master/master.hpp 13c6ff153e77c527822309e787942eb463d59e7d 
>   src/master/master.cpp f242b70a0ebeff9cd3204343735b8615230a6108 
>   src/sched/sched.cpp 525255eec808c3fe5c0e38b3d1a2086bbd4eb171 
>   src/scheduler/scheduler.cpp 7ea1c2567f37a73160bca346a25bb2f0c54e71a0 
>   src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 
>   src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
>   src/tests/fault_tolerance_tests.cpp 
> f2b8dba809518cf716b2b5a7a6a8a5fe62e57646 
>   src/tests/master_allocator_tests.cpp 
> cba7c36471f93b678d94e1da0251a28a893696b1 
>   src/tests/master_authorization_tests.cpp 
> 29c89fb11da792c3e71eb880a19657ea225b3cc8 
>   src/tests/master_contender_detector_tests.cpp 
> 255ab8119a04b55bb4f1b61dee19c4be64499376 
>   src/tests/master_slave_reconciliation_tests.cpp 
> d41178eb41df519073fc0890c5716bbc9fed6ad2 
>   src/tests/master_tests.cpp 36595772b34bcb8d37dbc74d247bdf4614f10150 
>   src/tests/oversubscription_tests.cpp 
> e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
>   src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
>   src/tests/persistent_volume_tests.cpp 
> bf19c81fbcf973d1ac27fbd42eedfd7118b7ba50 
>   src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 
>   src/tests/reservation_tests.cpp d7f9de6f2bce061316916260f356efdb96ecd482 
>   src/tests/scheduler_event_call_tests.cpp 
> 8c02ceeb3ec1783cb2f63f100700508e70f586e4 
>   src/tests/scheduler_http_api_tests.cpp 
> 428e12646d80b45daec30cfe607b97f36170fdf5 
>   src/tests/scheduler_tests.cpp 2b1693eaf1a6106f5e7d269e4e3f6c353dd6f017 
>   src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
>   src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 
> 
> Diff: https://reviews.apache.org/r/44288/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/44289/.
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Qian Zhang]

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

(Updated March 5, 2016, 12:14 a.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Add agent flags for specifying CNI plugin and config directories.


Diffs (updated)
-

  docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
  src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
  src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44200, 44269]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 4, 2016, 2:34 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 2:34 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43883: Added allocator metrics for the number of offer filters per framework.

[Benjamin Bannier]

> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 288-297
> > 
> >
> > I suggest we create a helper in `Metrics` for this. Also, minor nit: in 
> > previous patches you didn't separate `.put()` from 
> > `process::metrics::add()` with a blank line.

I introduced a number of helpers in the latest incarnation of this series, 
among others this one. Blank lines should be consistent now.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 293
> > 
> >
> > I believe this one requires `->self()` as well.

Code got moved to helper, and adjusted there.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 368-369
> > 
> >
> > Same here.

Done.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 538
> > 
> >
> > Why do explicitly use double `0` here? I thought you rely on implicit 
> > conversions and promotions in these patches.

This was a simple oversight, fixed now.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2462-2466
> > 
> >
> > Here you can also check that 2 counters for offers filters were 
> > installed.

We do now.


- Benjamin


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


On March 4, 2016, 5:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43883/
> ---
> 
> (Updated March 4, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4722
> https://issues.apache.org/jira/browse/MESOS-4722
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43883/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 43881: Added allocator metric for the number of allocations to a framework.

[Benjamin Bannier]

> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 275-280
> > 
> >
> > I would pull this into a helper in `Metrics` similar to 
> > `createGaugesForResource()`. This way we keep the allocator code minimal.

Done.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 344-347
> > 
> >
> > Maybe here a helper as well.

Done.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1498-1499
> > 
> >
> > I think this should be around `allocated()` calls for sorters. When 
> > `allocated()` is called, the sorter's internal allocation counter is 
> > incremented. I'd say we should keep the counters in sync.
> > 
> > Alternatively, we can expose the allocations counter from sorters and 
> > query it directly.

For now I put these in the locations where a `Sorter`'s `allocated` is called.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 59-60
> > 
> >
> > If you say `using process::metrics::Counter` at the top, you can fit 
> > this into one line : ).

Done, also for other `Metric`s in introduced here in this series.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2464
> > 
> >
> > Backtick `framework1` please.

Done.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2464-2472
> > 
> >
> > Do you want to set quota as part of your test? Because otherwise we can 
> > simplify the test a bit by not setting it and having `agent2` allocated to 
> > `framework2`.

It does not hurt introducing it here, and we will make more explicit use of 
this information at a later point in this series.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2487
> > 
> >
> > Feel free to kill this line

It's dead, Alex :D


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2492-2495
> > 
> >
> > How about 
> > ```
> >   Clock::advance(flags.allocation_interval);
> >   Clock::settle();
> >   ++allocations;
> > ```
> > to keep clock manipulation together?

So be it.


- Benjamin


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


On March 4, 2016, 5:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> ---
> 
> (Updated March 4, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43881/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 43880: Added allocator metrics for total and allocated scalar resources.

[Benjamin Bannier]

> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.hpp, line 27
> > 
> >
> > Kill this line.

As if it never was there now.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.cpp, lines 61-63
> > 
> >
> > Does it make sense to store the pointer to 
> > `HierarchicalAllocatorProcess` in the `Metrics` instance and remove the 
> > first parameter here? The value of this approach will be more evident when 
> > we will have more helpers later.

Yes, that makes sense. This puts some more restrictions on how an allocator 
`Metrics` can be used. For now I made the class e.g., non-copyable and added a 
`NOTE` on the lifetime dependency with the `HierarchicalAllocatorProcess`.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2406
> > 
> >
> > s/users/frameworks

Done.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2420-2424
> > 
> >
> > Can you move it up where you check allocated/* metrics? Adding a 
> > framework shouldn't influence this counter.

I think this moved down here in some cleanup, I moved it back up.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2449-2453
> > 
> >
> > I think resources will be offered to `framework2`. But is it actually 
> > important? I think you can kill this comment altogether (also the similar 
> > one above), and add a short one around `settle()`.

Done.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2463
> > 
> >
> > Feel free to kill this line.

It didn't leave a trace.


> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2491
> > 
> >
> > /cluster/cluster state/

Stated as such.


- Benjamin


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


On March 4, 2016, 5:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated March 4, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4720
> https://issues.apache.org/jira/browse/MESOS-4720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43880/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 43884: Added allocator metrics for used quotas.

[Benjamin Bannier]

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

(Updated March 4, 2016, 5:01 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Adressed comments from Alex.


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


Repository: mesos


Description
---

Added allocator metrics for used quotas.


Diffs (updated)
-

  docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
  src/master/allocator/mesos/hierarchical.hpp 
3043888630b066505410d3b32c5b3f813cc458c1 
  src/master/allocator/mesos/hierarchical.cpp 
70291075c00a9a557529c2562dedcfc6c6c3ec32 
  src/master/allocator/mesos/metrics.hpp PRE-CREATION 
  src/master/allocator/mesos/metrics.cpp PRE-CREATION 
  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

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


Testing
---

make check (OS X)

I confirmed that this does not lead to general performance regressions in the 
allocator; this is partially expected since the added code only inserts metrics 
in the allocator while the actual work is perform asynchronously. These tests 
where performed with `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an 
optimized build under OS X using clang(trunk) as compiler.


Thanks,

Benjamin Bannier

Re: Review Request 43884: Added allocator metrics for used quotas.

[Benjamin Bannier]

> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > docs/monitoring.md, line 905
> > 
> >
> > In the same vein as `allocator/allocated/KIND`, this should be 
> > something like `allocator/quota/ROLE/allocated/KIND`, right?

Done.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > docs/monitoring.md, line 908
> > 
> >
> > s/in use/allocated

Done.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1081-1092
> > 
> >
> > Here we will be also adding a gauge for "set" quotas, a candidate for a 
> > helper in `Metrics`

Set quotas aren't part of this series as they can currently be obtained from a 
master endpoint, while quota allocations cannot.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1124-1132
> > 
> >
> > Let's do it in `Metrics` as well.

Done.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1820
> > 
> >
> > Let's be consistent and call it `_quota_allocated`. Same for the local 
> > variable.

Done.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.hpp, lines 74-75
> > 
> >
> > let's call it `quota_allocated`.

Done.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2474
> > 
> >
> > s/famework/framework

Done.


> On March 4, 2016, 2:46 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/metrics.hpp, line 75
> > 
> >
> > two spaces

I might be wrong, but don't we indent by four spaces, unless we wrap an 
assignment, in which case we indent by two spaces? There's no assignment here.


- Benjamin


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


On March 4, 2016, 5:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> ---
> 
> (Updated March 4, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
> https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metrics for used quotas.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43884/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 43880: Added allocator metrics for total and allocated scalar resources.

[Benjamin Bannier]

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

(Updated March 4, 2016, 5:01 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Adressed comments from Alex.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
  src/master/allocator/mesos/hierarchical.hpp 
3043888630b066505410d3b32c5b3f813cc458c1 
  src/master/allocator/mesos/hierarchical.cpp 
70291075c00a9a557529c2562dedcfc6c6c3ec32 
  src/master/allocator/mesos/metrics.hpp PRE-CREATION 
  src/master/allocator/mesos/metrics.cpp PRE-CREATION 
  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

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


Testing
---

make check (OS X)

I confirmed that this does not lead to general performance regressions in the 
allocator; this is partially expected since the added code only inserts metrics 
in the allocator while the actual work is perform asynchronously. These tests 
where performed with `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an 
optimized build under OS X using clang(trunk) as compiler.


Thanks,

Benjamin Bannier

Re: Review Request 43293: Ignored invalid env vars.

[Guangya Liu]

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

(Updated March 4, 2016, 3:34 p.m.)


Review request for mesos, haosdent huang, Jie Yu, and Timothy Chen.


Changes
---

Rebase


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


Repository: mesos


Description
---

Ignored invalid env vars when creating docker image.


Diffs (updated)
-

  src/docker/docker.cpp 52728707d985517e57525af7e470ccb468039373 
  src/tests/containerizer/docker_tests.cpp 
7ef52ade0d3389f9e24e3c5c7dda4f8809b9d83f 

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


Testing
---

make
make check

$ GLOG_v=2 ./bin/mesos-tests.sh  
--gtest_filter="DockerImageTest.ParseInspectonImage" --verbose
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from DockerImageTest
[ RUN  ] DockerImageTest.ParseInspectonImage
I0216 12:13:00.501356 1918300928 process.cpp:2489] Spawned process 
files@192.168.1.102:54061
I0216 12:13:00.501369 216694784 process.cpp:2499] Resuming 
files@192.168.1.102:54061 at 2016-02-16 04:13:00.501399040+00:00
I0216 12:13:00.501513 217231360 process.cpp:2499] Resuming 
help@192.168.1.102:54061 at 2016-02-16 04:13:00.501527040+00:00
I0216 12:13:00.505641 1918300928 docker.cpp:398] Overriding the environment 
variable 'JAVA_VERSION' from '8u66' to '8u77'
W0216 12:13:00.505677 1918300928 docker.cpp:391] Skipping invalid environment 
variable 'JAVA_VERSION+8u66' for 'ContainerConfig.Env'
I0216 12:13:00.506271 214548480 process.cpp:2499] Resuming 
AuthenticationRouter(1)@192.168.1.102:54061 at 2016-02-16 
04:13:00.506306048+00:00
I0216 12:13:00.506393 216158208 process.cpp:2499] Resuming 
files@192.168.1.102:54061 at 2016-02-16 04:13:00.506411008+00:00
I0216 12:13:00.506433 216158208 process.cpp:2604] Cleaning up 
files@192.168.1.102:54061
I0216 12:13:00.506475 215621632 process.cpp:2499] Resuming 
help@192.168.1.102:54061 at 2016-02-16 04:13:00.506503168+00:00
[   OK ] DockerImageTest.ParseInspectonImage (6 ms)
[--] 1 test from DockerImageTest (6 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (18 ms total)
[  PASSED  ] 1 test.


Thanks,

Guangya Liu

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Avinash sridharan]

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




src/slave/containerizer/mesos/isolators/network/cni.hpp (line 28)


Maybe:
 The isolator implements support for Container Network Interface (CNI) 
specification  . It provides network isolation to 
containers by creating a network namespace for each container, and then adding 
the container to the CNI network sepcified in the `NetworkInfo` for the 
container. It adds the container to a CNI network by using CNI plugins 
specified by the operator for the corresponding CNI network.



src/slave/containerizer/mesos/isolators/network/cni.hpp (line 75)


We use camel case for private variables. 

Why the const qualifier? I see that currently you are creating the hashmap 
during create and I guess you are assuming you are going to update it during 
the lifetime of the isolator, but what if in the future we allow updates to 
configs? Lets remove the const over here.



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 71)


shouldn't we be returning an error here as well ? Without any plugins CNI 
can't do much. Or can we still do operations (port forwarding, --net=none) ?



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 80)


Shouldn't we do a size check here as well ? Bail out if we don't have any 
files in this directory ?



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 81)


We use camel case for variables.



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 108)


Conver this to LOG(ERROR)



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 108 - 115)


isn't this the same as !nameValue.isSome() ?

Also LOG(WARNING) should be LOG(ERROR)



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 118 - 126)


Ditto to my comment above on `isSome`



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 129)


Can we also check if the plugin is executable? Since later we are going to 
try and execute the plugin.



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 134)


I don't understand this comment. We just made sure the plugin does not 
exist? So what does the comment imply "it can
// still be valid as long as operator puts the CNI plugin binary
// that it uses under '--network_cni_plugins_dir'." ?

I think at this point we should return an error. If can't find an 
executable for a named network, the behavior will become undefined. We should 
bail at this point.



src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 142 - 150)


we can use !isSome?



src/slave/containerizer/mesos/isolators/network/cni.cpp (line 160)


Ditto comment on bailing out if we can't verify the plugin?


- Avinash sridharan


On March 4, 2016, 2:34 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> ---
> 
> (Updated March 4, 2016, 2:34 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 44391: Added document for overlayfs backend.

[Guangya Liu]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added document for overlayfs backend.


Diffs
-

  docs/mesos-provisioner.md 1b19406cb93bbc3f5330eaf9d29b1be98a674136 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 44227: Added AWAIT_READY() call to reservation_endpoint_tests.

[Till Toenshoff]

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


Ship it!




Ship It!

- Till Toenshoff


On March 4, 2016, 3:07 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44227/
> ---
> 
> (Updated March 4, 2016, 3:07 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Michael Park.
> 
> 
> Bugs: MESOS-4002
> https://issues.apache.org/jira/browse/MESOS-4002
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Calling .get() on a future is a blocking operation and a test calling get() 
> without prior waiting for the future (e.g., AWAIT_READY()) can block forever.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_endpoints_tests.cpp 
> f3a143812aa10bc445ac5d27c00318e91eb086aa 
> 
> Diff: https://reviews.apache.org/r/44227/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44227: Added AWAIT_READY() call to reservation_endpoint_tests.

[Joerg Schad]

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

(Updated March 4, 2016, 3:07 p.m.)


Review request for mesos, Anand Mazumdar and Michael Park.


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


Repository: mesos


Description (updated)
---

Calling .get() on a future is a blocking operation and a test calling get() 
without prior waiting for the future (e.g., AWAIT_READY()) can block forever.


Diffs
-

  src/tests/reservation_endpoints_tests.cpp 
f3a143812aa10bc445ac5d27c00318e91eb086aa 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 44382: Update leveldb-1.4.patch to support PowerPC LE platform.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

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

- Mesos ReviewBot


On March 4, 2016, 9:31 a.m., Zhiwei Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44382/
> ---
> 
> (Updated March 4, 2016, 9:31 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update leveldb-1.4.patch to support PowerPC LE platform.
> 
> 
> Diffs
> -
> 
>   3rdparty/leveldb-1.4.patch ad8c19b9caa856ff85978ba832d48df11b3a83f0 
> 
> Diff: https://reviews.apache.org/r/44382/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhiwei Chen
> 
>

Re: Review Request 43629: Especially updated tests to use the updated MesosTest helpers.

[Bernd Mathiske]

> On March 3, 2016, 5:55 a.m., Bernd Mathiske wrote:
> > src/tests/slave_recovery_tests.cpp, line 3461
> > 
> >
> > Why was this moved up here? Couldn't this be in line 3389/3402?
> 
> Joseph Wu wrote:
> In this case, it's because all injections of `Try 
> slave` must be stack-allocated above.  Even though we use `containerizer2` 
> much later, if we put it lower down, it would be deallocated before the 
> second `slave` (and then segfault!).

I am not following yet. 

1. How could it be deallocated before it has been allocated? How could it even 
be referenced then? I assume "it" refers to sontainerizer2. 
2. The second slave would still be constructed later than containerizer2.


- Bernd


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


On March 2, 2016, 1:45 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43629/
> ---
> 
> (Updated March 2, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Continuation of https://reviews.apache.org/r/43615/ with a slightly different 
> pattern.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 
>   src/tests/resource_offers_tests.cpp 
> 0bad45dd1dabecc88fef1ab46e8ea26718070b33 
>   src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
> 
> Diff: https://reviews.apache.org/r/43629/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 44332: Fixed a typo in the HierarchicalAllocatorTest.CoarseGrained test.

[Alexander Rukletsov]

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

(Updated March 4, 2016, 2:52 p.m.)


Review request for mesos, Ben Mahler and Joris Van Remoortere.


Changes
---

Joris' comment


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

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


Testing
---

On Mac OS 10.10.4:

`make check`

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


Thanks,

Alexander Rukletsov

Re: Review Request 44336: Removed numeric suffixes where appropriate in allocator tests.

[Alexander Rukletsov]

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

(Updated March 4, 2016, 2:52 p.m.)


Review request for mesos, Bernd Mathiske and Ben Mahler.


Repository: mesos


Description
---

We should not enumerate variables if the corresponding instance is the
only one of such type in the test.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

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


Testing
---

On Mac OS 10.10.4:

`make check`

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


Thanks,

Alexander Rukletsov

Re: Review Request 43630: Especially updated scheduler tests to use the updated MesosTest helpers.

[Bernd Mathiske]

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


Ship it!




- Bernd Mathiske


On March 2, 2016, 1:45 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43630/
> ---
> 
> (Updated March 2, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Continuation of https://reviews.apache.org/r/43615/ with re-ordering of some 
> local variables due to the order of destruction.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 3faa8136cf57276295553910319480028f433e4c 
>   src/tests/scheduler_tests.cpp 2b1693eaf1a6106f5e7d269e4e3f6c353dd6f017 
> 
> Diff: https://reviews.apache.org/r/43630/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Avinash sridharan]

> On March 2, 2016, 4:07 p.m., Avinash sridharan wrote:
> > src/slave/flags.cpp, line 694
> > 
> >
> > s/directory/location
> > 
> > remove this line:
> > This flag is used for\n"
> >   "the `network/cni` isolator.\n",
> >   "/tmp/mesos/cni/plugins"
> 
> Qian Zhang wrote:
> Did you mean removing the line: `This flag is used for the 'network/cni' 
> isolator.`? I think this is the existing convention, please see the help 
> message of `Flags::eth0_name`, `Flags::network_enable_snmp_statistics`, 
> `Flags::container_disk_watch_interval`, etc. They all have the line like: 
> `This flag is used for the 'xxx' isolator.`.
> 
> Avinash sridharan wrote:
> Not sure what you mean by convention in help strings? The sentence is 
> redundant (doesn't given any more information than the previous sentences). 
> Lets use the correct form here.  Help strings should be concise.
> 
> Qian Zhang wrote:
> What I mean by convention in help strings is, if a flag is specific to an 
> isolator, then we should mention that in the help strings of that flag, 
> please see the following code for an example (and there are more in agent's 
> flags):
> https://github.com/apache/mesos/blob/0.27.1/src/slave/flags.cpp#L586:L587
> 
> So your comment is that I should remove the line `This flag is used for 
> the 'network/cni' isolator.` from the help strings of both 
> `Flags::network_cni_plugins_dir` and `Flags::network_cni_config_dir`?

Yup. Just remove that line.


- Avinash


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


On March 4, 2016, 2:36 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 4, 2016, 2:36 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43615: Update test suite to use the reworked MesosTest helpers.

[Bernd Mathiske]

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




src/tests/container_logger_tests.cpp (line 301)


This indentation is OK, but prefered is this:

Try slave = 
  StartSlave(detector.get(), containerizer.get(), flags);
  
Same elsewhere. Please update all of these.



src/tests/master_allocator_tests.cpp (line 507)


Line break after "=" is prefered.

Please update other places like this, too.



src/tests/master_allocator_tests.cpp (line 1264)


I like this arrangement of MockExecutor and TestContainerizer a lot better, 
too.



src/tests/master_tests.cpp (line 142)


This indentation is fine, but that one is easier to read:

EXPECT_EQ(slave.get()->pid.address.hostname().get(),
  offers.get()[0].hostname());



src/tests/master_tests.cpp (line 973)


EXPECT_EQ(master.get()->pid.address.ip, 
  net::IP(ntohl(masterInfo.get().ip(;



src/tests/master_tests.cpp (line 1030)


Indentation see above.



src/tests/master_tests.cpp (line 1076)


Insert blank line above, please.



src/tests/master_tests.cpp (line 1110)


It's OK to continue with the first arg on the same line in such cases.

Here and elsewhere.



src/tests/master_tests.cpp (line 1875)


The master needs to be stopped here, not below, right? Otherwise we might 
be missing part of what we want to test here.



src/tests/master_tests.cpp (line 2046)


Same as above. Aren't we stopping the master too late for our purposes?



src/tests/master_tests.cpp (line 3648)


This could go in line 3661.


- Bernd Mathiske


On March 3, 2016, 3:11 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43615/
> ---
> 
> (Updated March 3, 2016, 3:11 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Includes the following changes:
> 
> * Added the `` header where appropriate.
> * Added the namespace `using process::Owned;` where appropriate.
> * Generally replaced `Try` with `Owned`.  And 
> `Try` with `Owned`.
> * Added the (now required) `MasterDetector` argument to all slaves.  Before, 
> this was fetched from the first master in `Cluster`.
> * Removed `Shutdown();` from all tests.
> * Replaced `Stop(...)` with the appropriate master/slave destruction calls.
> * Wrap various slave objects in `Owned` (i.e. containerizers, isolators, 
> launchers, etc).
> * Replace `CHECK` in tests with `ASSERT`.
> 
> 
> Diffs
> -
> 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/command_executor_tests.cpp 
> 0d2fcf6d4b8d9a925eb6748e6bd33cf279b8f7f8 
>   src/tests/container_logger_tests.cpp 
> 00f4129e46aa9268fbb66da25b34e61004fa87b2 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6aecd912fc84b72d2b64f7a842891fddcbc469ac 
>   src/tests/containerizer/external_containerizer_test.cpp 
> 8e1dbe306a088eb16cd3b9c6174b95fad5685da4 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> e72239a55724f1aeeec5362cc370c93dbeca7164 
>   src/tests/containerizer/isolator_tests.cpp 
> 342037ce0a5f8caa4e3cf1550b8f9a7cc328acf9 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 79f134996c4b80bf49cbb8bee28eab5e6b4f5822 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 983a6be160aefe5a32acb6111bb3c85230ec 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 5b685bfd842d0d98e8ea5ec5ddea8d2cd893dd81 
>   src/tests/credentials_tests.cpp 7edcc857e0f6f8e80e265deeec59d6349d392224 
>   src/tests/disk_quota_tests.cpp 413e562026a4fc9779f616e921ae2fa2ca51e012 
>   src/tests/exception_tests.cpp 6b71316d545e97f14a45daa14d0fd95204befd3b 
>   src/tests/executor_http_api_tests.cpp 
> 

Re: Review Request 44269: Added the framework of 'network/cni' isolator.

[Qian Zhang]

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

(Updated March 4, 2016, 10:34 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Changes
---

Implemented NetworkCniIsolatorProcess::create().


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


Repository: mesos


Description
---

Added the framework of 'network/cni' isolator.


Diffs (updated)
-

  src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
  src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
  src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Review Request 44390: No need to enable SSL when using registry puller.

[Guangya Liu]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

No need to enable SSL when using registry puller.


Diffs
-

  docs/mesos-provisioner.md 1b19406cb93bbc3f5330eaf9d29b1be98a674136 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 44372: Upgrade http-parser to 2.6.1 to support Power LE platform.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [44372]

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

Error:
2016-03-04 14:15:58 URL:https://reviews.apache.org/r/44372/diff/raw/ 
[10748/10748] -> "44372.patch" [1]
error: missing binary patch data for 
'3rdparty/libprocess/3rdparty/http-parser-2.6.1.tar.gz'
error: binary patch does not apply to 
'3rdparty/libprocess/3rdparty/http-parser-2.6.1.tar.gz'
error: 3rdparty/libprocess/3rdparty/http-parser-2.6.1.tar.gz: patch does not 
apply

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

- Mesos ReviewBot


On March 4, 2016, 7:24 a.m., Zhiwei Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44372/
> ---
> 
> (Updated March 4, 2016, 7:24 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Alex Clemmer, Kapil Arya, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-4805
> https://issues.apache.org/jira/browse/MESOS-4805
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Upgrade http-parser to 2.6.1 to support Power LE platform.
> 
> 
> Diffs
> -
> 
>   3rdparty/cmake/Versions.cmake 6eac4dc0f7189e209e7d7232419e4de4bc0875c0 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 
> b8351ad0181d885a984580ae8de208ea0524b0e7 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> ddf7e3d9bf76d4a03c33f02d52ec29812aef8509 
>   3rdparty/libprocess/3rdparty/http-parser-2.6.1.tar.gz PRE-CREATION 
>   3rdparty/libprocess/3rdparty/ry-http-parser-1c3624a.patch 
> f9fac12437a6bedc66353fda1ce9c0d7a383225a 
>   3rdparty/libprocess/3rdparty/ry-http-parser-1c3624a.tar.gz 
> b811b63ce0ad6d71d9d296fed76656c023c76fc5 
>   3rdparty/libprocess/3rdparty/versions.am 
> 98195b8eb60b2673d610d8ab7ea31103f137debf 
>   3rdparty/libprocess/Makefile.am ac8cc8d29baccf6e3a17367540ddd1f28585ef6d 
>   3rdparty/libprocess/src/decoder.hpp 
> a20b5ba8fc50d834573d253948645cc863f030dd 
>   LICENSE 66a99b8a84e614cc89a22df02e3c47d01e26cd39 
> 
> Diff: https://reviews.apache.org/r/44372/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhiwei Chen
> 
>

Re: Review Request 44380: Change IOTest.BufferedRead to write to the temporary directory.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44380]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 4, 2016, 7:27 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44380/
> ---
> 
> (Updated March 4, 2016, 7:27 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-4807
> https://issues.apache.org/jira/browse/MESOS-4807
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit changes IOTest.BufferedRead so that tests could be
> executed from temporary directories via TemporaryDirectoryTest
> fixture (MESOS-4807).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/io_tests.cpp 
> 2bffc7cd9c3aa204a1d1b8eb45f0bff12f49ca62 
> 
> Diff: https://reviews.apache.org/r/44380/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 43884: Added allocator metrics for used quotas.

[Alexander Rukletsov]

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




docs/monitoring.md (line 905)


In the same vein as `allocator/allocated/KIND`, this should be something 
like `allocator/quota/ROLE/allocated/KIND`, right?



docs/monitoring.md (line 908)


s/in use/allocated



src/master/allocator/mesos/hierarchical.cpp (lines 1081 - 1092)


Here we will be also adding a gauge for "set" quotas, a candidate for a 
helper in `Metrics`



src/master/allocator/mesos/hierarchical.cpp (lines 1124 - 1132)


Let's do it in `Metrics` as well.



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


Let's be consistent and call it `_quota_allocated`. Same for the local 
variable.



src/master/allocator/mesos/metrics.hpp (lines 74 - 75)


let's call it `quota_allocated`.



src/master/allocator/mesos/metrics.hpp (line 75)


two spaces



src/tests/hierarchical_allocator_tests.cpp (line 2474)


s/famework/framework


- Alexander Rukletsov


On March 3, 2016, 4:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43884/
> ---
> 
> (Updated March 3, 2016, 4:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4723
> https://issues.apache.org/jira/browse/MESOS-4723
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metrics for used quotas.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43884/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 43614: Refactor MesosTest and remove cleanup logic.

[Bernd Mathiske]

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


Fix it, then Ship it!





src/tests/mesos.hpp (line 190)


If we were pedantic, we could make the order of the params the same in the 
comment and the signature.



src/tests/mesos.hpp (line 204)


Param order in comments vs. signature.


- Bernd Mathiske


On March 2, 2016, 1:45 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43614/
> ---
> 
> (Updated March 2, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updates `StartMaster` and `StartSlave` test helpers to use the reworked 
> `cluster` helpers.  Removes all `MesosTest` cleanup logic and as well as the 
> helpers that accept a `MockExecutor` pointer.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 1d4f075e470a60040e17b9f011aea6202310c437 
>   src/tests/mesos.cpp e0f641c6828833de13a0a233e39ff6dc3f343d5c 
> 
> Diff: https://reviews.apache.org/r/43614/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

[Bernd Mathiske]

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


Ship it!




Note to reviewers: the resulting code can still be improved (as could the 
original), but if you focus just on the refactoring, without making additional 
improvements which could be addressed in extra patches, I think we are done 
here.

- Bernd Mathiske


On March 2, 2016, 1:45 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> ---
> 
> (Updated March 2, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem 
> Harutyunyan, and Michael Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope 
> of test objects to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  
> The `Slave` object performs cleanup originally found in 
> `cluster::Slave::stop`.  `cluster::Master::start` and `cluster::Slave::start` 
> were changed to factory methods.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> ---
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

[Bernd Mathiske]

> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.hpp, line 213
> > 
> >
> > I don't think we need this constructor. We are using an instance of 
> > this merely as an aggregate temporary variable for the injections into the 
> > actual instance to be constructed.
> 
> Joseph Wu wrote:
> I'd argue the small constructor is better for readability.
> 
> The alternative would involve adding one temporary variable for each 
> injection into the `cluster::Slave::start` factory.  The private constructor 
> would then look like:
> ```
> class Slave 
> {
> ...
> 
> private:
>   Slave(
>   MasterDetector* _detector,
>   slave::Flags _flags,
>   slave::Containerizer* _containerizer,
>   process::Owned& _ownedContainerizer,
>   process::Owned& _fetcher,
>   process::Owned& _gc,
>   process::Owned& _qosController,
>   process::Owned& _resourceEstimator,
>   process::Owned& _statusUpdateManager)
> : detector(_detector), 
>   flags(_flags),
>   containerizer(_containerizer),
>   ownedContainerizer(_ownedContainerizer),
>   fetcher(_fetcher),
>   gc(_gc),
>   qosController(_qosController),
>   resourceEstimator(_resourceEstimator),
>   statusUpdateManager(_statusUpdateManager)
>   {
> slave = new slave::Slave(
>   id.isSome() ? id.get() : process::ID::generate("slave"),
>   flags,
>   detector,
>   slave->containerizer,
>   >files,
>   gc.getOrElse(slave->gc.get()),
>   statusUpdateManager.getOrElse(slave->statusUpdateManager.get()),
>   resourceEstimator.getOrElse(slave->resourceEstimator.get()),
>   qosController.getOrElse(slave->qosController.get()));
>   }
> }
> ```
> 
> That's a lot of code bloat just to pass some variables around.
> Also, at least one constructor is necessary so that we can enforce the 
> `private`-ness of said constructor.  It wouldn't make sense to have tests 
> construct the `cluster::Slave` in any other way.

The way I see it we need exactly one constructor and that can be the 
non-trivial one. The objective is that all members can be const then. 

I don't mind a large number of temp variables in a local scope.

That said, I would agree with leaving the code as is for now, because it 
simplifies the current patch and does not introduce a second kind of change. 
However, I would recommend an extra patch on top of that for later.


> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 279
> > 
> >
> > We shouldn't really use a temp master object just to reset it with the 
> > same values it already has. I'd prefer a more functional style with only 
> > one constructor. See also the slave below.
> 
> Joseph Wu wrote:
> We aren't resetting the `cluster::Master` object though.  This line is 
> passing objects owned by `cluster::Master` into `master::Master`.
> 
> Would it be more clear if the line read like this?
> ```
>   master->master = new master::Master(...
> ```

Dropping for same reason as above. Refactoring and improving the programming 
style can be dojne in 2 steps instead of one. Let's stick to the refactoring 
for now.


> On March 3, 2016, 5:22 a.m., Bernd Mathiske wrote:
> > src/tests/cluster.cpp, line 339
> > 
> >
> > Before this patch, with a cluster object that holds all masters, it 
> > makes sense to revoke the authenticator for all masters at shutdown. 
> > However, in our tests, each master constructor sets the authenticator in 
> > libprocess the same way, with an equivalent value. And libprocess assumes 
> > ownership, i.e. it will destruct any lingering authenticator eventually. It 
> > will also destruct the previous one if a new one is set. 
> > 
> > Therefore, we don't need to actively unset the authenticator here. In 
> > fact, this prevents multi-master tests. If one master gets destructed, it 
> > takes the authenticator for the others with it. Because there is only one - 
> > in libprocess.
> 
> Joseph Wu wrote:
> There are a couple other problems with multi-master tests 
> (https://issues.apache.org/jira/browse/MESOS-2976).
> 
> Note that the comment right below was retained from here 
> https://reviews.apache.org/r/43614/diff/2#1.97
> ```
> // This means that multiple masters in tests are not supported.
> ```
> 
> ---
> 
> Looks like there are also a few tests that require this.
> On OSX, I removed this line and saw these tests fail:
> ```
> [  FAILED  ] MasterQuotaTest.NoAuthenticationNoAuthorization
>

Re: Review Request 44332: Fixed a typo in the HierarchicalAllocatorTest.CoarseGrained test.

[Alexander Rukletsov]

> On March 3, 2016, 7:04 p.m., Joris Van Remoortere wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 806
> > 
> >
> > Can you fix the extra `.` here as well?

Sure


- Alexander


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


On March 3, 2016, 1 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44332/
> ---
> 
> (Updated March 3, 2016, 1 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44332/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `make check`
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 43883: Added allocator metrics for the number of offer filters per framework.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.cpp (lines 288 - 297)


I suggest we create a helper in `Metrics` for this. Also, minor nit: in 
previous patches you didn't separate `.put()` from `process::metrics::add()` 
with a blank line.



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


I believe this one requires `->self()` as well.



src/master/allocator/mesos/hierarchical.cpp (lines 368 - 369)


Same here.



src/tests/hierarchical_allocator_tests.cpp (line 538)


Why do explicitly use double `0` here? I thought you rely on implicit 
conversions and promotions in these patches.



src/tests/hierarchical_allocator_tests.cpp (lines 2462 - 2466)


Here you can also check that 2 counters for offers filters were installed.


- Alexander Rukletsov


On March 3, 2016, 4:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43883/
> ---
> 
> (Updated March 3, 2016, 4:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4722
> https://issues.apache.org/jira/browse/MESOS-4722
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metrics for the number of offer filters per framework.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43883/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 43882: Added allocation metrics for allocation time.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp (lines 2568 - 2569)


Let's pull it into the test preambular comment.



src/tests/hierarchical_allocator_tests.cpp (lines 2578 - 2579)


s/agent1/agent



src/tests/hierarchical_allocator_tests.cpp (line 2581)


s/least/at least


- Alexander Rukletsov


On March 3, 2016, 4:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43882/
> ---
> 
> (Updated March 3, 2016, 4:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4721
> https://issues.apache.org/jira/browse/MESOS-4721
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocation metrics for allocation time.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43882/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 43881: Added allocator metric for the number of allocations to a framework.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.cpp (lines 275 - 280)


I would pull this into a helper in `Metrics` similar to 
`createGaugesForResource()`. This way we keep the allocator code minimal.



src/master/allocator/mesos/hierarchical.cpp (lines 344 - 347)


Maybe here a helper as well.



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


I think this should be around `allocated()` calls for sorters. When 
`allocated()` is called, the sorter's internal allocation counter is 
incremented. I'd say we should keep the counters in sync.

Alternatively, we can expose the allocations counter from sorters and query 
it directly.



src/master/allocator/mesos/metrics.cpp (lines 59 - 60)


If you say `using process::metrics::Counter` at the top, you can fit this 
into one line : ).



src/tests/hierarchical_allocator_tests.cpp (line 2464)


Backtick `framework1` please.



src/tests/hierarchical_allocator_tests.cpp (lines 2464 - 2472)


Do you want to set quota as part of your test? Because otherwise we can 
simplify the test a bit by not setting it and having `agent2` allocated to 
`framework2`.



src/tests/hierarchical_allocator_tests.cpp (line 2487)


Feel free to kill this line



src/tests/hierarchical_allocator_tests.cpp (lines 2492 - 2495)


How about 
```
  Clock::advance(flags.allocation_interval);
  Clock::settle();
  ++allocations;
```
to keep clock manipulation together?


- Alexander Rukletsov


On March 3, 2016, 4:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> ---
> 
> (Updated March 3, 2016, 4:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metric for the number of allocations to a framework.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43881/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 43880: Added allocator metrics for total and allocated scalar resources.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/master/allocator/mesos/metrics.hpp (line 27)


Kill this line.



src/master/allocator/mesos/metrics.cpp (lines 61 - 63)


Does it make sense to store the pointer to `HierarchicalAllocatorProcess` 
in the `Metrics` instance and remove the first parameter here? The value of 
this approach will be more evident when we will have more helpers later.



src/tests/hierarchical_allocator_tests.cpp (line 2406)


s/users/frameworks



src/tests/hierarchical_allocator_tests.cpp (lines 2420 - 2424)


Can you move it up where you check allocated/* metrics? Adding a framework 
shouldn't influence this counter.



src/tests/hierarchical_allocator_tests.cpp (lines 2449 - 2453)


I think resources will be offered to `framework2`. But is it actually 
important? I think you can kill this comment altogether (also the similar one 
above), and add a short one around `settle()`.



src/tests/hierarchical_allocator_tests.cpp (line 2463)


Feel free to kill this line.



src/tests/hierarchical_allocator_tests.cpp (line 2491)


/cluster/cluster state/


- Alexander Rukletsov


On March 3, 2016, 4:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated March 3, 2016, 4:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4720
> https://issues.apache.org/jira/browse/MESOS-4720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metrics for total and allocated scalar resources.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43880/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 44366: Added GPUs as an explicit resource.

[Klaus Ma]

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




src/slave/containerizer/containerizer.cpp (line 105)


`` for --nvidia_gpus; not only this line, please also update them in the 
following comments.



src/slave/containerizer/containerizer.cpp (line 115)


I'd like to say "`--navidia_gpus` must be set when specifying `gpu` 
resources."


- Klaus Ma


On March 4, 2016, 9:11 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 4, 2016, 9:11 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Rob Todd.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently we can either infer the number of GPUs to offer as resources
> from the --nvidia_gpus flag, or pass the amount explicity as a "gpus"
> resource. In the future, we will generalize this via autodiscovery of
> gpus and support for gpus types other than Nvidia.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 
>   include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e 
>   src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 
>   src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
>   src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d 
> 
> Diff: https://reviews.apache.org/r/44366/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44336: Removed numeric suffixes where appropriate in allocator tests.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On March 3, 2016, 5:01 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44336/
> ---
> 
> (Updated March 3, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We should not enumerate variables if the corresponding instance is the
> only one of such type in the test.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44336/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> 
> `make check`
> 
> `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44379: Correctly parse perf stat format for 3.10 kernel.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

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

- Mesos ReviewBot


On March 4, 2016, 6:28 a.m., fan du wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44379/
> ---
> 
> (Updated March 4, 2016, 6:28 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-4705
> https://issues.apache.org/jira/browse/MESOS-4705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Correctly parse perf stat format for 3.10 kernel.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
> 
> Diff: https://reviews.apache.org/r/44379/diff/
> 
> 
> Testing
> ---
> 
> {Found and Test} with Serenity, ema filter could get perf event statistics 
> correctly as expected.
> 
> 
> Thanks,
> 
> fan du
> 
>

Re: Review Request 44335: Moved variable declarations closer to where they are used.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On March 3, 2016, 5:01 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44335/
> ---
> 
> (Updated March 3, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44335/diff/
> 
> 
> Testing
> ---
> 
> Tested as a chain in https://reviews.apache.org/r/44336/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44334: Cleaned up empty hashmaps from allocator tests.

[Bernd Mathiske]

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



"{}" is short, but cryptic. It is unclear what kind of entity is being passed 
here. "EMPTY" was not any better. "hashmap()" at least 
revealed the type which hinted a little at the presumable purpose. A better 
identifier than "EMPTY" would beat all of the above. Maybe "NO_USED_RESOURCES"?

Could the parameter of the methods in question be supplied with a default 
value? Then no corresponding argument at all would appear in these tests.

- Bernd Mathiske


On March 3, 2016, 5 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44334/
> ---
> 
> (Updated March 3, 2016, 5 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/44334/diff/
> 
> 
> Testing
> ---
> 
> Tested as a chain in https://reviews.apache.org/r/44336/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44378: Upgrade libev to 4.22 to support PowerPC LE platform.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [44378]

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

Error:
2016-03-04 11:32:40 URL:https://reviews.apache.org/r/44378/diff/raw/ 
[5710/5710] -> "44378.patch" [1]
error: missing binary patch data for 
'3rdparty/libprocess/3rdparty/libev-4.22.tar.gz'
error: binary patch does not apply to 
'3rdparty/libprocess/3rdparty/libev-4.22.tar.gz'
error: 3rdparty/libprocess/3rdparty/libev-4.22.tar.gz: patch does not apply

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

- Mesos ReviewBot


On March 4, 2016, 6:16 a.m., Zhiwei Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44378/
> ---
> 
> (Updated March 4, 2016, 6:16 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Alex Clemmer, Kapil Arya, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-4803
> https://issues.apache.org/jira/browse/MESOS-4803
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Upgrade libev to 4.22 to support PowerPC LE platform.
> 
> 
> Diffs
> -
> 
>   3rdparty/cmake/Versions.cmake 6eac4dc0f7189e209e7d7232419e4de4bc0875c0 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 
> b8351ad0181d885a984580ae8de208ea0524b0e7 
>   3rdparty/libprocess/3rdparty/libev-4.15.patch 
> bbd83e6928e6caba3bc5a9739823d60923cfaa2a 
>   3rdparty/libprocess/3rdparty/libev-4.15.tar.gz 
> 4c282b573aa9331fd16197ef286faf323b6515eb 
>   3rdparty/libprocess/3rdparty/libev-4.22.patch PRE-CREATION 
>   3rdparty/libprocess/3rdparty/libev-4.22.tar.gz PRE-CREATION 
>   3rdparty/libprocess/3rdparty/versions.am 
> 98195b8eb60b2673d610d8ab7ea31103f137debf 
>   LICENSE 66a99b8a84e614cc89a22df02e3c47d01e26cd39 
>   src/python/native/ext_modules.py.in 
> eb93864733713dddad66141c6b8b6cd895f41484 
> 
> Diff: https://reviews.apache.org/r/44378/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhiwei Chen
> 
>

Re: Review Request 43881: Added allocator metric for the number of allocations to a framework.

[Alexander Rukletsov]

> On March 1, 2016, 11:48 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 268-272
> > 
> >
> > Does it make sense to wrap framework-related metrics in a separate 
> > struct in metrics and hide `process::metrics::add` there? You can look here 
> > for inspiration: 
> > https://github.com/apache/mesos/blob/0fd95ccc54e4d144c3eb60e98bf77d53b6bdab63/src/master/metrics.hpp#L81
> 
> Benjamin Bannier wrote:
> That's something I also thought about. This might be something useful 
> more generally, so I suggest to look at this independently of this series and 
> fix everywhere.
> 
> What I find slightly suboptimal about the wrapper you linked to is that 
> it would `add` and `remove` the same metric even when the wrapper is just 
> copied (the `MetricProcess` uses just the name of the `Metric` to check 
> identity). The calls `add` and `remove` return `Future`s to indicate 
> success or failure but we cannot propagate that information in an RAII way in 
> mesos (no exceptions). I think one would either need a type w/o copy ctr and 
> a specific move ctr, or a more complicated infrastructure of e.g., handlers 
> and shared metrics in the back.

Good point. I think we can leave it as is for now and follow up later. Have you 
synced with BenM about it?


- Alexander


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


On March 3, 2016, 4:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43881/
> ---
> 
> (Updated March 3, 2016, 4:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4719
> https://issues.apache.org/jira/browse/MESOS-4719
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metric for the number of allocations to a framework.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43881/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 44343: Used single space in license check error message.

[Till Toenshoff]

> On March 3, 2016, 8:12 p.m., Till Toenshoff wrote:
> > support/mesos-style.py, line 90
> > 
> >
> > Are the two leading spaces intentional?
> 
> Benjamin Bannier wrote:
> Yes, this is intentional to be consistent with `cpplint`'s output format, 
> see https://github.com/apache/mesos/blob/master/support/cpplint.py#L1012 (we 
> use the default output format which is `emacs`).

Thanks for clearifying - committing this now.


- Till


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


On March 3, 2016, 5:06 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44343/
> ---
> 
> (Updated March 3, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used single space in license check error message.
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 815d88c1935ae8248cd76a73d3dd613312b1d730 
> 
> Diff: https://reviews.apache.org/r/44343/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 43880: Added allocator metrics for total and allocated scalar resources.

[Alexander Rukletsov]

> On March 3, 2016, 3:04 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1711-1712
> > 
> >
> > My intuition is that using `allocationScalarQuantities` will be more 
> > efficient. However, we should expose known clients from the sorter. What do 
> > you think?
> 
> Benjamin Bannier wrote:
> As for efficiency, I don't think there's an issue here; we just create a 
> `Value::Scalar` which I think cannot be avoided, and e.g., never create 
> temporary `Resources`. Would we use `allocationScalarQuantities`, we'd always 
> be forced to create temporary `Resources` (which in an ideal world could be 
> optimized away again).

I was thinking that iterating over active roles should be cheaper than over all 
agents. But it's fine to leave it as is and come back if we observe some 
performance issues.


- Alexander


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


On March 3, 2016, 4:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43880/
> ---
> 
> (Updated March 3, 2016, 4:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4720
> https://issues.apache.org/jira/browse/MESOS-4720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator metrics for total and allocated scalar resources.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3043888630b066505410d3b32c5b3f813cc458c1 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
>   src/master/allocator/mesos/metrics.hpp PRE-CREATION 
>   src/master/allocator/mesos/metrics.cpp PRE-CREATION 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43880/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> I confirmed that this does not lead to general performance regressions in the 
> allocator; this is partially expected since the added code only inserts 
> metrics in the allocator while the actual work is perform asynchronously. 
> These tests where performed with 
> `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build 
> under OS X using clang(trunk) as compiler.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>