Re: Review Request 67993: Avoid resource copying while serving state json.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 67987.

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

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1960/mesos-review-67993

Relevant logs:

- 
[apply-review-67987-stdout.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1960/mesos-review-67993/logs/apply-review-67987-stdout.log):

```
error: missing binary patch data for '3rdparty/rapidjson-1.1.0.tar.gz'
error: binary patch does not apply to '3rdparty/rapidjson-1.1.0.tar.gz'
error: 3rdparty/rapidjson-1.1.0.tar.gz: patch does not apply
```

- Mesos Reviewbot Windows


On July 20, 2018, 5:23 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67993/
> ---
> 
> (Updated July 20, 2018, 5:23 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benno Evers.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The state serving code was constructing `Resources` wrappers in
> order to jsonify resources. This incurs a copy of the underlying
> Resource objects which is unnecessary and can be costly. This
> removes the copy by proving a direct way to jsonify
> `RepeatedPtrField`.
> 
> This shows a small 10-15% reduction in the master StateQuery
> benchmark timings:
> 
>minq1q3   max
> baseline  3.48  3.54  4.12  4.40
> after this patch  2.82  3.11  3.31  3.71
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 9d2b01f5cb0f787d1ea84d298b25b7e0d072cc66 
>   src/common/http.cpp 9dfbfd12e22b215a24d0598b5c87d96807dc7a6c 
>   src/master/http.cpp c855de1b7bb71cb455d111fcd71c4486f421f2c2 
>   src/slave/http.cpp cf93c487b0936baf70238ffe6eba62f752ea9a81 
> 
> 
> Diff: https://reviews.apache.org/r/67993/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 67993: Avoid resource copying while serving state json.

[Benjamin Mahler]

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

Review request for mesos, Alexander Rukletsov and Benno Evers.


Repository: mesos


Description
---

The state serving code was constructing `Resources` wrappers in
order to jsonify resources. This incurs a copy of the underlying
Resource objects which is unnecessary and can be costly. This
removes the copy by proving a direct way to jsonify
`RepeatedPtrField`.

This shows a small 10-15% reduction in the master StateQuery
benchmark timings:

   minq1q3   max
baseline  3.48  3.54  4.12  4.40
after this patch  2.82  3.11  3.31  3.71


Diffs
-

  src/common/http.hpp 9d2b01f5cb0f787d1ea84d298b25b7e0d072cc66 
  src/common/http.cpp 9dfbfd12e22b215a24d0598b5c87d96807dc7a6c 
  src/master/http.cpp c855de1b7bb71cb455d111fcd71c4486f421f2c2 
  src/slave/http.cpp cf93c487b0936baf70238ffe6eba62f752ea9a81 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 67980: Windows: Enabled rest of `ProcessTest` suite.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67951, 67952, 67976, 67977, 67978, 67979, 67980]

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

- Mesos Reviewbot


