Re: Review Request 68637: Avoided common-case extra copying of Resources in Master::offer.

[Meng Zhu]

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




src/master/master.cpp
Line 9379 (original), 9378 (patched)


`DeleteSubrange` will affect all the elements after `i`, this may cause 
quite some overhead.

We can swap `ephemeral_ports` resources to the tail and `DeleteSubrange` 
them in one go.

I do not know how often ephemeral ports are used, but the above approach 
seems to be worthwhile and would not hurt readability much.


- Meng Zhu


On Sept. 5, 2018, 3:15 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68637/
> ---
> 
> (Updated Sept. 5, 2018, 3:15 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's more typical for there not to be ephemeral ports resources,
> therefore we avoid copying in the common case.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e307df001fba3afbd128c3c0d9db955d6777186a 
> 
> 
> Diff: https://reviews.apache.org/r/68637/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68646: Updated launchers to use subprocess's `whitelist_fds` parameter.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68646 was successfully built and tested.

Reviews applied: `['68642', '68643', '68644', '68645', '68646']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2301/mesos-review-68646

- Mesos Reviewbot Windows


On Sept. 6, 2018, 1:25 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68646/
> ---
> 
> (Updated Sept. 6, 2018, 1:25 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and James Peach.
> 
> 
> Bugs: MESOS-9152
> https://issues.apache.org/jira/browse/MESOS-9152
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated launchers to use subprocess's `whitelist_fds` parameter.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launcher.cpp 
> a18bdff6b64f978b87af16c2655d07ba8553bc95 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> e9ab36a16b24a123a660506d55f16dea16dee911 
> 
> 
> Diff: https://reviews.apache.org/r/68646/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 68639: Renamed `Resources::resources` to `noMutationWithoutExclusiveOwnership`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68639 was successfully built and tested.

Reviews applied: `['68490', '68639']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2300/mesos-review-68639

- Mesos Reviewbot Windows


On Sept. 6, 2018, 12:13 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68639/
> ---
> 
> (Updated Sept. 6, 2018, 12:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón 
> Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to the copy-on-write optimization (MESOS-6765), one needs to
> check the `use_count` of `Resource_` before mutating. Currently,
> there is no mechanism to enforce this. As a short-term mitigation
> measure, we rename `resources` to `noMutationWithoutExclusiveOwnership`
> to alert people about obtaining an exclusive ownership before mutating
> the `Resource_` objects.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
>   include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
>   src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
>   src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 
> 
> 
> Diff: https://reviews.apache.org/r/68639/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 68645: Updated IO switchboard to use subprocess's `whitelist_fds` parameter.

[Qian Zhang]

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

Review request for mesos, Gilbert Song and James Peach.


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


Repository: mesos


Description
---

Updated IO switchboard to use subprocess's `whitelist_fds` parameter.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
52b0e521ed1c651c90b3a3df7c4df576288bf400 


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


Testing
---


Thanks,

Qian Zhang

Review Request 68642: Added `lsof()` into stout.

[Qian Zhang]

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

Review request for mesos, Gilbert Song and James Peach.


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


Repository: mesos


Description
---

Added `lsof()` into stout.


Diffs
-

  3rdparty/stout/include/Makefile.am a9c61fcba253a811494cdcdb0afb3d3a018f4585 
  3rdparty/stout/include/stout/os.hpp aee041891b7e7ff93a0b1ac31019a7a3d4eae962 
  3rdparty/stout/include/stout/os/lsof.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/posix/lsof.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/lsof.hpp PRE-CREATION 


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


Testing
---


Thanks,

Qian Zhang

Review Request 68646: Updated launchers to use subprocess's `whitelist_fds` parameter.

[Qian Zhang]

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

Review request for mesos, Gilbert Song and James Peach.


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


Repository: mesos


Description
---

Updated launchers to use subprocess's `whitelist_fds` parameter.


Diffs
-

  src/slave/containerizer/mesos/launcher.cpp 
a18bdff6b64f978b87af16c2655d07ba8553bc95 
  src/slave/containerizer/mesos/linux_launcher.cpp 
e9ab36a16b24a123a660506d55f16dea16dee911 


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


Testing
---

sudo make check


Thanks,

Qian Zhang

Review Request 68644: Closed all file descriptors except `whitelist_fds` in posix/subprocess.

[Qian Zhang]

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

Review request for mesos, Gilbert Song and James Peach.


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


Repository: mesos


Description
---

Closed all file descriptors except `whitelist_fds` in posix/subprocess.


Diffs
-

  3rdparty/libprocess/src/posix/subprocess.hpp 
007058b61fdcd4716aa793516c842c3cef8c0a29 
  3rdparty/libprocess/src/subprocess.cpp 
c0640de2dc4278b884282dfaad98c49c3b067a5b 


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


Testing
---


Thanks,

Qian Zhang

Review Request 68643: Updated `MesosContainerizerLaunch` to call `os::lsof()`.

[Qian Zhang]

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

Review request for mesos, Gilbert Song and James Peach.


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


Repository: mesos


Description
---

Updated `MesosContainerizerLaunch` to call `os::lsof()`.


Diffs
-

  src/slave/containerizer/mesos/launch.cpp 
7193da0a094df3e441e185c62b3a0379a0bdc4a2 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 68641: Added version check and bundling of libevent within libprocess.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 68640.

Failed command: `python.exe .\support\apply-reviews.py -n -r 68640`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2299/mesos-review-68641

Relevant logs:

- 
[apply-review-68640.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2299/mesos-review-68641/logs/apply-review-68640.log):

```
68640.patch:77: trailing whitespace.
 
68640.patch:90: space before tab in indent.
return 1;
68640.patch:92: trailing whitespace.
 
68640.patch:95: space before tab in indent.
if (!b)
68640.patch:96: space before tab in indent.
return 0;
error: missing binary patch data for '3rdparty/libevent-2.0.22-stable.tar.gz'
error: binary patch does not apply to '3rdparty/libevent-2.0.22-stable.tar.gz'
error: 3rdparty/libevent-2.0.22-stable.tar.gz: patch does not apply
```

- Mesos Reviewbot Windows


On Sept. 6, 2018, 8:32 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68641/
> ---
> 
> (Updated Sept. 6, 2018, 8:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.
> 
> 
> Bugs: MESOS-7076
> https://issues.apache.org/jira/browse/MESOS-7076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added version check and bundling of libevent within libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am f9d9d0609f5c8fed419be83319319e67afc063b7 
>   3rdparty/libprocess/configure.ac e9e2434b7b6fe6c94c28744a86f9412b6302cbe0 
> 
> 
> Diff: https://reviews.apache.org/r/68641/diff/1/
> 
> 
> Testing
> ---
> 
> Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. Additional testing 
> is ongoing. CMake updates will follow.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 68637: Avoided common-case extra copying of Resources in Master::offer.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68637 was successfully built and tested.

Reviews applied: `['68634', '68635', '68636', '68637']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2298/mesos-review-68637

- Mesos Reviewbot Windows


On Sept. 6, 2018, 6:15 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68637/
> ---
> 
> (Updated Sept. 6, 2018, 6:15 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's more typical for there not to be ephemeral ports resources,
> therefore we avoid copying in the common case.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e307df001fba3afbd128c3c0d9db955d6777186a 
> 
> 
> Diff: https://reviews.apache.org/r/68637/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68636: Reserved vector capacities within Master::offer.

[Benjamin Mahler]

> On Sept. 6, 2018, 12:41 a.m., Meng Zhu wrote:
> > src/master/master.cpp
> > Lines 9249 (patched)
> > 
> >
> > can you put the type here instead of `auto`?

Unfortunately templated types don't work inside `foreach(|key|value)`, I would 
need a typedef.


- Benjamin


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


On Sept. 5, 2018, 10:15 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68636/
> ---
> 
> (Updated Sept. 5, 2018, 10:15 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e307df001fba3afbd128c3c0d9db955d6777186a 
> 
> 
> Diff: https://reviews.apache.org/r/68636/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68636: Reserved vector capacities within Master::offer.

[Meng Zhu]

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


Ship it!





src/master/master.cpp
Lines 9249 (patched)


can you put the type here instead of `auto`?


- Meng Zhu


On Sept. 5, 2018, 3:15 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68636/
> ---
> 
> (Updated Sept. 5, 2018, 3:15 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e307df001fba3afbd128c3c0d9db955d6777186a 
> 
> 
> Diff: https://reviews.apache.org/r/68636/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 68640: Added version check and bundling of libevent to autotools.

[Till Toenshoff via Review Board]

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

Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.


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


Repository: mesos


Description
---

Bundles libevent 2.0.22 to ensure functional SSL builds accross all
supported platforms. Namely macOS and Ubuntu have shown problems when
using more recent libevent versions. The underlying problem has been
under investigation for longer period of time - so far without a
solution. The bundled libevent includes a patch to make it libssl
version > 1.0.x compatible. That patch has been extracted from the
Fedora source package libevent-2.0.22-6.fc27. The resulting libevent
builds against both, libssl 1.0.x as well as libssl 1.1.x.
For unbundled builds a version detection of known problematic
distributions vs. provided libevent is included.


Diffs
-

  3rdparty/Makefile.am a6709ff9c7ef81896174ed4d35e1b1da9bb33757 
  3rdparty/libevent-2.0.22-stable.patch PRE-CREATION 
  3rdparty/libevent-2.0.22-stable.tar.gz PRE-CREATION 
  3rdparty/versions.am 8fd8bd4d7482af5304ab0b56270e49f447e0b3f5 
  configure.ac 9de51e59796e2a64138acb82cbbb080f1ae6cc6e 
  src/python/native_common/ext_modules.py.in 
7efbb8ec91b74c32ac629f1f2e7de82986d518db 


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


Testing
---

Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. Additional testing is 
ongoing. CMake updates will follow.


Thanks,

Till Toenshoff

Review Request 68641: Added version check and bundling of libevent within libprocess.

[Till Toenshoff via Review Board]

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

Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and James Peach.


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


Repository: mesos


Description
---

Added version check and bundling of libevent within libprocess.


Diffs
-

  3rdparty/libprocess/Makefile.am f9d9d0609f5c8fed419be83319319e67afc063b7 
  3rdparty/libprocess/configure.ac e9e2434b7b6fe6c94c28744a86f9412b6302cbe0 


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


Testing
---

Manual testing on macOS 10.14 as well as Ubuntu 18.04LTS. Additional testing is 
ongoing. CMake updates will follow.


Thanks,

Till Toenshoff

Re: Review Request 68635: Avoided triple-lookup of the framework in Master::offer.

[Meng Zhu]

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


Ship it!




Ship It!

- Meng Zhu


On Sept. 5, 2018, 3:15 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68635/
> ---
> 
> (Updated Sept. 5, 2018, 3:15 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e307df001fba3afbd128c3c0d9db955d6777186a 
> 
> 
> Diff: https://reviews.apache.org/r/68635/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 68639: Renamed `Resources::resources` to `noMutationWithoutExclusiveOwnership`.

[Meng Zhu]

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

Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón Kleiman.


Repository: mesos


Description
---

Due to the copy-on-write optimization (MESOS-6765), one needs to
check the `use_count` of `Resource_` before mutating. Currently,
there is no mechanism to enforce this. As a short-term mitigation
measure, we rename `resources` to `noMutationWithoutExclusiveOwnership`
to alert people about obtaining an exclusive ownership before mutating
the `Resource_` objects.


Diffs
-

  include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
  include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
  src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
  src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 68404: Updated XFS recovery tests with persistent volumes.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68404 was successfully built and tested.

Reviews applied: `['68398', '68399', '68400', '68401', '68402', '68403', 
'68404']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2297/mesos-review-68404

- Mesos Reviewbot Windows


On Aug. 16, 2018, 11:52 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68404/
> ---
> 
> (Updated Aug. 16, 2018, 11:52 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5158
> https://issues.apache.org/jira/browse/MESOS-5158
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the `disk/xfs` recovery tests to ensure that projects IDs on
> persistent volumes are correctly accounted for across agent restarts.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> 59ec182c1c3af3978156044f03d9e3d784d51fce 
> 
> 
> Diff: https://reviews.apache.org/r/68404/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 28)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 68634: Avoided double-lookup in Master::getFramework.

[Meng Zhu]

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


Ship it!




Ship It!

- Meng Zhu


On Sept. 5, 2018, 3:15 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68634/
> ---
> 
> (Updated Sept. 5, 2018, 3:15 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e307df001fba3afbd128c3c0d9db955d6777186a 
> 
> 
> Diff: https://reviews.apache.org/r/68634/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 68634: Avoided double-lookup in Master::getFramework.

[Benjamin Mahler]

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

Review request for mesos, Gastón Kleiman and Meng Zhu.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/master.cpp e307df001fba3afbd128c3c0d9db955d6777186a 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 68636: Reserved vector capacities within Master::offer.

[Benjamin Mahler]

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

Review request for mesos, Gastón Kleiman and Meng Zhu.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/master.cpp e307df001fba3afbd128c3c0d9db955d6777186a 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 68637: Avoided common-case extra copying of Resources in Master::offer.

[Benjamin Mahler]

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

Review request for mesos, Gastón Kleiman and Meng Zhu.


Repository: mesos


Description
---

It's more typical for there not to be ephemeral ports resources,
therefore we avoid copying in the common case.


Diffs
-

  src/master/master.cpp e307df001fba3afbd128c3c0d9db955d6777186a 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 68635: Avoided triple-lookup of the framework in Master::offer.

[Benjamin Mahler]

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

Review request for mesos, Gastón Kleiman and Meng Zhu.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/master.cpp e307df001fba3afbd128c3c0d9db955d6777186a 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 68400: Exposed an XFS helper to find the block device for a given path.

[James Peach]

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

(Updated Sept. 5, 2018, 9:59 p.m.)


Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and 
Jiang Yan Xu.


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


Repository: mesos


Description
---

To manage persistent volumes in the `disk/xfs` isolator we need
to keep track of the block device that hosts a given filesystem
path. Expose the `getDeviceForPath` helper API to directly return
this information.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
e269eb5489ceed8937775a30b3420f7960ab4cd4 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
b691c76e94f686e1a866380dca99ef0fa18e2f49 
  src/tests/containerizer/xfs_quota_tests.cpp 
59ec182c1c3af3978156044f03d9e3d784d51fce 


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

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


Testing
---

sudo make check (Fedora 28)


Thanks,

James Peach

Re: Review Request 68398: Added `fs::used` helper API to stout.

[James Peach]

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

(Updated Sept. 5, 2018, 9:58 p.m.)


Review request for mesos, Chun-Hung Hsiao, Ilya Pronin, Jie Yu, Joseph Wu, and 
Jiang Yan Xu.


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


Repository: mesos


Description (updated)
---

Added a `fs::used` API that returns the amount of used space
on a given filesystem.


Diffs (updated)
-

  3rdparty/stout/include/stout/posix/fs.hpp 
269a4f50f1df8d68be9e11030f885cf2c254c9d8 
  3rdparty/stout/tests/os/filesystem_tests.cpp 
880b551b65eceff9492dd414157694b986608115 


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

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


Testing
---

sudo make check (Fedora 28)


Thanks,

James Peach

Re: Review Request 68490: Optimized `class Resources` with copy-on-write.

[Meng Zhu]

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

(Updated Sept. 5, 2018, 2:47 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón Kleiman.


Changes
---

Added benchmark results.


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


Repository: mesos


Description
---

This patch lets `class Resources` only store shared
pointers to the underlying resource objects, so that
read-only filter operations such as `reserved()`,
`unreserved()` and etc. can avoid making copies of
the whole resource objects. Instead, only shared pointers
are copied.

In write operations, we check if there are more than one
references to the resource object. If so, a copy is made
for safe mutation without affecting owners.

To maintain the usage abstraction that `class Resources`
still holds resource objects, we utilize
`boost::indirect_iterator` iterator adapter to deference
the shared pointers as we iterate.


Diffs
-

  include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
  include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
  src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
  src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 


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


Testing (updated)
---

make check

Benchmarked on a Ubuntu box using clang -O2 optimized build with an i5 
processor, frequency fixed at 1.6GHz.

Resources benchmarks (Measure geo-mean of various operations):

Normalized Geomean latency

Master  Copy-on-write
Arithmetic  1   1.09
Filter  1   0.28
Contains1   0.27
Parse   1   1.01

Sample allocator benchmark (Measure first allocation latency):

HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1 (1k agents, 50 frameworks):

Master 1
Copy-on-write 0.72

HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1 (1k agents, 50 
frameworks):

Master 1
Copy-on-write 0.72


Thanks,

Meng Zhu

Re: Review Request 68490: Optimized `class Resources` with copy-on-write.

[Meng Zhu]

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

(Updated Sept. 5, 2018, 2:30 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón Kleiman.


Changes
---

Made foreach loops inside `class Resources` iterate over 
`shared_ptr` consistently.


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


Repository: mesos


Description
---

This patch lets `class Resources` only store shared
pointers to the underlying resource objects, so that
read-only filter operations such as `reserved()`,
`unreserved()` and etc. can avoid making copies of
the whole resource objects. Instead, only shared pointers
are copied.

In write operations, we check if there are more than one
references to the resource object. If so, a copy is made
for safe mutation without affecting owners.

To maintain the usage abstraction that `class Resources`
still holds resource objects, we utilize
`boost::indirect_iterator` iterator adapter to deference
the shared pointers as we iterate.


Diffs (updated)
-

  include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
  include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
  src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
  src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 


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

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


Testing
---

make check

Did a quick test on Mac with an optimized build, running benchmark 
`HierarchicalAllocator_BENCHMARK_Test.ResourceLabels`, here are the results of 
comparing "before" and "after". Note, DVFS is not fixed. And we only did a 
partial run to verify the validity of the patch, full evaluation coming soon.

**Overall, 33% performance improvement for the 1st steup (1000 agents and 1 
frameworks) and 32% improvement for the first 50 iterarions of the 2nd setup 
(1000 agents and 50 frameworks).**

Before:

[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/0

Using 1000 agents and 1 frameworks
Added 1 frameworks in 1.220022ms
Added 1000 agents in 465.045382ms
round 0 allocate() took 54.803773ms to make 0 offers after filtering 1000 offers
round 1 allocate() took 66.690456ms to make 0 offers after filtering 1000 offers

Using 1000 agents and 50 frameworks
Added 50 frameworks in 1.40774ms
Added 1000 agents in 460.562488ms
round 0 allocate() took 155.500548ms to make 1000 offers after filtering 1000 
offers
round 1 allocate() took 155.194986ms to make 1000 offers after filtering 2000 
offers
round 2 allocate() took 160.706712ms to make 1000 offers after filtering 3000 
offers
round 3 allocate() took 156.701883ms to make 1000 offers after filtering 4000 
offers
round 4 allocate() took 164.246987ms to make 1000 offers after filtering 5000 
offers
round 5 allocate() took 160.791809ms to make 1000 offers after filtering 6000 
offers
round 6 allocate() took 166.064652ms to make 1000 offers after filtering 7000 
offers
round 7 allocate() took 162.177213ms to make 1000 offers after filtering 8000 
offers
round 8 allocate() took 166.620163ms to make 1000 offers after filtering 9000 
offers
round 9 allocate() took 167.970045ms to make 1000 offers after filtering 1 
offers
round 10 allocate() took 166.189463ms to make 1000 offers after filtering 11000 
offers
round 11 allocate() took 170.863462ms to make 1000 offers after filtering 12000 
offers
round 12 allocate() took 172.260473ms to make 1000 offers after filtering 13000 
offers
round 13 allocate() took 170.553911ms to make 1000 offers after filtering 14000 
offers
round 14 allocate() took 235.764593ms to make 1000 offers after filtering 15000 
offers
round 15 allocate() took 247.250433ms to make 1000 offers after filtering 16000 
offers
round 16 allocate() took 276.932824ms to make 1000 offers after filtering 17000 
offers
round 17 allocate() took 248.888469ms to make 1000 offers after filtering 18000 
offers
round 18 allocate() took 193.890556ms to make 1000 offers after filtering 19000 
offers
round 19 allocate() took 195.105346ms to make 1000 offers after filtering 2 
offers
round 20 allocate() took 194.447428ms to make 1000 offers after filtering 21000 
offers
round 21 allocate() took 205.486287ms to make 1000 offers after filtering 22000 
offers
round 22 allocate() took 199.241922ms to make 1000 offers after filtering 23000 
offers
round 23 allocate() took 200.885488ms to make 1000 offers after filtering 24000 
offers
round 24 allocate() took 216.132361ms to make 1000 offers after filtering 25000 
offers
round 25 allocate() took 210.638273ms to make 1000 offers after filtering 26000 
offers
round 26 allocate() took 232.397778ms to make 1000 offers after filtering 27000 
offers
round 27 allocate() took 239.633708ms to make 1000 

Re: Review Request 65112: Added documentation about standalone containers.

[Greg Mann]

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




docs/standalone-containers.md
Lines 140-142 (patched)


Could we also include response information for the happy case of KILL and 
REMOVE calls? Currently the doc only tells users what to expect in case of 
failure.


- Greg Mann


On Jan. 12, 2018, 1:51 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65112/
> ---
> 
> (Updated Jan. 12, 2018, 1:51 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This outlines some of the differences to expect from this
> new type of container and shows some example API calls.
> 
> 
> Diffs
> -
> 
>   docs/csi.md ace5bd4c4293747fffe9bc8dbcceaf34ce035f2f 
>   docs/home.md 4c1fb3738bc9cbb45e0bca65e0f5defaf8c33c0a 
>   docs/standalone-containers.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65112/diff/1/
> 
> 
> Testing
> ---
> 
> Previewed the docs via the website generator.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 68426: Refactored some cgroups helpers to do verify from callers.

[Benjamin Mahler]

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



For posterity, numbers with --enabled-optimize:

```
Before Fix:
Prepared mount 'Took 55.044348852secs to launch 1000 containers
Took 52.265658534secs to destroy 1000 containers

After Fix:
Took 30.683818359secs to launch 1000 containers
Took 22.182255592secs to destroy 1000 containers
```

- Benjamin Mahler


On Aug. 20, 2018, 4:46 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68426/
> ---
> 
> (Updated Aug. 20, 2018, 4:46 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gilbert Song, and James Peach.
> 
> 
> Bugs: MESOS-8418 and MESOS-9081
> https://issues.apache.org/jira/browse/MESOS-8418
> https://issues.apache.org/jira/browse/MESOS-9081
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, some of the cgroups helpers call `cgroups::verify`
> to make sure the hierarchy and cgroup are valid. This causes some
> performance issues for the agent because `cgroups::verify` will read the
> entire mount table, and mount table read is expensive if the mount table
> is large. See more details in MESOS-8418.
> 
> In most of the cases, the verification has already been done when those
> cgroups helpers are called. Therefore, it is better to let the caller do
> the verification and make those cgroups helpers pure helpers.
> 
> Also, the verification is subject to race anyway. The cgroups global
> state might change (e.g., operators modify the cgroups) after the
> verification is done, making the verification itself not that super
> useful.
> 
> This patch refactored the cgroups helpers and moved the verifications to
> the callers.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 282b8e783e8c70742e4388a19003382bc57db8d5 
>   src/linux/cgroups.cpp 86b0306eee804732a55041d2d82c14db35ec5095 
>   src/linux/systemd.cpp 2444ff4e432e61071c62e8e1ee94d62ac6b96c88 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 1444c0543540cf61e253579e9e49d8687bd4ed3a 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
> 321671f18b72edec1f8ed00f0069b539a698c4af 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
> d84f78c99eb118eba1a5bc3e74a2cf57556a325a 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> e51352af12743cc35f09b9fbb7d770f0108f908c 
>   src/slave/main.cpp 489e87522588be259d382f588b66907ba29f1788 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> b80e40b6f4b18e076ebb3c1668f475286c337127 
> 
> 
> Diff: https://reviews.apache.org/r/68426/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> I also used the benchmark I developed in https://reviews.apache.org/r/68266/ 
> to see the speedup
> 
> Before the patch
> Took 56.xxx secs to launch 1000 containers
> Took 55.xxx secs to destroy 1000 containers
> 
> After the patch
> Took 37.xxx secs to launch 1000 containers
> Took 26.xxx secs to destroy 1000 containers
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 68555: Made checker library retry to remove the previous check container.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Aug. 29, 2018, 12:22 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68555/
> ---
> 
> (Updated Aug. 29, 2018, 12:22 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Alexander Rukletsov, Gastón Kleiman, 
> and Gilbert Song.
> 
> 
> Bugs: MESOS-8568
> https://issues.apache.org/jira/browse/MESOS-8568
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously when checker library fails to remove the previous check
> container, it will discard the promise and launch a new check container
> which will cause two problems:
>   1. The discarded promise is used to launch the new check container,
>  that means even the new check container is launched successfully,
>  we still have no chance to process its check result since the
>  promise has already been discarded.
>   2. The previous check container will never get a chance to be removed
>  which is leak, i.e., its runtime directory and sandbox directory
>  will not be removed.
> 
> Now in this patch, when checker library fails to remove the previous
> check container, we make it remove the previous check container again.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 77a76f465fe57eab89f027b5acb74c2339551678 
> 
> 
> Diff: https://reviews.apache.org/r/68555/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 68495: Made command check always waits before removing the nested container.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Sept. 3, 2018, 6:53 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68495/
> ---
> 
> (Updated Sept. 3, 2018, 6:53 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Alexander Rukletsov, Gastón Kleiman, 
> and Gilbert Song.
> 
> 
> Bugs: MESOS-8568
> https://issues.apache.org/jira/browse/MESOS-8568
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made command check always waits before removing the nested container.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp 77a76f465fe57eab89f027b5acb74c2339551678 
> 
> 
> Diff: https://reviews.apache.org/r/68495/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 68553: Set 'Connection: close' in the master's streaming API responses.

[Benjamin Mahler]

> On Sept. 5, 2018, 3:11 a.m., Gastón Kleiman wrote:
> > We should do the same in the executor API handler.

Yeah, originally I did that in this patch, but it ended up being more 
complicated than I anticipated. In any case, I commented in MESOS-9189 that I 
would leave the ticket open (although, probably I need to pull out another 
ticket for the sake of the CHANGELOG and backporting).


- Benjamin


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


On Aug. 29, 2018, 2:03 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68553/
> ---
> 
> (Updated Aug. 29, 2018, 2:03 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-9189
> https://issues.apache.org/jira/browse/MESOS-9189
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We've seen some HTTP intermediaries (e.g. ELB) decide to re-use
> connections to mesos as an optimization to avoid re-connection
> overhead. As a result, when the end-client of the streaming API
> disconnects from the intermediary, the intermediary leaves the
> connection to Mesos open in an attempt to re-use the connection
> for another request once the response completes. Mesos then thinks
> that the subscriber never disconnected and the intermediary happily
> continues to read the streaming events even though there's no
> end-client.
> 
> To help indicate to intermediaries that the connection SHOULD NOT
> be re-used, we can set the 'Connection: close' header for streaming
> API responses. It may not be respected (since the language seems to
> be SHOULD NOT), but some intermediaries may respect it and close the
> connection if the end-client disconnects.
> 
> Note that libprocess' http server currently doesn't close the the
> connection based on a handler setting this header, but it doesn't
> matter here since the master's operator / scheduler and agent's
> executor streaming API responses are infinite.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp e074a93666d82944774e7b0c3fa32d7021d93c97 
> 
> 
> Diff: https://reviews.apache.org/r/68553/diff/1/
> 
> 
> Testing
> ---
> 
> Manually verified headers make it through:
> 
> ```
> $ telnet 10.0.49.2 5050
> Trying 10.0.49.2...
> Connected to 10.0.49.2.
> Escape character is '^]'.
> POST /master/api/v1 HTTP/1.1
> Content-Type: application/json
> Accept: application/json
> Content-Length: 20
> 
> {"type":"SUBSCRIBE"}
> HTTP/1.1 200 OK
> Transfer-Encoding: chunked
> Content-Type: application/json
> Date: Wed, 29 Aug 2018 01:58:34 GMT
> Connection: close
> 
> 9e
> 154
> {"type":"SUBSCRIBED","subscribed":{"get_state":{"get_tasks":{},"get_executors":{},"get_frameworks":{},"get_agents":{}},"heartbeat_interval_seconds":15.0}}
> 17
> 20
> {"type":"HEARTBEAT"}
> ```
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68490: Optimized `class Resources` with copy-on-write.

[Meng Zhu]

> On Sept. 4, 2018, 2:36 a.m., Benjamin Bannier wrote:
> > Have you benchmarked against a dedicated copy-on-write abstraction? I 
> > understand that the current implementation which only copies when needed 
> > might be more performant, but it also appears to carry a huge mental 
> > overhead to me as users of `Resources` need to always pay a lot of 
> > attention to how used objects could be shared. Using a slightly safer 
> > copy-on-write abstraction might introduce some overhead, but could IMO be 
> > much easier to use and reduced the likelihood of subtle bugs. It would also 
> > help encapsulate low-level issues like `use_count`, see e.g., 
> > https://wg21.cmeerw.net/lwg/issue2776, 
> > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0521r0.html.
> > 
> > I imagine this could be useful elsewhere as well in the future.
> > 
> > Note that implementations like the one presented in 
> > https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Copy-on-write which allow 
> > getting references to wrapped value are prone to subtle issues in user 
> > code. Not directly giving users a reference requires a little more 
> > determination to introduce bugs.
> > 
> > Dummy:
> > 
> > // UNTESTED AND BUGGY. FOR DEMONSTRATION PURPOSES ONLY ;)
> > template  struct cow {
> >   // We can only construct from a value we need to wrap. All other 
> > constructors
> >   // should be deleted as needed.
> >   //
> >   // Verify with e.g., SFINAE that
> >   //
> >   // static_assert(is_convertible_v, T>);
> >   template  explicit cow(U);
> > 
> >   // Allow assignments from a `cow` we own. All other assignment 
> > operators
> >   // should be deleted as needed. These operators need to internally 
> > synchronize
> >   // concurrent access to the wrapped value.
> >   cow =(cow);
> > 
> >   // Non-mutable access to wrapped value.
> >   const T >() const;
> >   const T& operator*() const;
> > 
> >   // Mutating access to the wrapped value. `F` is callable as 
> > `void(T&)` and
> >   // applied to the wrapped value immediately. Synchronizes internally.
> >   template  cow (F) { return *this; }
> > };
> > 
> > void example() {
> > cow i(47);
> > assert(*i == 47);
> > 
> > i.mut([](int ) { x = 11; });
> > assert(*i == 11);
> > }

Thanks a lot for the suggestion, Benjamin! There is certainly room for 
improvement. As Ben mentioned above, the boilerplate code regarding use_count 
is brittle. Hide setters and only expose mutation in a controlled fashion is 
the way to go.

But I need some time to think through and evaluate the options. For now,  only 
code inside the realm of resource.cpp (`class Resources` specifically) needs to 
carry that mental burden. Hopefully, this part of the code is only touched 
sparingly. Added a todo and I will make sure to follow up on this.


- Meng


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


On Sept. 5, 2018, 12:17 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68490/
> ---
> 
> (Updated Sept. 5, 2018, 12:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6765
> https://issues.apache.org/jira/browse/MESOS-6765
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch lets `class Resources` only store shared
> pointers to the underlying resource objects, so that
> read-only filter operations such as `reserved()`,
> `unreserved()` and etc. can avoid making copies of
> the whole resource objects. Instead, only shared pointers
> are copied.
> 
> In write operations, we check if there are more than one
> references to the resource object. If so, a copy is made
> for safe mutation without affecting owners.
> 
> To maintain the usage abstraction that `class Resources`
> still holds resource objects, we utilize
> `boost::indirect_iterator` iterator adapter to deference
> the shared pointers as we iterate.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
>   include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
>   src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
>   src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 
> 
> 
> Diff: https://reviews.apache.org/r/68490/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> Did a quick test on Mac with an optimized build, running benchmark 
> `HierarchicalAllocator_BENCHMARK_Test.ResourceLabels`, here are the results 
> of comparing "before" and "after". Note, DVFS is not fixed. And 

Re: Review Request 68490: Optimized `class Resources` with copy-on-write.

[Meng Zhu]

> On Sept. 3, 2018, 9:59 p.m., Benjamin Mahler wrote:
> > Thanks! Can you also include some commentary on the rest of the allocation 
> > benchmarks as well as the overhead within the Resources micro-benchmarks?

Posted the updated patches. Will summarize and post more comprehensive results 
soon.


> On Sept. 3, 2018, 9:59 p.m., Benjamin Mahler wrote:
> > include/mesos/resources.hpp
> > Lines 678-679 (patched)
> > 
> >
> > What's this referring to? Reading this I see it already has an rvalue 
> > reference?

Referring to `void add(std::shared_ptr&& that);`. Moved the comment 
down to clarifiy it.


> On Sept. 3, 2018, 9:59 p.m., Benjamin Mahler wrote:
> > include/mesos/resources.hpp
> > Lines 682 (patched)
> > 
> >
> > Why do we need this? It seems like the callers should just be 
> > de-referencing their shared_ptr prior to calling?

Consider adding two non-mergeble `Resources`(e.g. 1 cpu + 1 mem), if we only 
have `Resources::add(const Resource_& that)`, we will always need to 
`make_shared(Resource_)`. If we directly pass `shared_ptr`, we can then just 
`push_back`. For any non-mergable `Resource_`, we will be able to save one 
`make_shared`.

One rule of thumb I figured is that, we should avoid `make_shared` when 
possible (because it makes a copy). This means if we already have a 
`shared_ptr` we should just use it and pass it around. This also explains why I 
use `shared_ptr` for foreach loops.


> On Sept. 3, 2018, 9:59 p.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Line 1471 (original), 1473 (patched)
> > 
> >
> > I'm a little puzzled about these `const shared_ptr<...>` foreach loops. 
> > Why don't the const ones do `const Resource_&` loops?
> > 
> > ```
> > foreach (const Resource_& resource_, that) {
> > ```
> > 
> > Something I'm missing?

See my comment above regarding `add(const std::shared_ptr& that)`.

Actual benefits aside, I also want to make it consistent that inside the `class 
Resources`, we are speaking `shared_ptr` unless we want to make an implicit 
copy as in `push/popReservation`.


> On Sept. 3, 2018, 9:59 p.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Line 1503 (original), 1505 (patched)
> > 
> >
> > It seems ok to slip this in here, but generally please send unrelated 
> > cleanups as separate patches. :)

Got it.


> On Sept. 3, 2018, 9:59 p.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Line 1514 (original), 1516-1518 (patched)
> > 
> >
> > I'm not sure how obvious these will be to readers, maybe a comment on 
> > all of these?
> > 
> > ```
> > // Copy-on-write (if more than 1 reference).
> > if (resource_.use_count() > 1) {
> >   resource_ = make_shared(*resource_);
> > }
> > ```
> > 
> > Maybe something like (since we can't use `mutable`):
> > 
> > ```
> > make_mutable(resource_);
> > ```
> > 
> > Maybe also make it composable?
> > 
> > ```
> > 
> > make_mutable(resource_)->resource.mutable_allocation_info()->set_role(role);
> > ```
> > 
> > Still, seems rather error prone and easy to forget to check for copies 
> > prior to mutating? Wonder if there's a way to make it less brittle.

Added the comment. Added a TODO regarding introducing a more controlled 
mutation interface.


> On Sept. 3, 2018, 9:59 p.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Lines 1667-1670 (patched)
> > 
> >
> > Seems odd, why don't we use `Resource` directly here?
> > 
> > ```
> >   Resource r = *resource_;
> >   r.clear_reservations();
> > 
> >   result.add(std::move(r));
> > ```
> > 
> > (I left a comment above about removing the add that takes a shared_ptr, 
> > since it doesn't seem needed?)

See my comments regarding `add` shared_ptr above.


> On Sept. 3, 2018, 9:59 p.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Line 1676 (original), 1692 (patched)
> > 
> >
> > Feel free to pull some of these unrelated improvements out in front of 
> > this, it would help make the diff easier for reviewers.

Got it.


> On Sept. 3, 2018, 9:59 p.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Line 1954 (original), 1972-1979 (patched)
> > 
> >
> > Why is this needed? Isn't the point of this change that toUnreserved 
> > avoids copying 

Re: Review Request 68490: Optimized `class Resources` with copy-on-write.

[Meng Zhu]

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

(Updated Sept. 5, 2018, 12:17 p.m.)


Review request for mesos, Benjamin Mahler and Gastón Kleiman.


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


Repository: mesos


Description
---

This patch lets `class Resources` only store shared
pointers to the underlying resource objects, so that
read-only filter operations such as `reserved()`,
`unreserved()` and etc. can avoid making copies of
the whole resource objects. Instead, only shared pointers
are copied.

In write operations, we check if there are more than one
references to the resource object. If so, a copy is made
for safe mutation without affecting owners.

To maintain the usage abstraction that `class Resources`
still holds resource objects, we utilize
`boost::indirect_iterator` iterator adapter to deference
the shared pointers as we iterate.


Diffs (updated)
-

  include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d 
  include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a 
  src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 
  src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d 


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

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


Testing
---

make check

Did a quick test on Mac with an optimized build, running benchmark 
`HierarchicalAllocator_BENCHMARK_Test.ResourceLabels`, here are the results of 
comparing "before" and "after". Note, DVFS is not fixed. And we only did a 
partial run to verify the validity of the patch, full evaluation coming soon.

**Overall, 33% performance improvement for the 1st steup (1000 agents and 1 
frameworks) and 32% improvement for the first 50 iterarions of the 2nd setup 
(1000 agents and 50 frameworks).**

Before:

[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/0

Using 1000 agents and 1 frameworks
Added 1 frameworks in 1.220022ms
Added 1000 agents in 465.045382ms
round 0 allocate() took 54.803773ms to make 0 offers after filtering 1000 offers
round 1 allocate() took 66.690456ms to make 0 offers after filtering 1000 offers

Using 1000 agents and 50 frameworks
Added 50 frameworks in 1.40774ms
Added 1000 agents in 460.562488ms
round 0 allocate() took 155.500548ms to make 1000 offers after filtering 1000 
offers
round 1 allocate() took 155.194986ms to make 1000 offers after filtering 2000 
offers
round 2 allocate() took 160.706712ms to make 1000 offers after filtering 3000 
offers
round 3 allocate() took 156.701883ms to make 1000 offers after filtering 4000 
offers
round 4 allocate() took 164.246987ms to make 1000 offers after filtering 5000 
offers
round 5 allocate() took 160.791809ms to make 1000 offers after filtering 6000 
offers
round 6 allocate() took 166.064652ms to make 1000 offers after filtering 7000 
offers
round 7 allocate() took 162.177213ms to make 1000 offers after filtering 8000 
offers
round 8 allocate() took 166.620163ms to make 1000 offers after filtering 9000 
offers
round 9 allocate() took 167.970045ms to make 1000 offers after filtering 1 
offers
round 10 allocate() took 166.189463ms to make 1000 offers after filtering 11000 
offers
round 11 allocate() took 170.863462ms to make 1000 offers after filtering 12000 
offers
round 12 allocate() took 172.260473ms to make 1000 offers after filtering 13000 
offers
round 13 allocate() took 170.553911ms to make 1000 offers after filtering 14000 
offers
round 14 allocate() took 235.764593ms to make 1000 offers after filtering 15000 
offers
round 15 allocate() took 247.250433ms to make 1000 offers after filtering 16000 
offers
round 16 allocate() took 276.932824ms to make 1000 offers after filtering 17000 
offers
round 17 allocate() took 248.888469ms to make 1000 offers after filtering 18000 
offers
round 18 allocate() took 193.890556ms to make 1000 offers after filtering 19000 
offers
round 19 allocate() took 195.105346ms to make 1000 offers after filtering 2 
offers
round 20 allocate() took 194.447428ms to make 1000 offers after filtering 21000 
offers
round 21 allocate() took 205.486287ms to make 1000 offers after filtering 22000 
offers
round 22 allocate() took 199.241922ms to make 1000 offers after filtering 23000 
offers
round 23 allocate() took 200.885488ms to make 1000 offers after filtering 24000 
offers
round 24 allocate() took 216.132361ms to make 1000 offers after filtering 25000 
offers
round 25 allocate() took 210.638273ms to make 1000 offers after filtering 26000 
offers
round 26 allocate() took 232.397778ms to make 1000 offers after filtering 27000 
offers
round 27 allocate() took 239.633708ms to make 1000 offers after filtering 28000 
offers
round 28 allocate() took 225.829677ms to make 1000 offers after filtering 29000 
offers

Re: Review Request 65168: Fixed HTTP errors caused by dropped HTTP responses by IOSwitchboard.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65168 was successfully built and tested.

Reviews applied: `['68232', '68230', '68231', '65168']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2296/mesos-review-65168

- Mesos Reviewbot Windows


On Sept. 5, 2018, 4:34 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65168/
> ---
> 
> (Updated Sept. 5, 2018, 4:34 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Qian Zhang.
> 
> 
> Bugs: MESOS-8545
> https://issues.apache.org/jira/browse/MESOS-8545
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, IOSwitchboard process could terminate before all HTTP
> responses had been sent to the agent. In the case of
> `ATTACH_CONTAINER_INPUT` call, we could drop a final HTTP `200 OK`
> response, so the agent got broken HTTP connection for the call.
> This patch introduces an acknowledgment for the received response
> for the `ATTACH_CONTAINER_INPUT` call. This acknowledgment is a new
> type of control messages for the `ATTACH_CONTAINER_INPUT` call. When
> IOSwitchboard receives an acknowledgment, and io redirects are
> finished, it terminates itself. That guarantees that the agent always
> receives a response for the `ATTACH_CONTAINER_INPUT` call.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 52b0e521ed1c651c90b3a3df7c4df576288bf400 
>   src/slave/http.hpp 78200874cb03a3d5af4ee0443c03a1d94cd494cc 
>   src/slave/http.cpp fb923686e92ff635e99cc2ccc6316f0282ab3e3a 
> 
> 
> Diff: https://reviews.apache.org/r/65168/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 65168: Fixed HTTP errors caused by dropped HTTP responses by IOSwitchboard.

[Andrei Budnik]

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

(Updated Sept. 5, 2018, 4:34 p.m.)


Review request for mesos and Alexander Rukletsov.


Summary (updated)
-

Fixed HTTP errors caused by dropped HTTP responses by IOSwitchboard.


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


Repository: mesos


Description (updated)
---

Previously, IOSwitchboard process could terminate before all HTTP
responses had been sent to the agent. In the case of
`ATTACH_CONTAINER_INPUT` call, we could drop a final HTTP `200 OK`
response, so the agent got broken HTTP connection for the call.
This patch introduces an acknowledgment for the received response
for the `ATTACH_CONTAINER_INPUT` call. This acknowledgment is a new
type of control messages for the `ATTACH_CONTAINER_INPUT` call. When
IOSwitchboard receives an acknowledgment, and io redirects are
finished, it terminates itself. That guarantees that the agent always
receives a response for the `ATTACH_CONTAINER_INPUT` call.


Diffs (updated)
-

  include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
  src/slave/containerizer/mesos/io/switchboard.cpp 
52b0e521ed1c651c90b3a3df7c4df576288bf400 
  src/slave/http.hpp 78200874cb03a3d5af4ee0443c03a1d94cd494cc 
  src/slave/http.cpp fb923686e92ff635e99cc2ccc6316f0282ab3e3a 


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

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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 68568: Added '/roles' to the set of batched master endpoints.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68568 was successfully built and tested.

Reviews applied: `['68567', '68568']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2295/mesos-review-68568

- Mesos Reviewbot Windows


On Sept. 5, 2018, 12:18 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68568/
> ---
> 
> (Updated Sept. 5, 2018, 12:18 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-9194
> https://issues.apache.org/jira/browse/MESOS-9194
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For improved consistency, the '/roles' endpoint on the
> master is now marked as read-only and uses the batching
> mechanism shared by the other read-only endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp e074a93666d82944774e7b0c3fa32d7021d93c97 
>   src/master/master.hpp eecb66c8826b2b681ef94e6457c2651fc63c724b 
>   src/master/readonly_handler.cpp 47d7de5bce4f6b21134596fe53dd02e457f9c069 
> 
> 
> Diff: https://reviews.apache.org/r/68568/diff/2/
> 
> 
> Testing
> ---
> 
> Successful internal CI run executing `make check` on several platforms. 
> (Jenkins id #4290)
> 
> For rev. 2, internal CI run #4340.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 68568: Added '/roles' to the set of batched master endpoints.

[Benno Evers]

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

(Updated Sept. 5, 2018, 12:18 p.m.)


Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description (updated)
---

For improved consistency, the '/roles' endpoint on the
master is now marked as read-only and uses the batching
mechanism shared by the other read-only endpoints.


Diffs (updated)
-

  src/master/http.cpp e074a93666d82944774e7b0c3fa32d7021d93c97 
  src/master/master.hpp eecb66c8826b2b681ef94e6457c2651fc63c724b 
  src/master/readonly_handler.cpp 47d7de5bce4f6b21134596fe53dd02e457f9c069 


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

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


Testing
---

Successful internal CI run executing `make check` on several platforms. 
(Jenkins id #4290)


Thanks,

Benno Evers

Re: Review Request 68567: Restructured /roles code.

[Benno Evers]

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

(Updated Sept. 5, 2018, 12:14 p.m.)


Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description (updated)
---

Reworked the structure of the role handling in both
v0 and v1 APIs in preparation for the subsequent commit.

As a nice side bonus on top, this also saves one trip
through the master queue for every call of this endpoint.


Diffs (updated)
-

  src/master/http.cpp e074a93666d82944774e7b0c3fa32d7021d93c97 
  src/master/master.hpp eecb66c8826b2b681ef94e6457c2651fc63c724b 


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

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


Testing
---

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


Thanks,

Benno Evers

Re: Review Request 68627: Brought site dependencies up to date.

[Till Toenshoff via Review Board]

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


Ship it!




Ship It!

- Till Toenshoff


On Sept. 5, 2018, 8:53 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68627/
> ---
> 
> (Updated Sept. 5, 2018, 8:53 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit bumps versions of indirect site dependencies to their
> latest version without bumping any pinned direct dependencies.
> 
> 
> Diffs
> -
> 
>   site/Gemfile.lock 5143ee5b7671cf22ec7fb1d80d74d3efa18eb218 
> 
> 
> Diff: https://reviews.apache.org/r/68627/diff/1/
> 
> 
> Testing
> ---
> 
> Manually tested.
> 
> $ bundle install
> $ bundle exec rake
> $ bundle exec middleman server
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 68627: Brought site dependencies up to date.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Sept. 5, 2018, 8:53 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68627/
> ---
> 
> (Updated Sept. 5, 2018, 8:53 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit bumps versions of indirect site dependencies to their
> latest version without bumping any pinned direct dependencies.
> 
> 
> Diffs
> -
> 
>   site/Gemfile.lock 5143ee5b7671cf22ec7fb1d80d74d3efa18eb218 
> 
> 
> Diff: https://reviews.apache.org/r/68627/diff/1/
> 
> 
> Testing
> ---
> 
> Manually tested.
> 
> $ bundle install
> $ bundle exec rake
> $ bundle exec middleman server
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 68627: Brought site dependencies up to date.

[Benjamin Bannier]

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

Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

This commit bumps versions of indirect site dependencies to their
latest version without bumping any pinned direct dependencies.


Diffs
-

  site/Gemfile.lock 5143ee5b7671cf22ec7fb1d80d74d3efa18eb218 


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


Testing
---

Manually tested.

$ bundle install
$ bundle exec rake
$ bundle exec middleman server


Thanks,

Benjamin Bannier