Re: Review Request 65475: Fixed SSL socket shutdown returned errno.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the current review.

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

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65475

Relevant logs:

- 
[apply-review-65475-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65475/logs/apply-review-65475-stdout.log):

```
error: patch failed: 3rdparty/libprocess/src/libevent_ssl_socket.cpp:165
error: 3rdparty/libprocess/src/libevent_ssl_socket.cpp: patch does not apply
```

- Mesos Reviewbot Windows


On Feb. 3, 2018, 4:48 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65475/
> ---
> 
> (Updated Feb. 3, 2018, 4:48 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Andrew Schwartzmeyer, Benjamin 
> Mahler, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 521b0cfbccd3599524b1407ef70880f4538941df 
> 
> 
> Diff: https://reviews.apache.org/r/65475/diff/2/
> 
> 
> Testing
> ---
> 
> make check and internal CI
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 65475: Fixed SSL socket shutdown returned errno.

[Till Toenshoff]

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

(Updated Feb. 3, 2018, 4:48 a.m.)


Review request for .


Repository: mesos


Description
---

see summary.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
521b0cfbccd3599524b1407ef70880f4538941df 


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

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


Testing
---

make check and internal CI


Thanks,

Till Toenshoff

Re: Review Request 65475: Fixed SSL socket shutdown returned errno.

[Till Toenshoff]

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

(Updated Feb. 3, 2018, 4:48 a.m.)


Review request for .


Repository: mesos


Description
---

see summary.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
521b0cfbccd3599524b1407ef70880f4538941df 


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

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


Testing
---

make check and internal CI


Thanks,

Till Toenshoff

Re: Review Request 65499: Fixed the flakiness of the ROOT_ConvertPreExistingVolume test.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the current review.

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

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65499

Relevant logs:

- 
[apply-review-65499-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65499/logs/apply-review-65499-stdout.log):

```
error: patch failed: src/tests/storage_local_resource_provider_tests.cpp:2403
error: src/tests/storage_local_resource_provider_tests.cpp: patch does not apply
```

- Mesos Reviewbot Windows


On Feb. 3, 2018, 1:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65499/
> ---
> 
> (Updated Feb. 3, 2018, 1:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-8474
> https://issues.apache.org/jira/browse/MESOS-8474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This unit test is flaky because the master might send out an offer
> before both offer operations of a previous `ACCEPT` call are finished.
> This is fixed by manipulating the clock such that no offer won't be
> sent out until the master receives status updates for both operations.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ce241617ee555e5a67e552dd30b0e7afee161386 
> 
> 
> Diff: https://reviews.apache.org/r/65499/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> Ran in repetition.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 65499: Fixed the flakiness of the ROOT_ConvertPreExistingVolume test.

[Chun-Hung Hsiao]

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

Review request for mesos, Alexander Rukletsov, Gaston Kleiman, Greg Mann, and 
Jie Yu.


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


Repository: mesos


Description
---

This unit test is flaky because the master might send out an offer
before both offer operations of a previous `ACCEPT` call are finished.
This is fixed by manipulating the clock such that no offer won't be
sent out until the master receives status updates for both operations.


Diffs
-

  src/tests/storage_local_resource_provider_tests.cpp 
ce241617ee555e5a67e552dd30b0e7afee161386 


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


Testing
---

sudo make check
Ran in repetition.


Thanks,

Chun-Hung Hsiao

Re: Review Request 65060: Cleaned up endpoint directories after SLRP tests.

[Chun-Hung Hsiao]

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

(Updated Feb. 3, 2018, 1:37 a.m.)


Review request for mesos, Gaston Kleiman, Greg Mann, and Jie Yu.


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


Repository: mesos


Description
---

Cleaned up endpoint directories after SLRP tests.


Diffs (updated)
-

  src/tests/cluster.cpp 19a41c7c1c303ad806daa4e5e3765a1e0b55933b 


Diff: https://reviews.apache.org/r/65060/diff/5/

Changes: https://reviews.apache.org/r/65060/diff/4-5/


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 65497: Added test to verify task-less executor is shutdown when re-subscribing.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 65496.

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

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65497

Relevant logs:

- 
[apply-review-65496-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65497/logs/apply-review-65496-stdout.log):

```
error: patch failed: src/slave/slave.cpp:4238
error: src/slave/slave.cpp: patch does not apply
```

- Mesos Reviewbot Windows


On Feb. 3, 2018, 1:18 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65497/
> ---
> 
> (Updated Feb. 3, 2018, 1:18 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8411
> https://issues.apache.org/jira/browse/MESOS-8411
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the v1 executor is shutdown if all of its
> initial tasks could not be delivered when re-subscribing with
> a recovered agent. See MESOS-8411.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp a35c68e8645384f6244d17e37cad71373aba6893 
>   src/tests/mesos.cpp d751b2e9c635eb6a5039678de426467176cda908 
>   src/tests/mock_slave.cpp 8357edc7b3a35624c813eccb9ecca9d3b5dbe07c 
>   src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f 
> 
> 
> Diff: https://reviews.apache.org/r/65497/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 65497: Added test to verify task-less executor is shutdown when re-subscribing.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This test verifies that the v1 executor is shutdown if all of its
initial tasks could not be delivered when re-subscribing with
a recovered agent. See MESOS-8411.


Diffs
-

  src/tests/mesos.hpp a35c68e8645384f6244d17e37cad71373aba6893 
  src/tests/mesos.cpp d751b2e9c635eb6a5039678de426467176cda908 
  src/tests/mock_slave.cpp 8357edc7b3a35624c813eccb9ecca9d3b5dbe07c 
  src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f 


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


Testing
---

make check


Thanks,

Meng Zhu

Review Request 65496: Fixed a bug where task-less v1 executor could linger.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

A task-less v1 executor could linger if the agent fails before
any of the executor's initial tasks got delivered.

This patch fixes this issue by checking if an executor is
task-less during the executor `subscribe()` and shutdown
the executor accordingly.


Diffs
-

  src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 


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


Testing
---

make check.
Dedicated test #65497


Thanks,

Meng Zhu

Re: Review Request 65495: Fixed clang compilation problem for the SLRP metrics test.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 3, 2018, 12:56 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65495/
> ---
> 
> (Updated Feb. 3, 2018, 12:56 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed clang compilation problem for the SLRP metrics test.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbacc3f431189c3ca2a0f20323e2368fdef38a59 
> 
> 
> Diff: https://reviews.apache.org/r/65495/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65495: Fixed clang compilation problem for the SLRP metrics test.

[Mesos Reviewbot Windows]

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



FAIL: The file 'D:\DCOS\mesos\build-output\logs\apply-review-65495-stdout.log' 
already exists.

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65495

- Mesos Reviewbot Windows


On Feb. 2, 2018, 4:56 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65495/
> ---
> 
> (Updated Feb. 2, 2018, 4:56 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed clang compilation problem for the SLRP metrics test.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> bbacc3f431189c3ca2a0f20323e2368fdef38a59 
> 
> 
> Diff: https://reviews.apache.org/r/65495/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65474: Updated socket shutdown to return SocketError.

[Andrei Budnik]

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




3rdparty/libprocess/include/process/socket.hpp
Line 192 (original), 193 (patched)


What about returning `Nothing()` for not connected sockets here:
```
if (::shutdown(s, how) < 0 && errno != ENOTCONN) {
```
and in `LibeventSSLSocketImpl::shutdown()`?

It will be 2-liner change (excluding `#ifdef __WINDOWS__` stuff).

We already have such errno check in 
https://github.com/apache/mesos/blob/master/3rdparty/stout/include/stout/os/windows/close.hpp#L40-L43


- Andrei Budnik


On Feb. 2, 2018, 5:50 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65474/
> ---
> 
> (Updated Feb. 2, 2018, 5:50 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8513
> https://issues.apache.org/jira/browse/MESOS-8513
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Socket::shutdown returns SocketError allowing for higher level
> functions to have more control over logging errors where needed.
> Also switches SocketManager::close's shutdown failure logging
> back to ERROR level as we are now able to filter out expected
> failures.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> ae6154d5d142f65352e00f37b4e66d0b62fdc3c2 
>   3rdparty/libprocess/src/http.cpp cc41fa6f671cf029a46722299eded1a27da210d3 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 640fa676ef570f7fcf3f96249662837497a2c76c 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 521b0cfbccd3599524b1407ef70880f4538941df 
>   3rdparty/libprocess/src/process.cpp 
> ba9bc291bb6741e32b3a066fe90771311d21852a 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> b1a0ea614d7483e683ac056cf822ce816221babb 
> 
> 
> Diff: https://reviews.apache.org/r/65474/diff/3/
> 
> 
> Testing
> ---
> 
> make check, visual logging inspection
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 65464: Introduced `mesos-build` along with pre-built docker images.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 65463.

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

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65464

Relevant logs:

- 
[apply-review-65463-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65464/logs/apply-review-65463-stdout.log):

```
error: patch failed: support/jenkins/buildbot.sh:21
error: support/jenkins/buildbot.sh: patch does not apply
```

- Mesos Reviewbot Windows


On Feb. 1, 2018, 7:20 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65464/
> ---
> 
> (Updated Feb. 1, 2018, 7:20 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The layout of `mesos-build` is similar to `mesos-tidy`.
> 
> The `mesos-build.sh` will be the user-facing driver, and `buildbot.sh`
> invokes `mesos-build.sh` along with the clean-up (`docker rmi`) phase.
> The `mesos-build` directory contains docker files and a common
> `entrypoint.sh`. The docker images are built with all of the
> dependencies required, and the various testing configurations are set
> within `entrypoint.sh`.
> 
> 
> Diffs
> -
> 
>   support/jenkins/buildbot.sh dab4b72b645eaca382a70acef4220fa04af96e36 
>   support/mesos-build.sh PRE-CREATION 
>   support/mesos-build/centos-7.dockerfile PRE-CREATION 
>   support/mesos-build/entrypoint.sh PRE-CREATION 
>   support/mesos-build/ubuntu-16.04.dockerfile PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65464/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 65492: Changed the SLRP test fixture.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On Feb. 3, 2018, 12:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65492/
> ---
> 
> (Updated Feb. 3, 2018, 12:08 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used the `ContainerizerTest` fixture instead. This
> guarantees that the cgroup root is different for each test run,
> prevening interferences.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 2761701eebd8ad376042991703b2cfef6d0ec08d 
> 
> 
> Diff: https://reviews.apache.org/r/65492/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 65492: Changed the SLRP test fixture.

[Mesos Reviewbot Windows]

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



FAIL: The file 'D:\DCOS\mesos\build-output\logs\apply-review-65492-stdout.log' 
already exists.

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65492

- Mesos Reviewbot Windows


On Feb. 2, 2018, 4:08 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65492/
> ---
> 
> (Updated Feb. 2, 2018, 4:08 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used the `ContainerizerTest` fixture instead. This
> guarantees that the cgroup root is different for each test run,
> prevening interferences.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 2761701eebd8ad376042991703b2cfef6d0ec08d 
> 
> 
> Diff: https://reviews.apache.org/r/65492/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 65492: Changed the SLRP test fixture.

[Jie Yu]

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

Review request for mesos and Chun-Hung Hsiao.


Repository: mesos


Description
---

Used the `ContainerizerTest` fixture instead. This
guarantees that the cgroup root is different for each test run,
prevening interferences.


Diffs
-

  src/tests/storage_local_resource_provider_tests.cpp 
2761701eebd8ad376042991703b2cfef6d0ec08d 


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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 65461: Provide new overload of 'ProcessBase::route()'.

[Benjamin Mahler]

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




3rdparty/libprocess/include/process/process.hpp
Lines 325-352 (original), 325-352 (patched)


These look obviated by your new one? Why do we still need these?


- Benjamin Mahler


On Feb. 1, 2018, 7:01 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65461/
> ---
> 
> (Updated Feb. 1, 2018, 7:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Provide new overload of 'ProcessBase::route()'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/logging.hpp 
> f9997677d69d54f5723d4fc0a495008d3ce11cc5 
>   3rdparty/libprocess/include/process/process.hpp 
> 8661706cb058efb26f5bfbcc84972f9930d3670f 
>   3rdparty/libprocess/include/process/profiler.hpp 
> 2991dd2033d68802a813de91babb47679c807aa0 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> f79541853b4c826014ee969633345c3d51520ecf 
> 
> 
> Diff: https://reviews.apache.org/r/65461/diff/1/
> 
> 
> Testing
> ---
> 
> It is a common pattern in libprocess to have a single function serving as 
> endpoint that handles both authenticated and non-authenticated requests, 
> providing `None()` as a default parameter in non-authenticated contexts.
> 
> This patch adds an overload to `ProcessBase::route()` implementing that so 
> that it does not need to be re-implemented in each individual process.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 65475: Fixed SSL socket shutdown returned errno.

[Till Toenshoff]

> On Feb. 2, 2018, 6:04 a.m., Benjamin Mahler wrote:
> > Can you include Andy on this review as well? I would love to know what the 
> > suggested approach to this is; if there is some solution in place other 
> > than `#ifdef`s.
> 
> Andrew Schwartzmeyer wrote:
> Depending on how this error code is used later, we could probably do some 
> sort of mapping in 
> [`WindowsSocketError`](https://github.com/apache/mesos/blob/1600ebc6901239ae86e4e133c82d3424c56c978e/3rdparty/stout/include/stout/windows/error.hpp#L124)
>  (the underlying type of `SocketError` on Windows) so you can manually 
> construct it with `ENOTCONN` and its mapped to `WSAENOTCONN` on Windows. 
> Thing is, then we'd also need to make sure comparisons are done the same way. 
> Hm...
> 
> Andrew Schwartzmeyer wrote:
> A simpler approach may just be the latter half of that suggestion. If you 
> compare `WindowsSocketError` to `ENOTCONN`, have the comparator return `true` 
> if its type is either `ENOTCONN` (for a hard-code like this) or `WSAENOTCONN` 
> (from an actual OS error).
> 
> Otherwise, I'd also suggest not returning `ENOTCONN` at all, since this 
> code is being a bit misleading about the state of the error. That is, I think 
> it's expected that `ENOTCONN` is only returned when an OS socket function 
> actually errors with `ENOTCONN`, and this error logic is our own case of 
> having never initialized. We could return our own error instead, that's the 
> same on both Linux and Windows (and BSD etc.).
> 
> Benjamin Mahler wrote:
> I believe ENOTCONN is what will be returned in this case if we were to 
> call shutdown on the fd:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/shutdown.html
> 
> Either we can return this directly and avoid the syscall or we can 
> directly call `::shutdown()` on the fd and return its error (which FWICT must 
> be ENOTCONN). Both sound ok to me, but I would avoid making up a new error 
> code.

Thanks for this exchange guys - much appreciated.

Glad we spoke about it as it is also my opinion that injecting an errno 
manually is a shortcut we can avoid. Instead invoke `::shutdown()` and see what 
it returns - then return `SocketError()` or `Nothing()` accordingly. One less 
assumption and we can entirely avoid nasty `#ifdef`s.


- Till


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


On Feb. 2, 2018, 5:50 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65475/
> ---
> 
> (Updated Feb. 2, 2018, 5:50 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 521b0cfbccd3599524b1407ef70880f4538941df 
> 
> 
> Diff: https://reviews.apache.org/r/65475/diff/1/
> 
> 
> Testing
> ---
> 
> make check and internal CI
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 65447: Refactored couple of launch task sanity checks into a single code path.

[Chun-Hung Hsiao]

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




src/slave/slave.cpp
Line 2033 (original), 2033 (patched)


Most of our member function names start with verbs. Maybe `checkX`? 
Also we cannot tell what kind of invariants are checked from the name.



src/slave/slave.cpp
Lines 2071-2079 (original), 2056-2064 (patched)


Agreed. This confuses me a bit because before going through the code, I 
didn't know why we need to do the same check in `_run` and its continuation.



src/slave/slave.cpp
Lines 2196-2197 (original), 2213-2214 (patched)



http://mesos.apache.org/documentation/latest/c++-style-guide/#function-definition-invocation


- Chun-Hung Hsiao


On Feb. 1, 2018, 2:03 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65447/
> ---
> 
> (Updated Feb. 1, 2018, 2:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initial steps common to `_run()` and `__run()` on the task launch
> code path are refactored.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 09c01ebd1b5e8008ba9e7d412042f1db76a2c5a5 
>   src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 
> 
> 
> Diff: https://reviews.apache.org/r/65447/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65474: Updated socket shutdown to return SocketError.

[Benjamin Mahler]

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


Ship it!





3rdparty/libprocess/src/libevent_ssl_socket.cpp
Line 168 (original), 168 (patched)


Note that here we should probably just directly call shutdown on the fd (in 
your second patch).


- Benjamin Mahler


On Feb. 2, 2018, 5:50 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65474/
> ---
> 
> (Updated Feb. 2, 2018, 5:50 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8513
> https://issues.apache.org/jira/browse/MESOS-8513
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Socket::shutdown returns SocketError allowing for higher level
> functions to have more control over logging errors where needed.
> Also switches SocketManager::close's shutdown failure logging
> back to ERROR level as we are now able to filter out expected
> failures.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> ae6154d5d142f65352e00f37b4e66d0b62fdc3c2 
>   3rdparty/libprocess/src/http.cpp cc41fa6f671cf029a46722299eded1a27da210d3 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 640fa676ef570f7fcf3f96249662837497a2c76c 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 521b0cfbccd3599524b1407ef70880f4538941df 
>   3rdparty/libprocess/src/process.cpp 
> ba9bc291bb6741e32b3a066fe90771311d21852a 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> b1a0ea614d7483e683ac056cf822ce816221babb 
> 
> 
> Diff: https://reviews.apache.org/r/65474/diff/3/
> 
> 
> Testing
> ---
> 
> make check, visual logging inspection
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 65475: Fixed SSL socket shutdown returned errno.

[Benjamin Mahler]

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


Ship it!




Per discussion with Andy, I'm ok with either returning ENOTCONN directly, or 
alternatively you can call `::shutdown` in this case and send out a 
`SocketError` (if this produces something other than ENOTCONN then we've 
misunderstood!).

- Benjamin Mahler


On Feb. 2, 2018, 5:50 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65475/
> ---
> 
> (Updated Feb. 2, 2018, 5:50 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 521b0cfbccd3599524b1407ef70880f4538941df 
> 
> 
> Diff: https://reviews.apache.org/r/65475/diff/1/
> 
> 
> Testing
> ---
> 
> make check and internal CI
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 65475: Fixed SSL socket shutdown returned errno.

[Benjamin Mahler]

> On Feb. 2, 2018, 6:04 a.m., Benjamin Mahler wrote:
> > Can you include Andy on this review as well? I would love to know what the 
> > suggested approach to this is; if there is some solution in place other 
> > than `#ifdef`s.
> 
> Andrew Schwartzmeyer wrote:
> Depending on how this error code is used later, we could probably do some 
> sort of mapping in 
> [`WindowsSocketError`](https://github.com/apache/mesos/blob/1600ebc6901239ae86e4e133c82d3424c56c978e/3rdparty/stout/include/stout/windows/error.hpp#L124)
>  (the underlying type of `SocketError` on Windows) so you can manually 
> construct it with `ENOTCONN` and its mapped to `WSAENOTCONN` on Windows. 
> Thing is, then we'd also need to make sure comparisons are done the same way. 
> Hm...
> 
> Andrew Schwartzmeyer wrote:
> A simpler approach may just be the latter half of that suggestion. If you 
> compare `WindowsSocketError` to `ENOTCONN`, have the comparator return `true` 
> if its type is either `ENOTCONN` (for a hard-code like this) or `WSAENOTCONN` 
> (from an actual OS error).
> 
> Otherwise, I'd also suggest not returning `ENOTCONN` at all, since this 
> code is being a bit misleading about the state of the error. That is, I think 
> it's expected that `ENOTCONN` is only returned when an OS socket function 
> actually errors with `ENOTCONN`, and this error logic is our own case of 
> having never initialized. We could return our own error instead, that's the 
> same on both Linux and Windows (and BSD etc.).

I believe ENOTCONN is what will be returned in this case if we were to call 
shutdown on the fd:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/shutdown.html

Either we can return this directly and avoid the syscall or we can directly 
call `::shutdown()` on the fd and return its error (which FWICT must be 
ENOTCONN). Both sound ok to me, but I would avoid making up a new error code.


- Benjamin


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


On Feb. 2, 2018, 5:50 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65475/
> ---
> 
> (Updated Feb. 2, 2018, 5:50 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 521b0cfbccd3599524b1407ef70880f4538941df 
> 
> 
> Diff: https://reviews.apache.org/r/65475/diff/1/
> 
> 
> Testing
> ---
> 
> make check and internal CI
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 65449: Fixed an issue where executor info linger on master if failed to launch.

[Vinod Kone]

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



partial review.


src/master/master.hpp
Lines 867-871 (patched)


s/toLaunchExecutor/launchExecutor/

More importnatly, we don't typically use this pattern of passing a pointer 
and changing its value for `int` like fields. See below.



src/master/master.cpp
Line 4914 (original), 4918 (patched)


How about we figure whether to launch an executor or not here and then call 
`addTask`?



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


This comes out as `0 executor launch` or `1 executor launch` which reads 
weird.

how about:

```
LOG(INFO) << "Launching task " << task.task_id() << " of framework "
  << *framework << " with resources " << task.resources()
  << " on agent " << *slave << " on "
  << (message.launch_executor() ? " new executor" << " existing 
executor");
```



src/master/master.cpp
Line 5143 (original), 5151-5160 (patched)


ditto. see above.

do the check for executor launch outside the for loop.



src/master/master.cpp
Lines 5177-5179 (patched)


see above. should be set outside the loop.



src/master/master.cpp
Lines 5193-5194 (patched)


see above.



src/slave/slave.hpp
Lines 170 (patched)


s/toLaunchExecutor/launchExecutor/



src/slave/slave.hpp
Lines 180 (patched)


ditto. here and below.



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


I think old masters didn't used to set this. If 1.x masters do it then we 
can do a CHECK.



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


you can put fraemworkInfo in the previous line?



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


do you need the temporary variable?


- Vinod Kone


On Feb. 1, 2018, 2:03 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65449/
> ---
> 
> (Updated Feb. 1, 2018, 2:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
> 
> 
> Bugs: MESOS-1720
> https://issues.apache.org/jira/browse/MESOS-1720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master relies on `ExitedExecutorMessage` from the agent to recycle
> executor entry. However, this message won't be sent if the executor
> never actually launched (due to transient error), leaving executor
> info on the master lingering and resource claimed.
> See MESOS-1720.
> 
> This patch fixes this issue by sending the `ExitedExecutorMessage`
> from the agent if the executor is never launched. And by
> setting a new field `launch_executor` in the RunTask(Group)Message,
> the master is able to control the executor creation on the agent.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 
>   src/master/master.cpp b97ebae6ebfd8ae0f73e617d0c55e140b9c3fce7 
>   src/slave/slave.hpp 09c01ebd1b5e8008ba9e7d412042f1db76a2c5a5 
>   src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 
>   src/tests/mock_slave.hpp 29ce7140501888d95d5f2d6c26b752ad276b484a 
>   src/tests/mock_slave.cpp 8357edc7b3a35624c813eccb9ecca9d3b5dbe07c 
>   src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f 
> 
> 
> Diff: https://reviews.apache.org/r/65449/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> Dedicated test in #65448
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65482: Fixed allocator bookkeeping of pending operations on master failover.

[Greg Mann]

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




src/master/master.cpp
Lines 7594-7597 (original), 7609-7612 (patched)


Is this function now only called with resources from already-existing 
frameworks? Does that mean that we can update the code here? 
https://github.com/apache/mesos/blob/5a1433576eca20f15f1ea309fc202f4bbaf3b6c7/src/master/allocator/mesos/hierarchical.cpp#L690-L703


- Greg Mann


On Feb. 2, 2018, 2:07 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65482/
> ---
> 
> (Updated Feb. 2, 2018, 2:07 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8536
> https://issues.apache.org/jira/browse/MESOS-8536
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes a bug where pending operations on a resource provider
> resources where not properly accounted for in the allocator. This lead
> to assertion failures when the operation became terminal and we
> attempted to recover the used resources.
> 
> Since framework information is only remembered on agents if the
> framework launched a task, there exists the possibility that a master
> learns about an allocation to a framework unknown to it, yet. To
> accommodate that do not bookkeep allocations to unknown frameworks in
> the allocator and update code handling of terminal operation updates
> accordingly.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f33ff767dcb93556beb696c96f8cfc17baccb05e 
>   src/master/master.cpp cc2685a6bc14103c639ce776cf1c912361e93381 
> 
> 
> Diff: https://reviews.apache.org/r/65482/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`, also tested with a version of the test added in r/65045 which 
> triggered this issue.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65482: Fixed allocator bookkeeping of pending operations on master failover.

[Greg Mann]

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




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


Does this mean we will not correctly account for resources when a framework 
has performed operations on an agent, but has not launched tasks there?


- Greg Mann


On Feb. 2, 2018, 2:07 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65482/
> ---
> 
> (Updated Feb. 2, 2018, 2:07 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8536
> https://issues.apache.org/jira/browse/MESOS-8536
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes a bug where pending operations on a resource provider
> resources where not properly accounted for in the allocator. This lead
> to assertion failures when the operation became terminal and we
> attempted to recover the used resources.
> 
> Since framework information is only remembered on agents if the
> framework launched a task, there exists the possibility that a master
> learns about an allocation to a framework unknown to it, yet. To
> accommodate that do not bookkeep allocations to unknown frameworks in
> the allocator and update code handling of terminal operation updates
> accordingly.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f33ff767dcb93556beb696c96f8cfc17baccb05e 
>   src/master/master.cpp cc2685a6bc14103c639ce776cf1c912361e93381 
> 
> 
> Diff: https://reviews.apache.org/r/65482/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`, also tested with a version of the test added in r/65045 which 
> triggered this issue.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65491: Added metrics for CSI plugin terminations.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





src/resource_provider/storage/provider.cpp
Lines 487 (patched)


Do we need to keep the prefix?



src/resource_provider/storage/provider.cpp
Lines 1828 (patched)


One line apart.



src/tests/storage_local_resource_provider_tests.cpp
Lines 2551-2559 (patched)


No need to set up the master flags since we don't create a framework 
waiting for offers in this test.


- Chun-Hung Hsiao


On Feb. 2, 2018, 10:40 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65491/
> ---
> 
> (Updated Feb. 2, 2018, 10:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Greg Mann.
> 
> 
> Bugs: MESOS-8539
> https://issues.apache.org/jira/browse/MESOS-8539
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added metrics for CSI plugin terminations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 163ce7f9bcc740525ad185d854c9bacd5e0937de 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 85eaef877cb3d62aed0eb2bd5adfba8007c3ee76 
> 
> 
> Diff: https://reviews.apache.org/r/65491/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 65491: Added metrics for CSI plugin terminations.

[Mesos Reviewbot Windows]

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



FAIL: The file 'D:\DCOS\mesos\build-output\logs\apply-review-65491-stdout.log' 
already exists.

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65491

- Mesos Reviewbot Windows


On Feb. 2, 2018, 10:40 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65491/
> ---
> 
> (Updated Feb. 2, 2018, 10:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Greg Mann.
> 
> 
> Bugs: MESOS-8539
> https://issues.apache.org/jira/browse/MESOS-8539
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added metrics for CSI plugin terminations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 163ce7f9bcc740525ad185d854c9bacd5e0937de 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 85eaef877cb3d62aed0eb2bd5adfba8007c3ee76 
> 
> 
> Diff: https://reviews.apache.org/r/65491/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 65491: Added metrics for CSI plugin terminations.

[Jie Yu]

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

Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Greg Mann.


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


Repository: mesos


Description
---

Added metrics for CSI plugin terminations.


Diffs
-

  src/resource_provider/storage/provider.cpp 
163ce7f9bcc740525ad185d854c9bacd5e0937de 
  src/tests/storage_local_resource_provider_tests.cpp 
85eaef877cb3d62aed0eb2bd5adfba8007c3ee76 


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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 65437: Added documentation for fault domains.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the current review.

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

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65437

Relevant logs:

- 
[apply-review-65437-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65437/logs/apply-review-65437-stdout.log):

```
error: patch failed: docs/configuration/master-and-agent.md:95
error: docs/configuration/master-and-agent.md: patch does not apply
error: docs/fault-domains.md: already exists in index
error: patch failed: docs/home.md:26
error: docs/home.md: patch does not apply
```

- Mesos Reviewbot Windows


On Feb. 2, 2018, 7:30 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65437/
> ---
> 
> (Updated Feb. 2, 2018, 7:30 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8291
> https://issues.apache.org/jira/browse/MESOS-8291
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fault domains are a new feature in 1.5 which did not yet have
> a corresponding description in the documentation.
> 
> 
> Diffs
> -
> 
>   docs/configuration/master-and-agent.md 
> f247498ead43a16bbef5afb49d453073dd9ab6ef 
>   docs/fault-domains.md PRE-CREATION 
>   docs/home.md f5b65cc7895b10181e1b8483e3ee9da596d00fd6 
> 
> 
> Diff: https://reviews.apache.org/r/65437/diff/3/
> 
> 
> Testing
> ---
> 
> No.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 65475: Fixed SSL socket shutdown returned errno.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the current review.

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

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65475

Relevant logs:

- 
[apply-review-65475-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65475/logs/apply-review-65475-stdout.log):

```
error: patch failed: 3rdparty/libprocess/src/libevent_ssl_socket.cpp:165
error: 3rdparty/libprocess/src/libevent_ssl_socket.cpp: patch does not apply
```

- Mesos Reviewbot Windows


On Feb. 2, 2018, 5:50 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65475/
> ---
> 
> (Updated Feb. 2, 2018, 5:50 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 521b0cfbccd3599524b1407ef70880f4538941df 
> 
> 
> Diff: https://reviews.apache.org/r/65475/diff/1/
> 
> 
> Testing
> ---
> 
> make check and internal CI
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 65045: Tested correct operation handling during master failover.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 65043.

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

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65045

Relevant logs:

- 
[apply-review-65043-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65045/logs/apply-review-65043-stdout.log):

```
error: patch failed: src/common/protobuf_utils.cpp:1275
error: src/common/protobuf_utils.cpp: patch does not apply
error: patch failed: src/master/http.cpp:2593
error: src/master/http.cpp: patch does not apply
```

- Mesos Reviewbot Windows


On Feb. 2, 2018, 5:30 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65045/
> ---
> 
> (Updated Feb. 2, 2018, 5:30 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8424
> https://issues.apache.org/jira/browse/MESOS-8424
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tested correct operation handling during master failover.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 7112bb4efcc312dc4f68bdc44fb685c7624002b1 
> 
> 
> Diff: https://reviews.apache.org/r/65045/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> This test may trigger MESOS-8524, we might want to commit this as a disabled 
> test.
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 65482: Fixed allocator bookkeeping of pending operations on master failover.

[Mesos Reviewbot Windows]

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



FAIL: The file 'D:\DCOS\mesos\build-output\logs\apply-review-65482-stdout.log' 
already exists.

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65482

- Mesos Reviewbot Windows


On Feb. 2, 2018, 6:07 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65482/
> ---
> 
> (Updated Feb. 2, 2018, 6:07 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8536
> https://issues.apache.org/jira/browse/MESOS-8536
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes a bug where pending operations on a resource provider
> resources where not properly accounted for in the allocator. This lead
> to assertion failures when the operation became terminal and we
> attempted to recover the used resources.
> 
> Since framework information is only remembered on agents if the
> framework launched a task, there exists the possibility that a master
> learns about an allocation to a framework unknown to it, yet. To
> accommodate that do not bookkeep allocations to unknown frameworks in
> the allocator and update code handling of terminal operation updates
> accordingly.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> f33ff767dcb93556beb696c96f8cfc17baccb05e 
>   src/master/master.cpp cc2685a6bc14103c639ce776cf1c912361e93381 
> 
> 
> Diff: https://reviews.apache.org/r/65482/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`, also tested with a version of the test added in r/65045 which 
> triggered this issue.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 65437: Added documentation for fault domains.

[Vinod Kone]

> On Feb. 2, 2018, 10:11 p.m., Vinod Kone wrote:
> >

Fixed the issues before committing.


- Vinod


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


On Feb. 2, 2018, 7:30 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65437/
> ---
> 
> (Updated Feb. 2, 2018, 7:30 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8483
> https://issues.apache.org/jira/browse/MESOS-8483
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fault domains are a new feature in 1.5 which did not yet have
> a corresponding description in the documentation.
> 
> 
> Diffs
> -
> 
>   docs/configuration/master-and-agent.md 
> f247498ead43a16bbef5afb49d453073dd9ab6ef 
>   docs/fault-domains.md PRE-CREATION 
>   docs/home.md f5b65cc7895b10181e1b8483e3ee9da596d00fd6 
> 
> 
> Diff: https://reviews.apache.org/r/65437/diff/3/
> 
> 
> Testing
> ---
> 
> No.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64604: Windows: Updated heath-checks.md with Windows implementation.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Jan. 4, 2018, 4:32 p.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64604/
> ---
> 
> (Updated Jan. 4, 2018, 4:32 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, and Michael Park.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated health-checks.md with details of the Windows health check
> implementations for the mesos and docker executors.
> 
> 
> Diffs
> -
> 
>   docs/health-checks.md 119d149f29f2f2d3178da6ef63a7ce97a4dbc952 
> 
> 
> Diff: https://reviews.apache.org/r/64604/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 64387: Windows: Ported docker health check tests.

[Andrew Schwartzmeyer]

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




src/tests/environment.cpp
Lines 350-356 (patched)


Could this be const-qualified if it were brace-initialized?



src/tests/environment.cpp
Lines 364-365 (patched)


This is going to error if `!status.isSome()`, as then `status.get()` (in 
the stringify) is invalid.



src/tests/environment.cpp
Lines 370 (patched)


Will there be a log message before we await so it doesn't look like we're 
hung for 30 seconds?



src/tests/environment.cpp
Lines 382-390 (patched)


Nit: `const int result`



src/tests/environment.cpp
Lines 388-389 (patched)


Is this the only possible way it could error? Is there a way we could run 
this and keep the `stderr` output?



src/tests/environment.cpp
Lines 402 (patched)


Just a heads up, Jame's chain https://reviews.apache.org/r/65202/ is 
replace `Seconds(15)` with a flag-configured variable.



src/tests/environment.cpp
Lines 412-413 (patched)


Is it not possible to run them with `--rm`? I don't like possibly leaving 
stale containers around if we crash or are interrupted or something.



src/tests/health_check_tests.cpp
Lines 96-99 (original), 93-96 (patched)


While we're in here, I guess I never fixed this, but we should have 
`-NoProfile` for sure on this.



src/tests/health_check_tests.cpp
Lines 973 (patched)


Should the value be `foo` to match the other command?



src/tests/health_check_tests.cpp
Lines 2283-2284 (patched)


Oof... We might want a test filter for the tests that can wait up to an 
hour, or fill a machine's disk. I'm thinking slow/small CI machines are going 
to run into a problem.



src/tests/health_check_tests.cpp
Lines 2404 (patched)


Is this something we should put into a constant at the top? I think it's 
liable to change in the future as Server Core is updated.



src/tests/health_check_tests.cpp
Lines 2544-2547 (original), 2663-2669 (patched)


This whole block keeps getting repeated. I twas repeated originally, but 
maybe now is the time to refactor ;)



src/tests/mesos.hpp
Lines 2173-2174 (patched)


It seems that this a feature Windows will support in the future; should we 
have an issue and TODO to track it?


- Andrew Schwartzmeyer


On Jan. 30, 2018, 2:21 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64387/
> ---
> 
> (Updated Jan. 30, 2018, 2:21 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, Jie Yu, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `HealthCheckTest.ROOT_DOCKER_*` and
> `DockerContainerizerHealthCheckTest.*` tests now work on Windows.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.cpp 13a4c9514fcd3016fe623c597decd067457e86cd 
>   src/tests/health_check_tests.cpp 1893c85169f5e94e164434b93e6a24268224225d 
>   src/tests/mesos.hpp a35c68e8645384f6244d17e37cad71373aba6893 
> 
> 
> Diff: https://reviews.apache.org/r/64387/diff/10/
> 
> 
> Testing
> ---
> 
> Windows Server:
> [==] Running 5 tests from 2 test cases.
> [--] Global test environment set-up.
> [--] 2 tests from HealthCheckTest
> [ RUN  ] HealthCheckTest.ROOT_DOCKER_DockerHealthyTask
> [   OK ] HealthCheckTest.ROOT_DOCKER_DockerHealthyTask (21263 ms)
> [ RUN  ] HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
> [   OK ] HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange (23512 ms)
> [--] 2 tests from HealthCheckTest (44835 ms total)
> 
> [--] 3 tests from NetworkProtocol/DockerContainerizerHealthCheckTest
> [ RUN  ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTP/0
> [   OK ] 
> NetworkProtocol/DockerContainerizerHealthCheckTest.ROOT_DOCKER_USERNETWORK_NETNAMESPACE_HealthyTaskViaHTTP/0
>  (28487 ms)
> [ RUN  ] 
> 

Re: Review Request 65437: Added documentation for fault domains.

[Vinod Kone]

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


Fix it, then Ship it!





docs/fault-domains.md
Line 74 (original), 79 (patched)


s/London/San Francisco/ ?



docs/fault-domains.md
Line 84 (original), 89 (patched)


s/companies/company's/


- Vinod Kone


On Feb. 2, 2018, 7:30 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65437/
> ---
> 
> (Updated Feb. 2, 2018, 7:30 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8483
> https://issues.apache.org/jira/browse/MESOS-8483
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fault domains are a new feature in 1.5 which did not yet have
> a corresponding description in the documentation.
> 
> 
> Diffs
> -
> 
>   docs/configuration/master-and-agent.md 
> f247498ead43a16bbef5afb49d453073dd9ab6ef 
>   docs/fault-domains.md PRE-CREATION 
>   docs/home.md f5b65cc7895b10181e1b8483e3ee9da596d00fd6 
> 
> 
> Diff: https://reviews.apache.org/r/65437/diff/3/
> 
> 
> Testing
> ---
> 
> No.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 65127: Windows: Enabled docker health checks.

[Andrew Schwartzmeyer]

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




src/checks/checker_process.hpp
Lines 45 (patched)


Nit: s/newere/newer



src/checks/checker_process.hpp
Lines 47 (patched)


Should we tag this so a push to their image doesn't break us?



src/checks/checker_process.cpp
Lines 1144-1151 (patched)


I still feel like there's a better way to output this status code.

From command prompt:
```
C:\Users\andrew>pwsh -c (Invoke-WebRequest -Uri google.com -UseBasicParsing 
-SkipCertificateCheck).StatusCode
200

C:\Users\andrew>
```

What's the problem with the output number?


- Andrew Schwartzmeyer


On Jan. 30, 2018, 2:18 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65127/
> ---
> 
> (Updated Jan. 30, 2018, 2:18 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Andrew Schwartzmeyer.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network health checks called curl and then executed setns to
> enter to container network namespace, which did not work on Windows.
> To do the equivalent, Windows nows calls docker run with powershell's
> curl equivalent (Invoke-WebRequest) and uses the
> network=container: flag to enter the container's namespace. The
> command health check was trivially fixed by replacing the hardcoded
> `sh -c`.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65127/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65419: Fixed quoting issues in docker executor command checks.

[Andrew Schwartzmeyer]

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




src/checks/checker_process.cpp
Lines 523-525 (original), 523-526 (patched)


I knew it looked funny.



src/checks/checker_process.cpp
Line 527 (original), 528-535 (patched)


I'm not quite following this. Why do we always push `command.value()` then?



src/checks/checker_process.cpp
Lines 535-537 (original), 543-547 (patched)


Can you explain why this was changed a little more? Maybe an example would 
help.


- Andrew Schwartzmeyer


On Jan. 30, 2018, 2:12 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65419/
> ---
> 
> (Updated Jan. 30, 2018, 2:12 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston 
> Kleiman, and Joseph Wu.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The docker executor was wrapping the docker exec command around with
> `sh -c ""`, which was causing quoting issues, especially on Windows.
> Now, the comamnd health check simply runs `docker exec` without any
> wrapping.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
> 
> 
> Diff: https://reviews.apache.org/r/65419/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65396: Moved docker command check code inside health check library.

[Andrew Schwartzmeyer]

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




src/checks/checker_process.cpp
Lines 523-525 (patched)


It's kind of funny they're quoting this. Not something to fix here, but 
it's weird to have hard-coded `sh -c`.

Joe and I have talked about making the shell switchable (so you can switch 
between cmd.exe and PowerShell on Windows, or sh and bash etc. on Linux). Maybe 
we should file an issue so can start tracking hard-coded shells with a `TODO`?



src/checks/checker_process.cpp
Lines 529 (patched)


Is `command.arguments()` iterable? If so, we could use the vector's reange 
constructor.


- Andrew Schwartzmeyer


On Jan. 29, 2018, 10:21 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65396/
> ---
> 
> (Updated Jan. 29, 2018, 10:21 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and 
> Gaston Kleiman.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now with the health check library refactor, the wrapping of `docker
> exec` for docker command health checks can be easily moved inside
> the library.
> 
> 
> Diffs
> -
> 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
> 
> 
> Diff: https://reviews.apache.org/r/65396/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65395: Refactored health checks to cleanly separate each different check.

[Andrew Schwartzmeyer]

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




src/checks/checker.hpp
Lines 92-93 (original), 62-63 (patched)


Omg were we using Doxygen once upon a time?



src/checks/checker.hpp
Lines 123-124 (original), 90-91 (patched)


Oh yeah, that's way cleaner.



src/checks/checker.cpp
Lines 127 (patched)


Nit: Unfortunately, while I am a big fan of `auto`, the Mesos rule on this 
is to use it only when the type is immediately specified on the 
right-hand-side. In this case, the type is deduced from the return type of a 
function elsewhere in the file, so it should be specified in full on the 
left-hand-side here.



src/checks/checker_process.hpp
Lines 43-45 (original), 47-49 (patched)


We probably need to relocate this comment as we no longer pass the scheme 
here.



src/checks/checker_process.hpp
Lines 143-148 (original)


Leaving a note to make sure this comment was kept around.



src/checks/checker_process.cpp
Lines 241-245 (patched)


I checked this against the visitors below, it checks out.



src/checks/checker_process.cpp
Lines 253 (patched)


s/E,F/E, F/g (See, I read the whole comment block!)

Hm, I am left wondering what the `/-` means for the `Nested` row. I assume 
that it means "not a thing on Windows", but we should probably exlicitly state 
it.



src/checks/checker_process.cpp
Lines 258-271 (patched)


I'm not generally a fan of capture-automatically semantics. I'm left 
wondering what exactly we're capturing by copy here, if anything (`this` 
maybe?).



src/checks/checker_process.cpp
Lines 415 (patched)


Is this even reachable?



src/checks/checker_process.cpp
Lines 435 (patched)


Ditto above (sorry). Though that type is awful enough, perhaps we want to 
make an exception.



src/checks/checker_process.cpp
Lines 450-451 (original), 510-511 (patched)


;)



src/checks/checker_process.cpp
Lines 807 (patched)


IIRC this is a struct, should this be a const-ref?



src/checks/checker_process.cpp
Lines 824-825 (original), 908-909 (patched)


Not something to fix in this chain, but dang we need a proper URI 
construction class.



src/checks/health_checker.cpp
Lines 187-214 (patched)


Where else did I read this code...



src/docker/executor.cpp
Lines 658-664 (patched)


Nit: Style-wise, can we brace-construct this whole struct? (Should we? 
Could it become const-qualified if we did?)


- Andrew Schwartzmeyer


On Jan. 30, 2018, 2:06 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65395/
> ---
> 
> (Updated Jan. 30, 2018, 2:06 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and 
> Gaston Kleiman.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored health check code to separate the logic for each check
> type and runtime (Plain/Docker/Nested). Now the matrix of different
> possiblilites is cleanly enumerated, making it easier to add
> future health checks, such as the ones for Windows.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 
>   src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d 
>   src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 
>   src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f 
>   src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 
>   src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
>   src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea 
>   src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f 
> 
> 
> Diff: https://reviews.apache.org/r/65395/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> 

Re: Review Request 65394: Added separate structs for health check runtime and check types.

[Andrew Schwartzmeyer]

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




src/checks/checks_runtime.hpp
Lines 32-66 (patched)


Overall, I like these abstractions. I think Plain, Docker, and Nested are 
all immediately understandable.

Nit: indentation (just run clang-format on the file please :).



src/checks/checks_types.hpp
Lines 31-77 (patched)


Again, I like these abstractions. This is going to clean up the health 
check code significantly.

Nit: clang-format!


- Andrew Schwartzmeyer


On Jan. 30, 2018, 2:05 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65394/
> ---
> 
> (Updated Jan. 30, 2018, 2:05 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, and 
> Gaston Kleiman.
> 
> 
> Bugs: MESOS-8498
> https://issues.apache.org/jira/browse/MESOS-8498
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is in preparation of another patch that will refactor the
> health check code using these types.
> 
> 
> Diffs
> -
> 
>   src/checks/checks_runtime.hpp PRE-CREATION 
>   src/checks/checks_types.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65394/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 65467: Windows: Added `internal::windows::enable_inherit(WindowsFD)`.

[Andrew Schwartzmeyer]

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

(Updated Feb. 2, 2018, 12:13 p.m.)


Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


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


Repository: mesos


Description
---

This function is used on Windows to explicitly enable inheritance on a
handle.


Diffs
-

  3rdparty/stout/include/stout/internal/windows/inherit.hpp PRE-CREATION 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65469: Windows: Made `IO::OWNED` file descriptors inheritable.

[Andrew Schwartzmeyer]

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

(Updated Feb. 2, 2018, 12:12 p.m.)


Review request for mesos, Akash Gupta, Jie Yu, and Joseph Wu.


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


Repository: mesos


Description
---

On Windows, we make all file handles non-inheritable by default.
Therefore when creating a `Subprocess::FD` for a subprocess to inherit a
file descriptor, we have to explicitly make it inheritable. This was
already happening as a side-effect of `os::dup` for `IO::DUPLICATED`
file descriptors, but not for `IO::OWNED`. Both, however, are meant to
be inherited.


Diffs
-

  3rdparty/libprocess/src/subprocess.cpp 
785e2e1083d115d25fffde2df74ed8bc1726511c 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 65437: Added documentation for fault domains.

[Vinod Kone]

> On Feb. 1, 2018, 10:18 p.m., Vinod Kone wrote:
> > docs/fault-domains.md
> > Lines 63 (patched)
> > 
> >
> > s/The default/By default, the/
> 
> Benno Evers wrote:
> Are you sure about this? It would imply to me as a reader that this 
> behaviour can be changed.

I see. Yea, I'm not very sure then, what you have is probably less confusing.


> On Feb. 1, 2018, 10:18 p.m., Vinod Kone wrote:
> > docs/fault-domains.md
> > Lines 80 (patched)
> > 
> >
> > Non-region-aware frameworks will only receive offers from the primary 
> > region (region containing masters). They won't get offers from other 
> > regions.
> 
> Benno Evers wrote:
> Does this actually imply that users should upgrade all their frameworks 
> to be partition-aware before configuring masters and agents with fault 
> domains? In this example, it would be quite devastating if two out of three 
> datacenters suddenly went completely unused.

s/primary/local/ in my first comment.

Do you mean REGION aware and not PARTION aware? So, yes, frameworks need to 
register with REGION_AWARE capability if they want remote region offers. The 
rationale was that most frameworks want their workloads in the local region and 
not magically go to remote regions with potentially higher latencies without an 
explicit opt-in. Note that region-aware frameworks should ideally expose the 
remote launching capability to their users too (e.g., via a configuration 
option in the app definition) before they start registering with REGION_AWARE 
capability.


- Vinod


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


On Feb. 2, 2018, 7:30 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65437/
> ---
> 
> (Updated Feb. 2, 2018, 7:30 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8483
> https://issues.apache.org/jira/browse/MESOS-8483
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fault domains are a new feature in 1.5 which did not yet have
> a corresponding description in the documentation.
> 
> 
> Diffs
> -
> 
>   docs/configuration/master-and-agent.md 
> f247498ead43a16bbef5afb49d453073dd9ab6ef 
>   docs/fault-domains.md PRE-CREATION 
>   docs/home.md f5b65cc7895b10181e1b8483e3ee9da596d00fd6 
> 
> 
> Diff: https://reviews.apache.org/r/65437/diff/3/
> 
> 
> Testing
> ---
> 
> No.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 65437: Added documentation for fault domains.

[Benno Evers]

> On Feb. 1, 2018, 10:18 p.m., Vinod Kone wrote:
> > docs/fault-domains.md
> > Lines 63 (patched)
> > 
> >
> > s/The default/By default, the/

Are you sure about this? It would imply to me as a reader that this behaviour 
can be changed.


> On Feb. 1, 2018, 10:18 p.m., Vinod Kone wrote:
> > docs/fault-domains.md
> > Lines 80 (patched)
> > 
> >
> > Non-region-aware frameworks will only receive offers from the primary 
> > region (region containing masters). They won't get offers from other 
> > regions.

Does this actually imply that users should upgrade all their frameworks to be 
partition-aware before configuring masters and agents with fault domains? In 
this example, it would be quite devastating if two out of three datacenters 
suddenly went completely unused.


- Benno


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


On Feb. 2, 2018, 7:30 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65437/
> ---
> 
> (Updated Feb. 2, 2018, 7:30 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8483
> https://issues.apache.org/jira/browse/MESOS-8483
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fault domains are a new feature in 1.5 which did not yet have
> a corresponding description in the documentation.
> 
> 
> Diffs
> -
> 
>   docs/configuration/master-and-agent.md 
> f247498ead43a16bbef5afb49d453073dd9ab6ef 
>   docs/fault-domains.md PRE-CREATION 
>   docs/home.md f5b65cc7895b10181e1b8483e3ee9da596d00fd6 
> 
> 
> Diff: https://reviews.apache.org/r/65437/diff/3/
> 
> 
> Testing
> ---
> 
> No.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 65437: Added documentation for fault domains.

[Benno Evers]

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

(Updated Feb. 2, 2018, 7:30 p.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Fault domains are a new feature in 1.5 which did not yet have
a corresponding description in the documentation.


Diffs (updated)
-

  docs/configuration/master-and-agent.md 
f247498ead43a16bbef5afb49d453073dd9ab6ef 
  docs/fault-domains.md PRE-CREATION 
  docs/home.md f5b65cc7895b10181e1b8483e3ee9da596d00fd6 


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

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


Testing
---

No.


Thanks,

Benno Evers

Re: Review Request 65475: Fixed SSL socket shutdown returned errno.

[Andrew Schwartzmeyer]

> On Feb. 1, 2018, 10:04 p.m., Benjamin Mahler wrote:
> > Can you include Andy on this review as well? I would love to know what the 
> > suggested approach to this is; if there is some solution in place other 
> > than `#ifdef`s.
> 
> Andrew Schwartzmeyer wrote:
> Depending on how this error code is used later, we could probably do some 
> sort of mapping in 
> [`WindowsSocketError`](https://github.com/apache/mesos/blob/1600ebc6901239ae86e4e133c82d3424c56c978e/3rdparty/stout/include/stout/windows/error.hpp#L124)
>  (the underlying type of `SocketError` on Windows) so you can manually 
> construct it with `ENOTCONN` and its mapped to `WSAENOTCONN` on Windows. 
> Thing is, then we'd also need to make sure comparisons are done the same way. 
> Hm...

A simpler approach may just be the latter half of that suggestion. If you 
compare `WindowsSocketError` to `ENOTCONN`, have the comparator return `true` 
if its type is either `ENOTCONN` (for a hard-code like this) or `WSAENOTCONN` 
(from an actual OS error).

Otherwise, I'd also suggest not returning `ENOTCONN` at all, since this code is 
being a bit misleading about the state of the error. That is, I think it's 
expected that `ENOTCONN` is only returned when an OS socket function actually 
errors with `ENOTCONN`, and this error logic is our own case of having never 
initialized. We could return our own error instead, that's the same on both 
Linux and Windows (and BSD etc.).


- Andrew


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


On Feb. 1, 2018, 9:50 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65475/
> ---
> 
> (Updated Feb. 1, 2018, 9:50 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, and Joseph 
> Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 521b0cfbccd3599524b1407ef70880f4538941df 
> 
> 
> Diff: https://reviews.apache.org/r/65475/diff/1/
> 
> 
> Testing
> ---
> 
> make check and internal CI
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 64970: Use tox for linting and testing code living uder src/python.

[Eric Chung]

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




src/python/cli_new/tox.ini
Lines 21 (patched)


fix --cov



support/mesos-style.py
Lines 381 (patched)


consider using __file__ instead


- Eric Chung


On Jan. 19, 2018, 4:29 a.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64970/
> ---
> 
> (Updated Jan. 19, 2018, 4:29 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use tox for linting and testing code living uder src/python.
> 
> At the moment, all linting is done through the same `pylint`
> installation under support/.virtualenv, which requires ALL dependencies
> (i.e. pip-requirements.txt, requirements.in scattered in various
> directories) to be installed in the same virtualenv, making things
> really messy -- e.g. when I've changed some code under `src/python/lib`,
> but don't have the dev virtualenv activated, linting will fail since
> none of the dependencies under `src/python/lib` have been installed.
> 
> Using tox, we can solve this problem by distributing a "test spec"
> (tox.ini) in each of the python source directories which are aware of
> its local dependencies only. To test or lint the code there would be as
> simple as running `tox -e py27-lint `, and the corresponding
> virtualenv and test dependencies would automatically be setup.
> 
> This patch modifies `support/mesos-style.py` to install `tox` in
> `support/.virtualenv` and delegates linting to a `tox` call when it sees
> python directories that have tox setup for it. Linting for all other
> languages will not be effected.
> 
> Testing Done:
> 1. intentionally create a lint error, such as extra spaces before a
> parens in a python file
> 2. run the pre-commit hook and see tox in action
> 
> Reviewed at https://reviews.apache.org/r/64970/
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/tox.ini PRE-CREATION 
>   src/python/lib/requirements-test.in 
> b2b73aab65377d9310797203ea84c5150ae60805 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/mesos-style.py 1b34ea2d9afa8f17b545841cea7a6853a6e18144 
> 
> 
> Diff: https://reviews.apache.org/r/64970/diff/3/
> 
> 
> Testing
> ---
> 
> 1. intentionally create a lint error, such as extra spaces before a parens in 
> a python file
> 2. run the pre-commit hook and see tox in action
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 65311: Added the ObjectApprovers to which unifies authorization logic.

[Alexander Rojas]

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

(Updated Feb. 2, 2018, 4:01 p.m.)


Review request for mesos, Benjamin Hindman and Greg Mann.


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


Repository: mesos


Description
---

Introduces the class `ObjectApprovers` which unifies the different
mechanisms used in Mesos in order to perform authorization.

This new class will supersede the `Acceptor` interfaces and their
logic.

Follow up patches will make use of this interface and remove
deprecated code.


Diffs (updated)
-

  src/common/http.hpp 750d3bfba1647624983108bdd23295a3b3091fe4 
  src/common/http.cpp 728fc554917ed031f9cb3d811fbbc064307b3e32 


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

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 65311: Added the ObjectApprovers to which unifies authorization logic.

[Alexander Rojas]

> On Jan. 26, 2018, 5:01 a.m., Greg Mann wrote:
> > src/common/http.hpp
> > Lines 57 (patched)
> > 
> >
> > Is this case needed for type deduction? clang seems to build this code 
> > fine without it.

>From the research I did, not getting the underlying type of an enum just 
>defaults to int. I think this was done by benh in our effort to move towards 
>class enums, so I would prefer to keep this one.


> On Jan. 26, 2018, 5:01 a.m., Greg Mann wrote:
> > src/common/http.cpp
> > Lines 870 (patched)
> > 
> >
> > This seems like very simple validation to do. I would suggest if we 
> > want to ensure this, let's just do it now and remove the TODO?
> > 
> > Should it be a CHECK?

I just add everything to a set which will remove duplicates. Not sure if having 
them even demands a log line.


> On Jan. 26, 2018, 5:01 a.m., Greg Mann wrote:
> > src/common/http.cpp
> > Lines 875-887 (patched)
> > 
> >
> > Since this is a pretty hot code path, should we consider optimizations 
> > now?
> > 
> > Since the authorizer is not SOME or NONE on a per-action basis, we 
> > could have an `AcceptingObjectApprovers` which we return immediately if 
> > there is no authorizer:
> > 
> > ```
> > if (authorizer.isNone()) {
> >   return Owned(
> >   new AcceptingObjectApprovers());
> > } else {
> >   return process::collect( etc...
> > ```
> > 
> > WDYT?

I agree that it makes no sense to waste time creating `ObjectApprover` 
instances which will be accepting ones, I just figured out it can be done 
before and faster.


> On Jan. 26, 2018, 5:01 a.m., Greg Mann wrote:
> > src/common/http.cpp
> > Lines 1057-1059 (patched)
> > 
> >
> > Instead of a free function helper, what do you think about adding a 
> > specialization/overload to handle resources with the VIEW_ROLE action, like:
> > ```
> > template <>
> > bool ObjectApprovers::approved(
> > const Resource& resource)
> > {
> >   // Necessary because recovered agents are presented in old format.
> >   if (resource.has_role() && resource.role() != "*" &&
> >   !approved(resource.role())) {
> > return false;
> >   }
> > 
> >   // Reservations follow a path model where each entry is a child of the
> >   // previous one. Therefore, to accept the resource the acceptor has to
> >   // accept all entries.
> >   foreach (Resource::ReservationInfo reservation, 
> > resource.reservations()) {
> > if (!approved(reservation.role())) {
> >   return false;
> > }
> >   }
> > 
> >   if (resource.has_allocation_info() &&
> >   !approved(
> >   resource.allocation_info().role())) {
> > return false;
> >   }
> > 
> >   return true;
> > }
> > ```
> > 
> > And then the callsites could look like:
> > ```
> > approvers->approved(resource);
> > ```
> > 
> > WDYT?

I liked the idea but somehow the program didn't resolved the templates 
correctly. I would leave it like this initially, and I will try to figure out 
the right way of doing it. If you're ok with it.


- Alexander


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


On Jan. 24, 2018, 6:30 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65311/
> ---
> 
> (Updated Jan. 24, 2018, 6:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Greg Mann.
> 
> 
> Bugs: MESOS-8434
> https://issues.apache.org/jira/browse/MESOS-8434
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the class `ObjectApprovers` which unifies the different
> mechanisms used in Mesos in order to perform authorization.
> 
> This new class will supersede the `Acceptor` interfaces and their
> logic.
> 
> Follow up patches will make use of this interface and remove
> deprecated code.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 750d3bfba1647624983108bdd23295a3b3091fe4 
>   src/common/http.cpp 728fc554917ed031f9cb3d811fbbc064307b3e32 
> 
> 
> Diff: https://reviews.apache.org/r/65311/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 65470: Added metrics for showing number of subscribed resource providers.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/resource_provider/manager.cpp
Lines 75 (patched)


If this section is sorted like includes, this should go right after the 
first `process::http` section just below.



src/resource_provider/manager.cpp
Lines 200-201 (original), 218-219 (patched)


Not yours, but we should pull these onto the line of the last parameter 
like elsewhere.



src/resource_provider/manager.cpp
Lines 772 (patched)


At some point we'll move the prefix to a constant somewhere.



src/tests/resource_provider_manager_tests.cpp
Lines 1322-1323 (patched)


`StartMaster` will automatically pass default `masterFlags` if none are 
given. Let's remove them here since we do not need them in the test.



src/tests/resource_provider_manager_tests.cpp
Lines 1352 (patched)


`s/subscribed1/subscribed/g`

Here and below.



src/tests/resource_provider_manager_tests.cpp
Lines 1365 (patched)


`map`'s `operator[]` is not `const` which makes it unclear whether the test 
unknowingly modifies the test data. I'd suggest to make `snapshot` `const` and 
then use `at` here. We should also add an assertion to the values contain that 
key just before attempting to access it.


- Benjamin Bannier


On Feb. 2, 2018, 1:27 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65470/
> ---
> 
> (Updated Feb. 2, 2018, 1:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Greg Mann.
> 
> 
> Bugs: MESOS-8527
> https://issues.apache.org/jira/browse/MESOS-8527
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added metrics for showing number of subscribed resource providers.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 5d064bff76d5f18d85e6c050adf9edbc26107c07 
>   src/tests/resource_provider_manager_tests.cpp 
> 2944b066f8bfcbe328469940eb0e17b9b2809f63 
> 
> 
> Diff: https://reviews.apache.org/r/65470/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 65482: Fixed allocator bookkeeping of pending operations on master failover.

[Benjamin Bannier]

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

Review request for mesos, Jie Yu and Jan Schlicht.


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


Repository: mesos


Description
---

This patch fixes a bug where pending operations on a resource provider
resources where not properly accounted for in the allocator. This lead
to assertion failures when the operation became terminal and we
attempted to recover the used resources.

Since framework information is only remembered on agents if the
framework launched a task, there exists the possibility that a master
learns about an allocation to a framework unknown to it, yet. To
accommodate that do not bookkeep allocations to unknown frameworks in
the allocator and update code handling of terminal operation updates
accordingly.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
f33ff767dcb93556beb696c96f8cfc17baccb05e 
  src/master/master.cpp cc2685a6bc14103c639ce776cf1c912361e93381 


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


Testing
---

`make check`, also tested with a version of the test added in r/65045 which 
triggered this issue.


Thanks,

Benjamin Bannier

Re: Review Request 65045: Tested correct operation handling during master failover.

[Jan Schlicht]

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

(Updated Feb. 2, 2018, 2:30 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed issues, removed flakyness.


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


Repository: mesos


Description
---

Tested correct operation handling during master failover.


Diffs (updated)
-

  src/tests/master_tests.cpp 7112bb4efcc312dc4f68bdc44fb685c7624002b1 


Diff: https://reviews.apache.org/r/65045/diff/6/

Changes: https://reviews.apache.org/r/65045/diff/5-6/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 65044: Added the v1 API 'GET_OPERATIONS' call for master and agent.

[Jan Schlicht]

> On Feb. 1, 2018, 7:46 a.m., Greg Mann wrote:
> > src/tests/api_tests.cpp
> > Lines 6280 (patched)
> > 
> >
> > Could you leave a comment here saying that we do this simply to make it 
> > easier to handle resource provider communication?

No longer necessary, I've removed this line here and in the other test case.


- Jan


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


On Feb. 2, 2018, 2:21 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65044/
> ---
> 
> (Updated Feb. 2, 2018, 2:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, Jie 
> Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-8424
> https://issues.apache.org/jira/browse/MESOS-8424
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The 'GET_OPERATIONS' call lists all operations known to master or agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 315820041990d28e5cdae71c8385d4551d56558c 
>   include/mesos/master/master.proto 3e34634f3b864222d07374936c0d9a18269c2fbd 
>   include/mesos/v1/agent/agent.proto 9e8b49defb83ffd4dd240ebb3a09b2dd3610ab2a 
>   include/mesos/v1/master/master.proto 
> 6759c3004e7e04c0044360b0c54f4438c2f6ec8a 
>   src/master/http.cpp c489b6f525f157811549b2cc84addd9d85e87990 
>   src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 
>   src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 
>   src/slave/http.hpp 1619bb77e3e7d5a6d387e4d4bb071d83ce914967 
>   src/slave/http.cpp 77e711ceeb0e2613d629b5e21fd686f85dfad11a 
>   src/slave/validation.cpp 0c2ccda177734cf3c47c0346ed34d20d58e7d932 
>   src/tests/api_tests.cpp b639a4bafbf69d3ea53aa0573e9f0489c3a1a57f 
> 
> 
> Diff: https://reviews.apache.org/r/65044/diff/9/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 65044: Added the v1 API 'GET_OPERATIONS' call for master and agent.

[Jan Schlicht]

> On Feb. 1, 2018, 7:46 a.m., Greg Mann wrote:
> > src/tests/api_tests.cpp
> > Lines 6349-6352 (patched)
> > 
> >
> > Are these necessary? I think that framework registration may be 
> > sufficient to produce an offer?

`settle` isn't necessary, but `advance` is, see the similar comment in #65045.


- Jan


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


On Feb. 2, 2018, 2:21 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65044/
> ---
> 
> (Updated Feb. 2, 2018, 2:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Greg Mann, Jie 
> Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-8424
> https://issues.apache.org/jira/browse/MESOS-8424
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The 'GET_OPERATIONS' call lists all operations known to master or agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 315820041990d28e5cdae71c8385d4551d56558c 
>   include/mesos/master/master.proto 3e34634f3b864222d07374936c0d9a18269c2fbd 
>   include/mesos/v1/agent/agent.proto 9e8b49defb83ffd4dd240ebb3a09b2dd3610ab2a 
>   include/mesos/v1/master/master.proto 
> 6759c3004e7e04c0044360b0c54f4438c2f6ec8a 
>   src/master/http.cpp c489b6f525f157811549b2cc84addd9d85e87990 
>   src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 
>   src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 
>   src/slave/http.hpp 1619bb77e3e7d5a6d387e4d4bb071d83ce914967 
>   src/slave/http.cpp 77e711ceeb0e2613d629b5e21fd686f85dfad11a 
>   src/slave/validation.cpp 0c2ccda177734cf3c47c0346ed34d20d58e7d932 
>   src/tests/api_tests.cpp b639a4bafbf69d3ea53aa0573e9f0489c3a1a57f 
> 
> 
> Diff: https://reviews.apache.org/r/65044/diff/9/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 65464: WIP: Introduced `mesos-build`, along with pre-built docker images.

[Benjamin Bannier]

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


Fix it, then Ship it!




Thanks for these patches mpark. I left some comments, mostly around reducing 
the size of the images the CI would need to download.

Before rolling this out we should make sure we have some automation in tool 
keeping the images up to date from source. Like discussed offline we might be 
able to leverage dockerhub's infra for that.


support/mesos-build.sh
Lines 33 (patched)


Nit: I think we don't use markdown in logging output, maybe use single 
quotes around `mesos-build` here.



support/mesos-build/centos-7.dockerfile
Lines 18 (patched)


Just as a heads-up, while We also use this in the `mesos-tidy` image, the 
syntax is actually deprecated upstream by now, 
https://docs.docker.com/engine/reference/builder/#maintainer-deprecated.



support/mesos-build/centos-7.dockerfile
Lines 21 (patched)


This seems even worse than the unpinned dependencies we have below since we 
might pick up random, disruptive upstream changes.

Can we just get rid of this? It seems to not be required.



support/mesos-build/centos-7.dockerfile
Lines 22-24 (patched)


Let's use a single layer for this, i.e., just chain the commands with `&&`.



support/mesos-build/centos-7.dockerfile
Lines 42 (patched)


Let's clean up local caches in this layer, i.e.,

yum clean all && \
rm -rf /var/cache/yum



support/mesos-build/centos-7.dockerfile
Lines 47 (patched)


Let's remove the downloaded script in this layer, i.e., add

rm -f /tmp/install-cmake.sh



support/mesos-build/centos-7.dockerfile
Lines 52 (patched)


Let's use `COPY` here, 
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#add-or-copy.



support/mesos-build/entrypoint.sh
Lines 53 (patched)


Quote this.



support/mesos-build/entrypoint.sh
Lines 77 (patched)


Quote this.



support/mesos-build/ubuntu-16.04.dockerfile
Lines 18 (patched)


Just as a heads-up, while We also use this in the `mesos-tidy` image, the 
syntax is actually deprecated upstream by now, 
https://docs.docker.com/engine/reference/builder/#maintainer-deprecated.



support/mesos-build/ubuntu-16.04.dockerfile
Lines 42 (patched)


We don't seem to need this.



support/mesos-build/ubuntu-16.04.dockerfile
Lines 44 (patched)


Let's also get rid of the dowloaded lists, i.e., add

rm -rf /var/lib/apt/lists



support/mesos-build/ubuntu-16.04.dockerfile
Lines 49 (patched)


Let's remove the downloaded script in this layer, i.e., add

rm -f /tmp/install-cmake.sh



support/mesos-build/ubuntu-16.04.dockerfile
Lines 55 (patched)


Let's use `COPY` here, 
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#add-or-copy.


- Benjamin Bannier


On Feb. 1, 2018, 8:20 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65464/
> ---
> 
> (Updated Feb. 1, 2018, 8:20 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced `mesos-build`, along with pre-built docker images.
> 
> 
> Diffs
> -
> 
>   support/jenkins/buildbot.sh 7f78509699b036df025c314fe913158f54402014 
>   support/mesos-build.sh PRE-CREATION 
>   support/mesos-build/centos-7.dockerfile PRE-CREATION 
>   support/mesos-build/entrypoint.sh PRE-CREATION 
>   support/mesos-build/ubuntu-16.04.dockerfile PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65464/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>