On July 19, 2018, 8:47 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67980/
> ---
> 
> (Updated July 19, 2018, 8:47 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> Liangyu Zhao, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-5817 and MESOS-7527
> https://issues.apache.org/jira/browse/MESOS-5817
> https://issues.apache.org/jira/browse/MESOS-7527
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Several of these worked as-is, and the two firewall tests were fixed
> by replace `path::join` (which joined with ``) with
> `strings::join("/")`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 8baf60d8bbb675e26fea5e76c825ef73fedbc629 
> 
> 
> Diff: https://reviews.apache.org/r/67980/diff/1/
> 
> 
> Testing
> ---
> 
> `ctest -V` on Windows and Ubuntu 18.04
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67992: Avoid performance cost of ostringstream in http::OK json constructors.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 67987.

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

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1959/mesos-review-67992

Relevant logs:

- 
[apply-review-67987-stdout.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1959/mesos-review-67992/logs/apply-review-67987-stdout.log):

```
error: missing binary patch data for '3rdparty/rapidjson-1.1.0.tar.gz'
error: binary patch does not apply to '3rdparty/rapidjson-1.1.0.tar.gz'
error: 3rdparty/rapidjson-1.1.0.tar.gz: patch does not apply
```

- Mesos Reviewbot Windows


On July 20, 2018, 3:39 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67992/
> ---
> 
> (Updated July 20, 2018, 3:39 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benno Evers.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the common case where no jsonp is passed this avoids an extra copy
> out from the ostringstream. In the jsonp case, the performance should
> be near identical or better as we now pre-reserve the space needed
> for the body.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/http.cpp cbb910282a4aff8a914dbd13ff20fc70d929c269 
> 
> 
> Diff: https://reviews.apache.org/r/67992/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 67992: Avoid performance cost of ostringstream in http::OK json constructors.

[Benjamin Mahler]

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

Review request for mesos, Alexander Rukletsov and Benno Evers.


Repository: mesos


Description
---

In the common case where no jsonp is passed this avoids an extra copy
out from the ostringstream. In the jsonp case, the performance should
be near identical or better as we now pre-reserve the space needed
for the body.


Diffs
-

  3rdparty/libprocess/src/http.cpp cbb910282a4aff8a914dbd13ff20fc70d929c269 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 67991: Adjusted Mesos to compile against jsonify rapidjson changes.

[Benjamin Mahler]

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

Review request for mesos, Alexander Rukletsov and Benno Evers.


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


Repository: mesos


Description
---

The `StringWriter` no longer allows appending multiple times
and instead has a `set` function that must be used.


Diffs
-

  src/common/http.cpp 9dfbfd12e22b215a24d0598b5c87d96807dc7a6c 
  src/master/http.cpp c855de1b7bb71cb455d111fcd71c4486f421f2c2 
  src/slave/http.cpp cf93c487b0936baf70238ffe6eba62f752ea9a81 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 67989: Fixed issues with the JSON serialization tests.

[Benjamin Mahler]

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

Review request for mesos, Alexander Rukletsov and Benno Evers.


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


Repository: mesos


Description
---

There were some issues in the existing JSON serialization tests
when it comes to rapidjson. For example:

  - 0x7F does not need to be escaped per ECMA-404. This is what
rapidjson adheres to.
  - Forward slash does not need to be escaped.

This updates the test to have a valid UTF-8 case with a multi-byte
sequence (which does not need to be escaped per ECMA-404), as well
as an invalid case where a multibyte sequence is partial (which we
allow for now since we do not have validation errors exposed).


Diffs
-

  3rdparty/stout/tests/json_tests.cpp 508d8433797a81ec7f720ddc678a007297fec5b3 
  3rdparty/stout/tests/jsonify_tests.cpp 
7de95a4a15912fae3c9269733758e765abc7330d 


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


Testing
---

Tested at the end of this chain, since this is split across 
stout/libprocess/mesos.


Thanks,

Benjamin Mahler

Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Benjamin Mahler]

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

Review request for mesos, Alexander Rukletsov, Benno Evers, and Michael Park.


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


Repository: mesos


Description
---

This reduces the time needed for the client to finish receiving a
master's /state response by 50% in the `StateQuery` benchmark:

minq1q3   max
baseline   6.52  6.76  7.33  8.26
rapidjson w/ SIMD  3.48  3.54  4.12  4.4
rapidjson  3.29  3.32  3.65  3.85

SIMD is left disabled for now since it showed slightly slower
results.


Diffs
-

  3rdparty/stout/include/stout/jsonify.hpp 
2314980e185ee61cc2ea54f1b4d2a8b35e58121c 


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


Testing
---

Tested at the end of this chain, since this is split across 
stout/libprocess/mesos.


Thanks,

Benjamin Mahler

Review Request 67985: Added rapidjson to the stout build.

[Benjamin Mahler]

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

Review request for mesos, Alexander Rukletsov and Benno Evers.


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


Repository: mesos


Description
---

This follows the approach used for picojson.


Diffs
-

  3rdparty/stout/3rdparty/Makefile.am cc831a93a57d970bc28d754acd04569780154bee 
  3rdparty/stout/CMakeLists.txt 9cbb6f2a13fe9201fdebb9e9994d7725e53af083 
  3rdparty/stout/Makefile.am 44710c21d3c5a2bc2309b0fe10419712523d3187 
  3rdparty/stout/configure.ac 7e85c7efccdcbca8f3ef1173075384b0060a15c7 


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


Testing
---

Tested at the end of this chain, since this is split across 
stout/libprocess/mesos.


Thanks,

Benjamin Mahler

Review Request 67986: Added rapidjson to the libprocess build.

[Benjamin Mahler]

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

Review request for mesos, Alexander Rukletsov and Benno Evers.


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


Repository: mesos


Description
---

This follows the approach used for picojson.


Diffs
-

  3rdparty/libprocess/3rdparty/Makefile.am 
d13e93f0ce8ae054161dcc4017b1f16d0d5596e1 
  3rdparty/libprocess/Makefile.am e0d35d9fdddcecf6498f9e617a9c5c604a7f9fc6 
  3rdparty/libprocess/configure.ac 7fa596aabf6f2082aa6cde2a36d8bae7becdef47 


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


Testing
---

Tested at the end of this chain, since this is split across 
stout/libprocess/mesos.


Thanks,

Benjamin Mahler

Review Request 67990: Fixed libprocess tests against rapidjson.

[Benjamin Mahler]

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

Review request for mesos, Alexander Rukletsov and Benno Evers.


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


Repository: mesos


Description
---

The libprocess tests were checking the serialized format of metrics,
which previously escaped forward slashes. However, this is not what
rapidjson does and it's also valid json according to ECMA-404.


Diffs
-

  3rdparty/libprocess/src/tests/metrics_tests.cpp 
8590bdb4da1bead462a5b91c23bbc31905e57a47 


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


Testing
---

Tested at the end of this chain, since this is split across 
stout/libprocess/mesos.


Thanks,

Benjamin Mahler

Review Request 67987: Added rapidjson to the mesos build.

[Benjamin Mahler]

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

Review request for mesos, Alexander Rukletsov, Benno Evers, and James Peach.


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


Repository: mesos


Description
---

This includes a stripped bundle of the latest release. Stripping
is required for licensing (see rapidjson.md), but also helps reduce
the bloat in the mesos git repo.

Also included is a readme for how to update the dependency.


Diffs
-

  3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
  3rdparty/Makefile.am 6123b8a0164d50f0bd771d2e5f485379811e7fcb 
  3rdparty/README.md fa9a6483ca2dc4980767a2cb723d654e342e6bb8 
  3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a 
  3rdparty/rapidjson-1.1.0.tar.gz PRE-CREATION 
  3rdparty/rapidjson.md PRE-CREATION 
  3rdparty/versions.am 57792800c4b4b67dd55a316ce00480db6c253d34 
  configure.ac e86c496140ee9732ad3204ef22ae8ce60cd5c079 
  src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 


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


Testing
---

Tested at the end of this chain, since this is split across 
stout/libprocess/mesos.


Thanks,

Benjamin Mahler

Re: Review Request 67984: Windows: Added CMake logic to download and "install" `wclayer.exe`.

[Mesos Reviewbot Windows]

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



Bad review!

Error:
Circular dependency detected for review 67931. Please fix the 'depends_on' 
field.

- Mesos Reviewbot Windows


On July 19, 2018, 5:07 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67984/
> ---
> 
> (Updated July 19, 2018, 5:07 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added CMake logic to download and "install" `wclayer.exe`.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
>   src/slave/CMakeLists.txt e5fe32a265e310f568139127ee6e5f441590985c 
> 
> 
> Diff: https://reviews.apache.org/r/67984/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>

Re: Review Request 67914: Updated XFS disk isolator to reclaim project IDs after disk GC.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67914 was successfully built and tested.

Reviews applied: `['67914']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1958/mesos-review-67914

- Mesos Reviewbot Windows


On July 19, 2018, 11:12 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67914/
> ---
> 
> (Updated July 19, 2018, 11:12 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-9007
> https://issues.apache.org/jira/browse/MESOS-9007
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently upon container destruction its project ID is unallocated by
> the isolator and removed from the container work directory. However due
> to API limitations we can't unset project IDs on symlinks that may exist
> inside the directory. Because of that the project may still exist until
> the container directory is garbage collected. If the project ID is
> reused for a new container, any lingering symlinks that still have that
> project ID will contribute to disk usage of the new container. Typically
> symlinks don't take much space, but still this leads to inaccuracy in
> disk space usage accounting.
> 
> This patch postpones project ID reclaiming until sandbox GC time. The
> isolator periodically checks if sandboxes of terminated containers still
> exist and deallocates project IDs of the ones that were removed. This
> mechanism can be improved in the future if we introduce a way for the
> isolators to learn about disk GCs.
> 
> Current number of available project IDs can be tracked with the new
> "containerizer/mesos/disk/project_ids_free" metric.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 0891f7709aa4f98758a727856d58e6177d46adca 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 25f52a43b34b141bdaf7c448817423cf4264e22a 
>   src/slave/flags.hpp eeb9708f9ec76d83b6719541f4a012544c7c0cbe 
>   src/slave/flags.cpp 58cdc0f1100fe244e5bf1036e1ccf39478d5d478 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> dc18a8a59d1eb7fae3592ef6ba8c046e4f46ee4a 
> 
> 
> Diff: https://reviews.apache.org/r/67914/diff/3/
> 
> 
> Testing
> ---
> 
> Added `ROOT_XFS_QuotaTest.ProjectIDReclaiming` test that verifies that 
> project ID is reclaimed and reused after sandbox GC. Ran `sudo make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Review Request 67984: Windows: Added CMake logic to download and "install" `wclayer.exe`.

[Liangyu Zhao via Review Board]

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

Review request for mesos.


Repository: mesos


Description
---

Windows: Added CMake logic to download and "install" `wclayer.exe`.


Diffs
-

  3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
  src/slave/CMakeLists.txt e5fe32a265e310f568139127ee6e5f441590985c 


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


Testing
---


Thanks,

Liangyu Zhao

Re: Review Request 67878: Added/updated tests to check per-framework metrics.

[Gastón Kleiman]

> On July 19, 2018, 3:45 p.m., Gastón Kleiman wrote:
> > Can you add a test that verifies whether allocator and non-allocator 
> > per-framework metrics are evicted once the `max_completed_frameworks` bound 
> > is reached?

And also one testing the `suppressed` metric.


- Gastón


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


On July 17, 2018, 6:46 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67878/
> ---
> 
> (Updated July 17, 2018, 6:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added/updated tests to check per-framework metrics.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 8e04023ed04e79881e0d323c2e2283bebaf262eb 
> 
> 
> Diff: https://reviews.apache.org/r/67878/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 67914: Updated XFS disk isolator to reclaim project IDs after disk GC.

[Ilya Pronin]

> On July 18, 2018, 1:51 p.m., James Peach wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Line 495 (original), 504 (patched)
> > 
> >
> > I'm a little concerned that we could exhaust the project IDs (we 
> > default to 5000 IDs). What do you think about adding a metric for the 
> > number of available project IDs?

Good idea. Added `containerizer/mesos/disk/project_ids_free` metric.


> On July 18, 2018, 1:51 p.m., James Peach wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 547 (patched)
> > 
> >
> > Let's make this message symmetric with the one we emit when we assign:
> > 
> > ```
> > LOG(INFO) << "Reclaimed project " << stringify(projectId.get()) << " 
> > from '"
> >   << containerConfig.directory() << "'";
> > ```

Fixed.


> On July 18, 2018, 1:51 p.m., James Peach wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 549 (patched)
> > 
> >
> > If possible, I think this is simple enough to inline into the check loop

Definitely possible, but I would prefer to keep that logical separation unless 
you feel strongly about that :) `loop()` schedules the periodic check and 
`checkProjectIdUsage()` actually performs that check.


> On July 18, 2018, 1:51 p.m., James Peach wrote:
> > src/slave/flags.cpp
> > Lines 1338 (patched)
> > 
> >
> > Rather than introducing a new flag, lets use 
> > `container_disk_watch_interval` or `disk_watch_interval` ... probably the 
> > former?

I think if we do this then we should rather use the latter because disk GC is 
driven by `disk_watch_interval` and `gc_delay` (maybe use which one of those 2 
is the smallest?). `container_disk_watch_interval` will most likely be set to a 
smaller value to promptly terminate containers that have breached their quota 
(that's true in our cluster). Running sandboxes check at that frequently would 
be wasteful.


> On July 18, 2018, 1:51 p.m., James Peach wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp
> > Lines 1197 (patched)
> > 
> >
> > For consistency with other tests, could we call this 
> > `ProjectIdReclaiming`?
> > 
> > Can you please add a short comment explaining the purpose of the test?

Done.


> On July 18, 2018, 1:51 p.m., James Peach wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp
> > Lines 1319 (patched)
> > 
> >
> > Is this re-using the futures an OK thing to do? If they are already 
> > ready from the previous task launch, won't they still be ready when we 
> > await them here?

Yes, it seems to be safe. The `Future` is reset in `FutureArg()` during 
expectations setup: 
https://github.com/apache/mesos/blob/9147283171d761a4d38710f24ba654f8a96e325c/3rdparty/libprocess/include/process/gmock.hpp#L82


> On July 18, 2018, 1:51 p.m., James Peach wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp
> > Lines 1330 (patched)
> > 
> >
> > This can be `EXPECT_EQ`.

Fixed.


> On July 18, 2018, 1:51 p.m., James Peach wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp
> > Lines 1332 (patched)
> > 
> >
> > Is the link the `latest` link? Can you add an explanatory comment?

Done.


> On July 18, 2018, 1:51 p.m., James Peach wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp
> > Lines 1333 (patched)
> > 
> >
> > This can me `EXPECT_SOME_EQ`.

Fixed.


- Ilya


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


On July 19, 2018, 4:12 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67914/
> ---
> 
> (Updated July 19, 2018, 4:12 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-9007
> https://issues.apache.org/jira/browse/MESOS-9007
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently upon container destruction its project ID is unallocated by
> the isolator and removed from the container work directory. However due
> to API limitations we can't unset project IDs on symlinks that may 

Re: Review Request 67914: Updated XFS disk isolator to reclaim project IDs after disk GC.

[Ilya Pronin]

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

(Updated July 19, 2018, 4:12 p.m.)


Review request for mesos and James Peach.


Changes
---

Addressed review comments.


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


Repository: mesos


Description (updated)
---

Currently upon container destruction its project ID is unallocated by
the isolator and removed from the container work directory. However due
to API limitations we can't unset project IDs on symlinks that may exist
inside the directory. Because of that the project may still exist until
the container directory is garbage collected. If the project ID is
reused for a new container, any lingering symlinks that still have that
project ID will contribute to disk usage of the new container. Typically
symlinks don't take much space, but still this leads to inaccuracy in
disk space usage accounting.

This patch postpones project ID reclaiming until sandbox GC time. The
isolator periodically checks if sandboxes of terminated containers still
exist and deallocates project IDs of the ones that were removed. This
mechanism can be improved in the future if we introduce a way for the
isolators to learn about disk GCs.

Current number of available project IDs can be tracked with the new
"containerizer/mesos/disk/project_ids_free" metric.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
0891f7709aa4f98758a727856d58e6177d46adca 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
25f52a43b34b141bdaf7c448817423cf4264e22a 
  src/slave/flags.hpp eeb9708f9ec76d83b6719541f4a012544c7c0cbe 
  src/slave/flags.cpp 58cdc0f1100fe244e5bf1036e1ccf39478d5d478 
  src/tests/containerizer/xfs_quota_tests.cpp 
dc18a8a59d1eb7fae3592ef6ba8c046e4f46ee4a 


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

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


Testing
---

Added `ROOT_XFS_QuotaTest.ProjectIDReclaiming` test that verifies that project 
ID is reclaimed and reused after sandbox GC. Ran `sudo make check`.


Thanks,

Ilya Pronin

Re: Review Request 66870: Added per-framework metrics for suppressed roles.

[Gastón Kleiman]

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



Can you add a test? At least one that calls suppress/revive, but ideally also 
one that adds and removes roles.


src/master/allocator/mesos/hierarchical.cpp
Line 489 (original), 498 (patched)


We need to call `FrameworkMetrics::addRole()` here.

Ditto `FrameworkMetrics::removeRole()` in the corresponding place.


- Gastón Kleiman


On July 17, 2018, 6:45 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66870/
> ---
> 
> (Updated July 17, 2018, 6:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, Gilbert Song, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added per-framework metrics for suppressed roles.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5a6cd3d2fc5bdbaaee2d05b9be9e83d4107c749b 
>   src/master/allocator/mesos/metrics.hpp 
> 6d386225c301d5ab44f3cc0ecdd1478fb5162e5b 
>   src/master/allocator/mesos/metrics.cpp 
> 82990b2dc0b827a43a392d898667eaf58c77ea36 
> 
> 
> Diff: https://reviews.apache.org/r/66870/diff/5/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 66856: Tracked completed framework metrics in the allocator.

[Gastón Kleiman]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.cpp
Lines 346-350 (patched)


This looks good, but the framework removal flow is not straightforward. I'd 
feel much more confident in the change if there was a test that verifies that 
completed frameworks are evicted once `max_completed_frameworks` is reached.

The test could start a master with `--max_completed_frameworks=2`, register 
three frameworks and then check the metrics.

I think you could add that test to https://reviews.apache.org/r/67878/ or 
in a new patch.


- Gastón Kleiman


On July 17, 2018, 6:45 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66856/
> ---
> 
> (Updated July 17, 2018, 6:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, Gilbert Song, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This ensures that per-framework metrics which are tracked in the
> allocator will be retained as long as the per-framework metrics
> which are tracked in the master.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> c19ab64ff1422a68cfb6eecea7e2cc4132da8dcc 
>   src/master/allocator/mesos/allocator.hpp 
> 900c8ee405da6e44532dee598edaa42373ebd4e5 
>   src/master/allocator/mesos/hierarchical.hpp 
> 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5a6cd3d2fc5bdbaaee2d05b9be9e83d4107c749b 
>   src/master/master.cpp 487ee349ef0cd78903ff6bcdea3f24688810cdc5 
>   src/tests/allocator.hpp 73fc06043746a0498d0dd1846fae9433db136d49 
>   src/tests/api_tests.cpp 728fb4c38c17c98b6c2a85447d3f32519c035e56 
>   src/tests/master_allocator_tests.cpp 
> 824a7554858fb8356751f34699607505bd98 
>   src/tests/master_quota_tests.cpp d836482f5593b462ad235620741148678ac2651d 
>   src/tests/reservation_tests.cpp 058a66d7914c7a84f0ba86dfd3ff2e3c0bbcb5c6 
>   src/tests/resource_offers_tests.cpp 
> 6e24cddce65e7f5a0ce8ea4fde8397a19236d8b8 
>   src/tests/slave_recovery_tests.cpp e8333402d8524a4bf302872d246fe5f5c006bdc5 
> 
> 
> Diff: https://reviews.apache.org/r/66856/diff/6/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 67878: Added/updated tests to check per-framework metrics.

[Gastón Kleiman]

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



Can you add a test that verifies whether allocator and non-allocator 
per-framework metrics are evicted once the `max_completed_frameworks` bound is 
reached?

- Gastón Kleiman


On July 17, 2018, 6:46 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67878/
> ---
> 
> (Updated July 17, 2018, 6:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added/updated tests to check per-framework metrics.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 8e04023ed04e79881e0d323c2e2283bebaf262eb 
> 
> 
> Diff: https://reviews.apache.org/r/67878/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 67980: Windows: Enabled rest of `ProcessTest` suite.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67980 was successfully built and tested.

Reviews applied: `['67951', '67952', '67976', '67977', '67978', '67979', 
'67980']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1957/mesos-review-67980

- Mesos Reviewbot Windows


On July 19, 2018, 1:47 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67980/
> ---
> 
> (Updated July 19, 2018, 1:47 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> Liangyu Zhao, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-5817 and MESOS-7527
> https://issues.apache.org/jira/browse/MESOS-5817
> https://issues.apache.org/jira/browse/MESOS-7527
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Several of these worked as-is, and the two firewall tests were fixed
> by replace `path::join` (which joined with ``) with
> `strings::join("/")`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 8baf60d8bbb675e26fea5e76c825ef73fedbc629 
> 
> 
> Diff: https://reviews.apache.org/r/67980/diff/1/
> 
> 
> Testing
> ---
> 
> `ctest -V` on Windows and Ubuntu 18.04
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67980: Windows: Enabled rest of `ProcessTest` suite.

[Andrew Schwartzmeyer]

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

(Updated July 19, 2018, 1:47 p.m.)


Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
Liangyu Zhao, and Radhika Jandhyala.


Changes
---

Added bug.


Bugs: MESOS-5817 and MESOS-7527
https://issues.apache.org/jira/browse/MESOS-5817
https://issues.apache.org/jira/browse/MESOS-7527


Repository: mesos


Description
---

Several of these worked as-is, and the two firewall tests were fixed
by replace `path::join` (which joined with ``) with
`strings::join("/")`.


Diffs
-

  3rdparty/libprocess/src/tests/process_tests.cpp 
8baf60d8bbb675e26fea5e76c825ef73fedbc629 


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


Testing
---

`ctest -V` on Windows


Thanks,

Andrew Schwartzmeyer

Review Request 67980: Windows: Enabled rest of `ProcessTest` suite.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
Liangyu Zhao, and Radhika Jandhyala.


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


Repository: mesos


Description
---

Several of these worked as-is, and the two firewall tests were fixed
by replace `path::join` (which joined with ``) with
`strings::join("/")`.


Diffs
-

  3rdparty/libprocess/src/tests/process_tests.cpp 
8baf60d8bbb675e26fea5e76c825ef73fedbc629 


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


Testing
---

`ctest -V` on Windows


Thanks,

Andrew Schwartzmeyer

Review Request 67979: Windows: Documented why the `RemoteLinkLeak` test is not enabled.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
Liangyu Zhao, and Radhika Jandhyala.


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


Repository: mesos


Description
---

Windows: Documented why the `RemoteLinkLeak` test is not enabled.


Diffs
-

  3rdparty/libprocess/src/tests/process_tests.cpp 
8baf60d8bbb675e26fea5e76c825ef73fedbc629 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 67977: Fixed `test-linkee` logic in `ProcessRemoteLinkTest::SetUp()`.

[Radhika Jandhyala via Review Board]

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


Ship it!




Ship It!

- Radhika Jandhyala


On July 19, 2018, 8:44 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67977/
> ---
> 
> (Updated July 19, 2018, 8:44 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> Liangyu Zhao, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-5941 and MESOS-9097
> https://issues.apache.org/jira/browse/MESOS-5941
> https://issues.apache.org/jira/browse/MESOS-9097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Several problems existed on Windows here:
> 
> * The `BUILD_DIR` path has forward slashes (probably fine, but wrong).
> * The executable must be named correctly, `.exe` and all.
> * We should assert that `test-linkee` exists.
> * The shell should not be used, otherwise `'(62)'` will resolve to an
>   unknown UPID inside `test-linkee` because of the single quotes.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 8baf60d8bbb675e26fea5e76c825ef73fedbc629 
> 
> 
> Diff: https://reviews.apache.org/r/67977/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Review Request 67978: Windows: Enabled `RemoteLink` tests.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
Liangyu Zhao, and Radhika Jandhyala.


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


Repository: mesos


Description
---

This resolves MESOS-5941. Note that on Windows, `os::killtree()` is
only valid for processes specifically spawned in a group via a job
object, otherwise it will fail (and the failure is not being checked
here). Since the `linkee` process only ever spawns the single OS
process `test-linkee`, it is safe to switch to `os::kill()` which can
correctly kill it.


Diffs
-

  3rdparty/libprocess/src/tests/process_tests.cpp 
8baf60d8bbb675e26fea5e76c825ef73fedbc629 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 67977: Fixed `test-linkee` logic in `ProcessRemoteLinkTest::SetUp()`.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
Liangyu Zhao, and Radhika Jandhyala.


Bugs: MESOS-5941 and MESOS-9097
https://issues.apache.org/jira/browse/MESOS-5941
https://issues.apache.org/jira/browse/MESOS-9097


Repository: mesos


Description
---

Several problems existed on Windows here:

* The `BUILD_DIR` path has forward slashes (probably fine, but wrong).
* The executable must be named correctly, `.exe` and all.
* We should assert that `test-linkee` exists.
* The shell should not be used, otherwise `'(62)'` will resolve to an
  unknown UPID inside `test-linkee` because of the single quotes.


Diffs
-

  3rdparty/libprocess/src/tests/process_tests.cpp 
8baf60d8bbb675e26fea5e76c825ef73fedbc629 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Review Request 67976: Windows: Added `nullptr` checks when using `libwinio_loop` pointer.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
Liangyu Zhao, and Radhika Jandhyala.


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


Repository: mesos


Description
---

It was discovered that the `Socket` constructor could dereference a
null pointer (by way of `prepare_async()`) if the Windows IOCP event
loop had not yet been initialized. So now we check for its
initialization before each dereference, and return an error or fatal
log event.

In order to ensure that it is initialized in `test-linkee`, we call
`process::initialize()`. This should be fixed in the future, per
MESOS-9097.


Diffs
-

  3rdparty/libprocess/src/tests/test_linkee.cpp 
cc482717290f72a5fd95fe745ac01893c0ce41f8 
  3rdparty/libprocess/src/windows/event_loop.cpp 
0050ff0d87fdd01bf37742233fcd38b02f284ff3 
  3rdparty/libprocess/src/windows/io.cpp 
1f9adde36192b673d7051549295a0f403be8e718 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 67951: Added optional `path_separator` parameter to `Path` constructor.

[Andrew Schwartzmeyer]

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

(Updated July 19, 2018, 1:41 p.m.)


Review request for mesos, Eric Mumau, John Kordich, Joseph Wu, Liangyu Zhao, 
and Radhika Jandhyala.


Changes
---

Removed const member so that it can be default copy constructed.


Bugs: MESOS-5817 and MESOS-5904
https://issues.apache.org/jira/browse/MESOS-5817
https://issues.apache.org/jira/browse/MESOS-5904


Repository: mesos


Description
---

This defaults to `os::PATH_SEPARATOR` and so by default retains the
previous behavior. However, now `Path` can be arbitrarily used with,
e.g., URLs on Windows by providing `/` as the separator.


Diffs (updated)
-

  3rdparty/stout/include/stout/path.hpp 
27438d31617b3b78bf3d4deffd25c93340610e8d 


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

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


Testing
---

`stout-tests` and `libprocess-tests` passed on Windows and Ubuntu


Thanks,

Andrew Schwartzmeyer

Re: Review Request 67955: Added some new generic flag parsers.

[Benjamin Hindman]

> On July 19, 2018, 6:51 p.m., Kevin Klues wrote:
> > 3rdparty/stout/include/stout/flags/parse.hpp
> > Lines 249-251 (patched)
> > 
> >
> > This comment is wrong. It's not for unsigned ints, it's for strings.

Okay I'll fix it, any reason I can't ship it once I change the comment?


- Benjamin


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


On July 18, 2018, 1:19 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67955/
> ---
> 
> (Updated July 18, 2018, 1:19 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added some new generic flag parsers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/flags/parse.hpp 
> 4566b798b8b66d47779a28cabdea06f588012686 
> 
> 
> Diff: https://reviews.apache.org/r/67955/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 67956: Removed some generic flag parsers that are now in stout.

[Kevin Klues]

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


Ship it!




I assume removing these in this commit seprate from adding them to stout in the 
previous commit didn't cause build errors, for the short time between commits 
when there were duplicate definitions of them?

- Kevin Klues


On July 18, 2018, 1:19 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67956/
> ---
> 
> (Updated July 18, 2018, 1:19 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed some generic flag parsers that are now in stout.
> 
> 
> Diffs
> -
> 
>   src/common/parse.hpp 03814e3112a043a1001764b316b9d49501d33665 
> 
> 
> Diff: https://reviews.apache.org/r/67956/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 67955: Added some new generic flag parsers.

[Kevin Klues]

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




3rdparty/stout/include/stout/flags/parse.hpp
Lines 249-251 (patched)


This comment is wrong. It's not for unsigned ints, it's for strings.


- Kevin Klues


On July 18, 2018, 1:19 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67955/
> ---
> 
> (Updated July 18, 2018, 1:19 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added some new generic flag parsers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/flags/parse.hpp 
> 4566b798b8b66d47779a28cabdea06f588012686 
> 
> 
> Diff: https://reviews.apache.org/r/67955/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 67503: Added support helper for fetching review ids.

[Dragos Schebesch via Review Board]

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

(Updated July 19, 2018, 3:48 p.m.)


Review request for mesos, Andrew Schwartzmeyer and Armand Grillet.


Repository: mesos


Description (updated)
---

Added helper for fetching review id, which is needed to decouple this logic 
from verify-reviews.py. We need this for the windows build so we can trigger a 
downstream job for each review request on another server.


Diffs
-

  support/python3/get-review-ids.py PRE-CREATION 


Diff: https://reviews.apache.org/r/67503/diff/4/


Testing
---

Sample run on this review request id:
```
./get-review-ids.py -r 67503
Dependent review: https://reviews.apache.org/api/review-requests/67502/
67502

67503
```


Thanks,

Dragos Schebesch

Re: Review Request 67502: Refactored ReviewBoard API functionality into separate module.

[Dragos Schebesch via Review Board]

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

(Updated July 19, 2018, 3:48 p.m.)


Review request for mesos, Andrew Schwartzmeyer and Armand Grillet.


Repository: mesos


Description (updated)
---

Refactored API functionality into separate module -- this was done to clean up 
the codebase and put all API calls in single place.


Diffs
-

  support/python3/common.py PRE-CREATION 


Diff: https://reviews.apache.org/r/67502/diff/4/


Testing
---

For example, to call the api on a specific review_url, with some data, we would 
use the following code:

```
ReviewBoardHandler().api(review_url, data)
```


Thanks,

Dragos Schebesch

Re: Review Request 67504: Added support script to post build results.

[Dragos Schebesch via Review Board]

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

(Updated July 19, 2018, 3:39 p.m.)


Review request for mesos, Andrew Schwartzmeyer and Armand Grillet.


Repository: mesos


Description
---

Added support script to post build results.


Diffs
-

  support/python3/post-build-result.py PRE-CREATION 


Diff: https://reviews.apache.org/r/67504/diff/4/


Testing (updated)
---

Sample test run that can be seen on this review request:
```
./python3/post-build-result.py -r 67504 -u  -p  -m 'dummy' -o 
http://dummy.com/artifact
Posting to review request: https://reviews.apache.org/r/67504
dummy

All the build artifacts available at: http://dummy.com/artifact
```


Thanks,

Dragos Schebesch

Re: Review Request 67504: Added support script to post build results.

[Dragos Schebesch via Review Board]

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



dummy

All the build artifacts available at: http://dummy.com/artifact

- Dragos Schebesch


On July 18, 2018, 8:50 a.m., Dragos Schebesch wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67504/
> ---
> 
> (Updated July 18, 2018, 8:50 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Armand Grillet.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support script to post build results.
> 
> 
> Diffs
> -
> 
>   support/python3/post-build-result.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67504/diff/4/
> 
> 
> Testing
> ---
> 
> Tests have been done later in the chain.
> 
> 
> Thanks,
> 
> Dragos Schebesch
> 
>

Re: Review Request 67972: RFC: Added RetentionPolicy for task metadata and sandboxes.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67972 was successfully built and tested.

Reviews applied: `['67972']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1956/mesos-review-67972

- Mesos Reviewbot Windows


On July 19, 2018, 12:59 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67972/
> ---
> 
> (Updated July 19, 2018, 12:59 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6285 and MESOS-7947
> https://issues.apache.org/jira/browse/MESOS-6285
> https://issues.apache.org/jira/browse/MESOS-7947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a protobuf which tells the agent to garbage collect
> more directories than it currently does.  The agent currently garbage
> collects directories at the executor level, which is not ideal for
> certain types of long-lived executors that launch many tasks or
> nested containers over its lifetime.
> 
> Each task launched under the same executor will result in a checkpointed
> TaskInfo in the agent's metadata.  This can result in slow agent
> recovery, as described in MESOS-6285, where an excessive number of tasks
> will actually cause the agent to be OOM-killed.
> 
> For the default executor, each task will be launched as a nested
> container, which will include a sandbox directory (under the executor's
> sandbox). If too many nested containers are launched without removing
> the associated sandboxes, the agent may run out of disk space.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
>   include/mesos/mesos.proto 5a985fca39cdfb7e9b4775650a7e5dbe68c3b8ae 
> 
> 
> Diff: https://reviews.apache.org/r/67972/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 67972: RFC: Added RetentionPolicy for task metadata and sandboxes.

[Joseph Wu]

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

Review request for mesos, Gilbert Song, Qian Zhang, and Vinod Kone.


Bugs: MESOS-6285 and MESOS-7947
https://issues.apache.org/jira/browse/MESOS-6285
https://issues.apache.org/jira/browse/MESOS-7947


Repository: mesos


Description
---

This adds a protobuf which tells the agent to garbage collect
more directories than it currently does.  The agent currently garbage
collects directories at the executor level, which is not ideal for
certain types of long-lived executors that launch many tasks or
nested containers over its lifetime.

Each task launched under the same executor will result in a checkpointed
TaskInfo in the agent's metadata.  This can result in slow agent
recovery, as described in MESOS-6285, where an excessive number of tasks
will actually cause the agent to be OOM-killed.

For the default executor, each task will be launched as a nested
container, which will include a sandbox directory (under the executor's
sandbox). If too many nested containers are launched without removing
the associated sandboxes, the agent may run out of disk space.


Diffs
-

  include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 
  include/mesos/mesos.proto 5a985fca39cdfb7e9b4775650a7e5dbe68c3b8ae 


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


Testing
---


Thanks,

Joseph Wu

Re: Review Request 67896: Added container-specific cgroups mount for freezer & systemd subsystems.

[Qian Zhang]

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

(Updated July 19, 2018, 5:10 p.m.)


Review request for mesos and Gilbert Song.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Added container-specific cgroups mount for freezer & systemd subsystems.


Diffs (updated)
-

  src/slave/containerizer/mesos/constants.hpp 
5be39500828aecad01cbc0e27cb20493048f990e 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
8fba6f9d335cc43a7cd0fcb51ed047ba0e7027ed 
  src/slave/containerizer/mesos/linux_launcher.cpp 
3bddcece7028745cec6623ac33dbfcaced629629 


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

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


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 67896: Added container-specific cgroups mount for freezer & systemd subsystems.

[Qian Zhang]

> On July 19, 2018, 6:46 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 628 (patched)
> > 
> >
> > Should we also use `CGROUP_SEPARATOR` and move it to a common place?

Agree, let me move it to `src/slave/containerizer/mesos/constants.hpp`.


- Qian


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


On July 12, 2018, 10:16 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67896/
> ---
> 
> (Updated July 12, 2018, 10:16 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-9070
> https://issues.apache.org/jira/browse/MESOS-9070
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added container-specific cgroups mount for freezer & systemd subsystems.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 8fba6f9d335cc43a7cd0fcb51ed047ba0e7027ed 
> 
> 
> Diff: https://reviews.apache.org/r/67896/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>