Re: Review Request 64689: Windows: Fixed `os::open()` to always use `O_BINARY`.

[Gaston Kleiman]

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


Ship it!




Ship it!!! It would have saved me a lot of time last week! =).

- Gaston Kleiman


On Dec. 18, 2017, 7:34 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64689/
> ---
> 
> (Updated Dec. 18, 2017, 7:34 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, Gaston Kleiman, and 
> Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we had been manually adding the `O_BINARY` flag as we
> encountered bugs due to the Windows default behavior of performing
> line-ending translation. This was error prone.
> 
> Given this precedent, it seems safe to assume that all our existing code
> expects the POSIX semantics of "binary mode", that is, no translation of
> written data at all. So now we add this flag by default in `os::open()`
> instead of in the users.
> 
> It is possible that a future use requires text translation. At such
> point, we can trivially fix `os::open()` to take a boolean flag to
> control the addition of `O_BINARY`, but we do not currently need to
> engineer this.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/net.hpp 
> 7b6557d40a83e1572bdc2dd89b5ff99fb8ed696a 
>   3rdparty/stout/include/stout/os/open.hpp 
> 1443b63d260b3da38073f234d15ffb4b97d4a736 
>   3rdparty/stout/include/stout/os/write.hpp 
> 4c718b1a5055a742f16cac0bc5e88aa4ab6acfcd 
>   3rdparty/stout/include/stout/protobuf.hpp 
> baad12648dd78ab72ea4277f4c7f99da16696a40 
> 
> 
> Diff: https://reviews.apache.org/r/64689/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 64698: Made quota headroom calculation on a per-role basis.

[Benjamin Mahler]

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


Fix it, then Ship it!




Nice fix!

I would suggest a summary that makes it clear we're fixing something here, e.g.

```
Fixed a bug where insufficient headroom is held for quota.
```

I think the second paragraph could use a little clarifying as well in terms of 
required vs available headroom? Or maybe just clarifying how it works?

```
This patch fixes the issue by computing the amount of unreserved
resources we need to hold back such that each quota role can have
its quota satisfied. Previously, we included the quota role
unallocated reservations as part of the headroom since we knew it
would be held back, but we didn't handle the case that a quota role
had more reservations than quota.
```

Something like this?

I can work these minor changes in and get this committed, thanks for the fix 
Meng!


src/master/allocator/mesos/hierarchical.cpp
Lines 1838-1844 (patched)


Hm.. couldn't we do?

```
if (allocated.contains(required)) {
  continue; // Quota already satisfied.
}

Resources unallocatedQuota = required - allocated;
```



src/master/allocator/mesos/hierarchical.cpp
Lines 1851-1852 (patched)


I think we can safely avoid the need for checking the subtraction here.


- Benjamin Mahler


On Dec. 19, 2017, 3:01 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64698/
> ---
> 
> (Updated Dec. 19, 2017, 3:01 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8339
> https://issues.apache.org/jira/browse/MESOS-8339
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a role has more reservation than its quota, the
> current quota headroom calculation is insufficient
> in guaranteeing quota allocation.
> See MESOS-8339.
> 
> This patch fixes this bug by calculating quota headroom
> on a per-role basis (before aggregating) and no longer
> counting unallocated reservations (including quota role's
> unallocated reservations) towards quota headroom.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2cabafeb7ba0cda482ea4464f19730d2ef30cc5e 
> 
> 
> Diff: https://reviews.apache.org/r/64698/diff/1/
> 
> 
> Testing
> ---
> 
> make check and a dedicated test #64699
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 64699: Added a test to ensure quota headroom is maintained.

[Benjamin Mahler]

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


Ship it!




Very nice!


src/tests/hierarchical_allocator_tests.cpp
Lines 1763 (patched)


WhenReservationsExceedQuota



src/tests/hierarchical_allocator_tests.cpp
Lines 1805 (patched)


are no


- Benjamin Mahler


On Dec. 19, 2017, 3:02 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64699/
> ---
> 
> (Updated Dec. 19, 2017, 3:02 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8339
> https://issues.apache.org/jira/browse/MESOS-8339
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that quota headroom is correctly
> maintained when some roles have more reservations than
> their quota. We need to ensure that one role's excessive
> reservaiton is not counted towards another role's quota
> headroom.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 4127d0591e4abffa74720fecccb1999c1a808534 
> 
> 
> Diff: https://reviews.apache.org/r/64699/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 64690: Windows: Removed manual use of `O_BINARY`.

[Mesos Reviewbot Windows]

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



FAIL: Mesos tests failed to build.

Reviews applied: `['64689', '64690']`

Failed command: `cmake.exe --build . --target mesos-tests --config Debug`

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

Relevant logs:

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

```
  D:\DCOS\mesos\mesos\src\authentication\cram_md5\authenticatee.cpp(75): 
warning C4267: '=': conversion from 'size_t' to 'unsigned long', possible loss 
of data [D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\authentication\cram_md5\authenticatee.cpp(271): 
warning C4267: 'argument': conversion from 'size_t' to 'unsigned int', possible 
loss of data [D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\authentication\cram_md5\authenticatee.cpp(333): 
warning C4267: '=': conversion from 'size_t' to 'unsigned int', possible loss 
of data [D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\authentication\cram_md5\authenticator.cpp(216): 
warning C4267: 'argument': conversion from 'size_t' to 'unsigned int', possible 
loss of data [D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\authentication\cram_md5\authenticator.cpp(242): 
warning C4267: 'argument': conversion from 'size_t' to 'unsigned int', possible 
loss of data [D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\authentication\cram_md5\authenticator.cpp(274): 
warning C4267: '=': conversion from 'size_t' to 'unsigned int', possible loss 
of data [D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master/master.hpp(2093): warning C4244: 'return': 
conversion from 'unsigned __int64' to 'double', possible loss of data 
(compiling source file D:\DCOS\mesos\mesos\src\common\protobuf_utils.cpp) 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\files\files.cpp(704): warning C4267: 'argument': 
conversion from 'size_t' to 'off_t', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master/master.hpp(2093): warning C4244: 'return': 
conversion from 'unsigned __int64' to 'double', possible loss of data 
(compiling source file D:\DCOS\mesos\mesos\src\master\master.cpp) 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master/master.hpp(2093): warning C4244: 'return': 
conversion from 'unsigned __int64' to 'double', possible loss of data 
(compiling source file D:\DCOS\mesos\mesos\src\master\quota_handler.cpp) 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master\master.cpp(6223): warning C4244: 'argument': 
conversion from 'const ::size_t' to 'double', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master\master.cpp(6325): warning C4244: 'argument': 
conversion from 'const ::size_t' to 'double', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master\master.cpp(6890): warning C4244: 'argument': 
conversion from 'const ::size_t' to 'double', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master/master.hpp(2093): warning C4244: 'return': 
conversion from 'unsigned __int64' to 'double', possible loss of data 
(compiling source file D:\DCOS\mesos\mesos\src\master\weights_handler.cpp) 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master\allocator\sorter\drf\sorter.cpp(589): warning 
C4267: 'return': conversion from 'size_t' to 'int', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master\master.cpp(9012): warning C4244: 'argument': 
conversion from 'const ::size_t' to 'double', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master\master.cpp(10853): warning C4244: 'return': 
conversion from '::size_t' to 'double', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]


"D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
"D:\DCOS\mesos\src\mesos.vcxproj" (default target) (12) ->
(ClCompile target) -> 
  D:\DCOS\mesos\mesos\include\csi/spec.hpp(21): fatal error C1083: Cannot open 
include file: 'csi/csi.grpc.pb.h': No such file or directory (compiling source 
file D:\DCOS\mesos\mesos\src\slave\task_status_update_manager.cpp) 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\include\csi/spec.hpp(21): fatal error C1083: Cannot open 
include file: 'csi/csi.grpc.pb.h': No such file or directory (compiling source 
file D:\DCOS\mesos\mesos\src\slave\slave.cpp) [D:\DCOS\mesos\src\mesos.vcxproj]

132 Warning(s)
2 Error(s)

Time Elapsed 00:02:53.45
```

- 
[mesos-tests-CMakeOutput.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64690/logs/mesos-tests-CMakeOutput.log):

Re: Review Request 64701: Windows: Added `/EHsc` to `CMAKE_CXX_FLAGS`.

[Mesos Reviewbot Windows]

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



FAIL: Mesos tests failed to build.

Reviews applied: `['64701']`

Failed command: `cmake.exe --build . --target mesos-tests --config Debug`

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

Relevant logs:

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

```
  D:\DCOS\mesos\mesos\src\authentication\cram_md5\authenticatee.cpp(75): 
warning C4267: '=': conversion from 'size_t' to 'unsigned long', possible loss 
of data [D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\authentication\cram_md5\authenticatee.cpp(271): 
warning C4267: 'argument': conversion from 'size_t' to 'unsigned int', possible 
loss of data [D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\authentication\cram_md5\authenticatee.cpp(333): 
warning C4267: '=': conversion from 'size_t' to 'unsigned int', possible loss 
of data [D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\authentication\cram_md5\authenticator.cpp(216): 
warning C4267: 'argument': conversion from 'size_t' to 'unsigned int', possible 
loss of data [D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\authentication\cram_md5\authenticator.cpp(242): 
warning C4267: 'argument': conversion from 'size_t' to 'unsigned int', possible 
loss of data [D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\authentication\cram_md5\authenticator.cpp(274): 
warning C4267: '=': conversion from 'size_t' to 'unsigned int', possible loss 
of data [D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master/master.hpp(2093): warning C4244: 'return': 
conversion from 'unsigned __int64' to 'double', possible loss of data 
(compiling source file D:\DCOS\mesos\mesos\src\common\protobuf_utils.cpp) 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\files\files.cpp(704): warning C4267: 'argument': 
conversion from 'size_t' to 'off_t', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master/master.hpp(2093): warning C4244: 'return': 
conversion from 'unsigned __int64' to 'double', possible loss of data 
(compiling source file D:\DCOS\mesos\mesos\src\master\master.cpp) 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master/master.hpp(2093): warning C4244: 'return': 
conversion from 'unsigned __int64' to 'double', possible loss of data 
(compiling source file D:\DCOS\mesos\mesos\src\master\quota_handler.cpp) 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master\master.cpp(6223): warning C4244: 'argument': 
conversion from 'const ::size_t' to 'double', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master\master.cpp(6325): warning C4244: 'argument': 
conversion from 'const ::size_t' to 'double', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master\master.cpp(6890): warning C4244: 'argument': 
conversion from 'const ::size_t' to 'double', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master/master.hpp(2093): warning C4244: 'return': 
conversion from 'unsigned __int64' to 'double', possible loss of data 
(compiling source file D:\DCOS\mesos\mesos\src\master\weights_handler.cpp) 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master\master.cpp(9012): warning C4244: 'argument': 
conversion from 'const ::size_t' to 'double', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master\allocator\sorter\drf\sorter.cpp(589): warning 
C4267: 'return': conversion from 'size_t' to 'int', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\src\master\master.cpp(10853): warning C4244: 'return': 
conversion from '::size_t' to 'double', possible loss of data 
[D:\DCOS\mesos\src\mesos.vcxproj]


"D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
"D:\DCOS\mesos\src\mesos.vcxproj" (default target) (12) ->
(ClCompile target) -> 
  D:\DCOS\mesos\mesos\include\csi/spec.hpp(21): fatal error C1083: Cannot open 
include file: 'csi/csi.grpc.pb.h': No such file or directory (compiling source 
file D:\DCOS\mesos\mesos\src\slave\task_status_update_manager.cpp) 
[D:\DCOS\mesos\src\mesos.vcxproj]
  D:\DCOS\mesos\mesos\include\csi/spec.hpp(21): fatal error C1083: Cannot open 
include file: 'csi/csi.grpc.pb.h': No such file or directory (compiling source 
file D:\DCOS\mesos\mesos\src\slave\slave.cpp) [D:\DCOS\mesos\src\mesos.vcxproj]

132 Warning(s)
2 Error(s)

Time Elapsed 00:02:46.17
```

- 
[mesos-tests-CMakeOutput.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64701/logs/mesos-tests-CMakeOutput.log):

```
  

Re: Review Request 64699: Added a test to ensure quota headroom is maintained.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64699 was successfully built and tested.

Reviews applied: `['64698', '64699']`

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

- Mesos Reviewbot Windows


On Dec. 19, 2017, 3:02 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64699/
> ---
> 
> (Updated Dec. 19, 2017, 3:02 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8339
> https://issues.apache.org/jira/browse/MESOS-8339
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that quota headroom is correctly
> maintained when some roles have more reservations than
> their quota. We need to ensure that one role's excessive
> reservaiton is not counted towards another role's quota
> headroom.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 4127d0591e4abffa74720fecccb1999c1a808534 
> 
> 
> Diff: https://reviews.apache.org/r/64699/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 64701: Windows: Added `/EHsc` to `CMAKE_CXX_FLAGS`.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Joseph Wu, and Michael Park.


Repository: mesos


Description
---

After upgrading Visual Studio to 15.5.2, warning C4530 started being
emitted for practically every file. Since we use the STL (which uses
exceptions), apparently we need to compile with `/EHsc`. This sets the
exception-handling model to catch C++ exceptions, and to assume that
functions declared as `extern "C"` never throw a C++ exception.


Diffs
-

  cmake/CompilationConfigure.cmake dc9dc161dce1c714748bcec2fc15c4fbfbccaec2 


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


Testing
---

Built on Windows, warning is gone (and gone in 3rdparty builds too).


Thanks,

Andrew Schwartzmeyer

Re: Review Request 64664: Updated logging for storage local resource provider.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 18, 2017, 11:57 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64664/
> ---
> 
> (Updated Dec. 18, 2017, 11:57 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated logging for storage local resource provider.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 2a167aa5c50598758cab4ab6c13f1b1e33e692dd 
>   src/resource_provider/storage/provider.cpp 
> 158b6b408002209aa9a79a6772da30c984aad61a 
> 
> 
> Diff: https://reviews.apache.org/r/64664/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64665: Fixed the calculation of available capacity test CSI plugin.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 16, 2017, 10:28 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64665/
> ---
> 
> (Updated Dec. 16, 2017, 10:28 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The size of existing volumes should be substracted away from the
> available capacity. Otherwise, the total capacity will keep increasing
> every time the plugin restarts.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp 742aea94c4978a9931b44c3f221648b80f45c2db 
> 
> 
> Diff: https://reviews.apache.org/r/64665/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 64690: Windows: Removed manual use of `O_BINARY`.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Jeff Coffler, Gaston Kleiman, and Joseph 
Wu.


Repository: mesos


Description
---

This is now superfluous as it is added in stout.


Diffs
-

  src/status_update_manager/status_update_manager_process.hpp 
5dc2bec14de972efa2dd00f9faf6c025f2e56e15 


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


Testing
---

`ctest -V -C Debug` on Windows 10.


Thanks,

Andrew Schwartzmeyer

Review Request 64689: Windows: Fixed `os::open()` to always use `O_BINARY`.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Jeff Coffler, Gaston Kleiman, and Joseph 
Wu.


Repository: mesos


Description
---

Previously, we had been manually adding the `O_BINARY` flag as we
encountered bugs due to the Windows default behavior of performing
line-ending translation. This was error prone.

Given this precedent, it seems safe to assume that all our existing code
expects the POSIX semantics of "binary mode", that is, no translation of
written data at all. So now we add this flag by default in `os::open()`
instead of in the users.

It is possible that a future use requires text translation. At such
point, we can trivially fix `os::open()` to take a boolean flag to
control the addition of `O_BINARY`, but we do not currently need to
engineer this.


Diffs
-

  3rdparty/stout/include/stout/net.hpp 7b6557d40a83e1572bdc2dd89b5ff99fb8ed696a 
  3rdparty/stout/include/stout/os/open.hpp 
1443b63d260b3da38073f234d15ffb4b97d4a736 
  3rdparty/stout/include/stout/os/write.hpp 
4c718b1a5055a742f16cac0bc5e88aa4ab6acfcd 
  3rdparty/stout/include/stout/protobuf.hpp 
baad12648dd78ab72ea4277f4c7f99da16696a40 


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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 64003: Made quota resource allocation fine-grained.

[Benjamin Mahler]

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




src/master/allocator/mesos/hierarchical.cpp
Line 1750 (original), 1797 (patched)


limits



src/master/allocator/mesos/hierarchical.cpp
Line 1751 (original), 1798 (patched)


I think this patch also needs to update the quota satisfaction criteria now 
that we can chop. Breaking when only a single guarantee is reached was meant as 
a stop-gap for the gaming ticket reference on `someGuaranteesReached`. Now we 
could move back to considering quota satisfied only when all of the guarantees 
are reached.



src/master/allocator/mesos/hierarchical.cpp
Line 1752 (original), 1799 (patched)


Maybe: These are all scalar quantities



src/master/allocator/mesos/hierarchical.cpp
Lines 1800 (patched)


Maybe `unsatisfiedQuota`?



src/master/allocator/mesos/hierarchical.cpp
Lines 1803 (patched)


Do we need a note here to clarify why we don't care to use 
`allocatableTo(role)`?

```
// Note that since we currently only support top-level roles to
// have quota, there are no ancestor reservations involved here.
```



src/master/allocator/mesos/hierarchical.cpp
Lines 1806-1814 (patched)


The more important distinction for the reader here seems to be the 
resources that have a limit and those that don't?

(1) resources w/ limit: always choppable, chop up to limit

(2) resources w/o limit: offer entirely, may or may not be choppable but we 
don't care..?

I think with the current logic here, if I don't have quota limit for 
"some-scalar", because it's choppable it will get excluded during the 
intersection and I won't get it offered?

Also, as discussed offline, this is missing metadata?


- Benjamin Mahler


On Dec. 12, 2017, 2:33 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64003/
> ---
> 
> (Updated Dec. 12, 2017, 2:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-7099
> https://issues.apache.org/jira/browse/MESOS-7099
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allocator now does fine-grained resource allocation up
> to the role’s quota limit.
> 
> Also fixed a few affected allocator tests.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> fccd71c96fe8e4d914e19b5ea8d8f69e9ebf2406 
>   src/tests/hierarchical_allocator_tests.cpp 
> f5fb47ed09682ebdd047aec7e79a86597ee09f53 
> 
> 
> Diff: https://reviews.apache.org/r/64003/diff/3/
> 
> 
> Testing
> ---
> 
> Fixed a few broken tests due to this patch.
> make check.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 64615: Moved SLRP resource recovery logic into a helper.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Dec. 14, 2017, 1:34 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64615/
> ---
> 
> (Updated Dec. 14, 2017, 1:34 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
> 
> 
> Bugs: MESOS-8251
> https://issues.apache.org/jira/browse/MESOS-8251
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This logically separates the Storage Local Resource Provider's
> recovery logic so that we can insert another step between
> recovering resources and starting the ResourceProvider Driver.
> 
> A subsequent patch will insert the intermediate recovery step(s).
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 158b6b408002209aa9a79a6772da30c984aad61a 
> 
> 
> Diff: https://reviews.apache.org/r/64615/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 64612: Added flag to pass VolumeProfileAdaptor module to SLRP.

[Jie Yu]

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


Fix it, then Ship it!





src/resource_provider/volume_profile.cpp
Lines 110 (patched)


2 lines apart


- Jie Yu


On Dec. 19, 2017, 3:08 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64612/
> ---
> 
> (Updated Dec. 19, 2017, 3:08 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
> 
> 
> Bugs: MESOS-8251
> https://issues.apache.org/jira/browse/MESOS-8251
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds an agent flag to choose the VolumeProfileAdaptor module
> to load and passes this information down to the storage resource
> providers. A single module can potentially be used by multiple
> storage resource providers.
> 
> We choose to pass the module along as a global variable because the
> module is not needed by many of the components in the injection path.
> Specifically, the Storage Local Resource Provider will need the module,
> but its parents, the Local Resource Provider and Resource Provider
> Daemons will not.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/volume_profile.hpp PRE-CREATION 
>   src/resource_provider/storage/provider.cpp 
> 158b6b408002209aa9a79a6772da30c984aad61a 
>   src/resource_provider/volume_profile.cpp PRE-CREATION 
>   src/slave/flags.hpp 5918484825c3c813883c42465f3e5a5693d6a2cd 
>   src/slave/flags.cpp d4b95b7542733b22299bc19235cf99220534c022 
>   src/slave/slave.hpp 75cc5836e39b2413eaefc5a5c9e905134db83743 
>   src/slave/slave.cpp e69d42a2bfae09e2defcd8333c0434a9fd8bac4c 
> 
> 
> Diff: https://reviews.apache.org/r/64612/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> Basically just a plumbing change to pass the module into the SLRP.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 64695: Improved the comments describing the offer operation states.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64695 was successfully built and tested.

Reviews applied: `['64695']`

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

- Mesos Reviewbot Windows


On Dec. 18, 2017, 7:54 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64695/
> ---
> 
> (Updated Dec. 18, 2017, 7:54 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved the comments describing the offer operation states.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto ddb9d368cc793a4cb865686e672ab05e91c9ec48 
>   include/mesos/v1/mesos.proto 309079309cd1f6d5dc5a2e7de4e0f86f434f6451 
> 
> 
> Diff: https://reviews.apache.org/r/64695/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 64697: Windows: Deleted unused and unnecessary OS version functions.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64697 was successfully built and tested.

Reviews applied: `['64697']`

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

- Mesos Reviewbot Windows


On Dec. 18, 2017, 9:07 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64697/
> ---
> 
> (Updated Dec. 18, 2017, 9:07 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Jeff Coffler, and Joseph Wu.
> 
> 
> Bugs: MESOS-7781
> https://issues.apache.org/jira/browse/MESOS-7781
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Windows API `GetVersionEx` was deprecated in Windows 8. Using it in
> the `os.hpp` header caused hundreds of warnings that it was deprecated
> to be emitted when building. While the function still exists, it stopped
> returning the correct value when it was deprecated (so the version it
> returns is 6.3, that is, Windows 8).
> 
> However, replacing it was less than straightforward. The recommended
> replacement is to use the "version helper functions", but these are not
> capable of providing the version information of the system. The intent
> of the deprecation and the new APIs is to prevent developers from using
> the version of Windows as a feature check. The new APIs are all of the
> form `bool IsWindows10OrGreater()`. While it is possible to use
> `IsWindowsServer()` to determine if we're on a client or server version
> of Windows, no current user-mode API is provided to query the build
> information of the OS. This is unfortunate, as retrieving this
> information is a valid use case of the now deprecated API.
> 
> An explored alternative was to query the registry, but this was
> discarded as it was not consistently available.
> 
> Another alternative was to parse the output of `systeminfo`, which can
> return CSV formatted version information. However, we chose not to do
> this currently as it is slow (on the order of seconds to invoke the
> command).
> 
> There still exists a kernel-mode API to retrieve the version
> information. However, to use it would entail either including WDK (which
> is massive and not something we're going to do), or to invoke it
> dynamically by getting the address from `nt.dll`. This is possible, but
> it would be hacky, and was not necessary.
> 
> The chosen route was to simply delete the use of `GetVersionEx`. After
> reviewing the use of these functions, it turned out to be entirely
> possible to `delete` them.
> 
> Note that this means the entirety of `systems_tests.cpp` was rendered
> pointless for Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os.hpp 
> 1a81db61faa55d7043d75a012fe1e66b49bf601c 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 26a1a049c7a4e1b6a3b6767a9c2c86e7538f6012 
>   3rdparty/stout/tests/CMakeLists.txt 
> 940fc9f6997695cf6c181ecbc72d6fd6e72dc7e7 
>   3rdparty/stout/tests/os/systems_tests.cpp 
> 286d34edacab932aaecacfa6259a0bc549f01b1b 
> 
> 
> Diff: https://reviews.apache.org/r/64697/diff/1/
> 
> 
> Testing
> ---
> 
> `ctest -V -C Debug` on Windows 10, `make check` on CentOS 7.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 64354: Added tests for UriVolumeProfile module.

[Joseph Wu]

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

(Updated Dec. 18, 2017, 7:07 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jie Yu.


Changes
---

Shortened one namespace.


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


Repository: mesos


Description
---

These tests mostly exercise the parsing logic used by the module
and some of the interaction expected from callers of the module
(i.e. the Storage Local Resource Provider).


Diffs (updated)
-

  src/Makefile.am f5a4edd245e8d6535502e951ecaa526c2bae25f9 
  src/tests/volume_profile_tests.cpp PRE-CREATION 


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

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


Testing
---

make check


Thanks,

Joseph Wu

Re: Review Request 64353: Added example VolumeProfile module.

[Joseph Wu]

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

(Updated Dec. 18, 2017, 7:08 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jie Yu.


Changes
---

Shortened one namespace.


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


Repository: mesos


Description
---

This example module shows how a VolumeProfile module might be
implemented (and is a viable module in its own right).  The module
can be configured to fetch a map of profiles from a URI (`file://` or
`http(s)://`) and possibly cache this item for some time.


Diffs (updated)
-

  src/Makefile.am f5a4edd245e8d6535502e951ecaa526c2bae25f9 
  src/csi/uri_volume_profile.proto PRE-CREATION 
  src/resource_provider/uri_volume_profile.hpp PRE-CREATION 
  src/resource_provider/uri_volume_profile.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/64353/diff/8/

Changes: https://reviews.apache.org/r/64353/diff/7-8/


Testing
---

See end of chain.


Thanks,

Joseph Wu

Re: Review Request 64612: Added flag to pass VolumeProfileAdaptor module to SLRP.

[Joseph Wu]

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

(Updated Dec. 18, 2017, 7:08 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jie Yu.


Changes
---

Changed a global var to a POD type.  Added some comments.


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


Repository: mesos


Description
---

This adds an agent flag to choose the VolumeProfileAdaptor module
to load and passes this information down to the storage resource
providers. A single module can potentially be used by multiple
storage resource providers.

We choose to pass the module along as a global variable because the
module is not needed by many of the components in the injection path.
Specifically, the Storage Local Resource Provider will need the module,
but its parents, the Local Resource Provider and Resource Provider
Daemons will not.


Diffs (updated)
-

  include/mesos/resource_provider/volume_profile.hpp PRE-CREATION 
  src/resource_provider/storage/provider.cpp 
158b6b408002209aa9a79a6772da30c984aad61a 
  src/resource_provider/volume_profile.cpp PRE-CREATION 
  src/slave/flags.hpp 5918484825c3c813883c42465f3e5a5693d6a2cd 
  src/slave/flags.cpp d4b95b7542733b22299bc19235cf99220534c022 
  src/slave/slave.hpp 75cc5836e39b2413eaefc5a5c9e905134db83743 
  src/slave/slave.cpp e69d42a2bfae09e2defcd8333c0434a9fd8bac4c 


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

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


Testing
---

make check

Basically just a plumbing change to pass the module into the SLRP.


Thanks,

Joseph Wu

Review Request 64699: Added a test to ensure quota headroom is maintained.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This test verifies that quota headroom is correctly
maintained when some roles have more reservations than
their quota. We need to ensure that one role's excessive
reservaiton is not counted towards another role's quota
headroom.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
4127d0591e4abffa74720fecccb1999c1a808534 


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


Testing
---

make check.


Thanks,

Meng Zhu

Review Request 64698: Made quota headroom calculation on a per-role basis.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

If a role has more reservation than its quota, the
current quota headroom calculation is insufficient
in guaranteeing quota allocation.
See MESOS-8339.

This patch fixes this bug by calculating quota headroom
on a per-role basis (before aggregating) and no longer
counting unallocated reservations (including quota role's
unallocated reservations) towards quota headroom.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
2cabafeb7ba0cda482ea4464f19730d2ef30cc5e 


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


Testing
---

make check and a dedicated test #64699


Thanks,

Meng Zhu

Re: Review Request 64612: Added flag to pass VolumeProfileAdaptor module to SLRP.

[Joseph Wu]

> On Dec. 15, 2017, 3:26 p.m., Jie Yu wrote:
> > src/resource_provider/local.cpp
> > Lines 39 (patched)
> > 
> >
> > It's weird that LocalResourceProvider needs to be aware 
> > `volumeProfileAdaptor` which is very StorageLocalResourceProvider specific.
> > 
> > It's more appropriate to instantiate that in 
> > StorageLocalResourceProvider and make it a singleton.
> > 
> > probably passing the agent flags all the way to this method (or use 
> > `Parameters` to pass in provider specifc parameters).

I've changed the injection method.  I did it the way I did ('create', 'get', 
and 'set' methods) because tests may instantiate different varieties of the 
module in tests, so just having a `once` is not sufficient to allow this.


- Joseph


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


On Dec. 18, 2017, 6:35 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64612/
> ---
> 
> (Updated Dec. 18, 2017, 6:35 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
> 
> 
> Bugs: MESOS-8251
> https://issues.apache.org/jira/browse/MESOS-8251
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds an agent flag to choose the VolumeProfileAdaptor module
> to load and passes this information down to the storage resource
> providers. A single module can potentially be used by multiple
> storage resource providers.
> 
> We choose to pass the module along as a global variable because the
> module is not needed by many of the components in the injection path.
> Specifically, the Storage Local Resource Provider will need the module,
> but its parents, the Local Resource Provider and Resource Provider
> Daemons will not.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/volume_profile.hpp PRE-CREATION 
>   src/resource_provider/storage/provider.cpp 
> 158b6b408002209aa9a79a6772da30c984aad61a 
>   src/resource_provider/volume_profile.cpp PRE-CREATION 
>   src/slave/flags.hpp 5918484825c3c813883c42465f3e5a5693d6a2cd 
>   src/slave/flags.cpp d4b95b7542733b22299bc19235cf99220534c022 
>   src/slave/slave.hpp 75cc5836e39b2413eaefc5a5c9e905134db83743 
>   src/slave/slave.cpp e69d42a2bfae09e2defcd8333c0434a9fd8bac4c 
> 
> 
> Diff: https://reviews.apache.org/r/64612/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> Basically just a plumbing change to pass the module into the SLRP.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 64612: Added flag to pass VolumeProfileAdaptor module to SLRP.

[Joseph Wu]

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

(Updated Dec. 18, 2017, 6:35 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jie Yu.


Changes
---

Changed the injection method for the module.  This involves much fewer 
interface changes, but is also unlike most of our other patterns.


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


Repository: mesos


Description (updated)
---

This adds an agent flag to choose the VolumeProfileAdaptor module
to load and passes this information down to the storage resource
providers. A single module can potentially be used by multiple
storage resource providers.

We choose to pass the module along as a global variable because the
module is not needed by many of the components in the injection path.
Specifically, the Storage Local Resource Provider will need the module,
but its parents, the Local Resource Provider and Resource Provider
Daemons will not.


Diffs (updated)
-

  include/mesos/resource_provider/volume_profile.hpp PRE-CREATION 
  src/resource_provider/storage/provider.cpp 
158b6b408002209aa9a79a6772da30c984aad61a 
  src/resource_provider/volume_profile.cpp PRE-CREATION 
  src/slave/flags.hpp 5918484825c3c813883c42465f3e5a5693d6a2cd 
  src/slave/flags.cpp d4b95b7542733b22299bc19235cf99220534c022 
  src/slave/slave.hpp 75cc5836e39b2413eaefc5a5c9e905134db83743 
  src/slave/slave.cpp e69d42a2bfae09e2defcd8333c0434a9fd8bac4c 


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

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


Testing
---

make check

Basically just a plumbing change to pass the module into the SLRP.


Thanks,

Joseph Wu

Re: Review Request 64694: Cleaned up `ResourceProviderManagerHttpApiTest.ConvertResources`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64694 was successfully built and tested.

Reviews applied: `['64694']`

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

- Mesos Reviewbot Windows


On Dec. 19, 2017, 12:04 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64694/
> ---
> 
> (Updated Dec. 19, 2017, 12:04 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the test follow the pattern used by the Default
> Executor tests.
> 
> 
> Diffs
> -
> 
>   src/tests/resource_provider_manager_tests.cpp 
> 4e86016a450c30fea90e9c6e5c1f2f1aab5f42c0 
> 
> 
> Diff: https://reviews.apache.org/r/64694/diff/1/
> 
> 
> Testing
> ---
> 
> The test passed 500 times on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 64003: Made quota resource allocation fine-grained.

[Jie Yu]

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




src/master/allocator/mesos/hierarchical.cpp
Lines 1599 (patched)


RAW without volume_id is choppable.


- Jie Yu


On Dec. 12, 2017, 2:33 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64003/
> ---
> 
> (Updated Dec. 12, 2017, 2:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-7099
> https://issues.apache.org/jira/browse/MESOS-7099
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allocator now does fine-grained resource allocation up
> to the role’s quota limit.
> 
> Also fixed a few affected allocator tests.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> fccd71c96fe8e4d914e19b5ea8d8f69e9ebf2406 
>   src/tests/hierarchical_allocator_tests.cpp 
> f5fb47ed09682ebdd047aec7e79a86597ee09f53 
> 
> 
> Diff: https://reviews.apache.org/r/64003/diff/3/
> 
> 
> Testing
> ---
> 
> Fixed a few broken tests due to this patch.
> make check.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 64697: Windows: Deleted unused and unnecessary OS version functions.

[Andrew Schwartzmeyer]

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

Review request for mesos, Akash Gupta, Jeff Coffler, and Joseph Wu.


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


Repository: mesos


Description
---

The Windows API `GetVersionEx` was deprecated in Windows 8. Using it in
the `os.hpp` header caused hundreds of warnings that it was deprecated
to be emitted when building. While the function still exists, it stopped
returning the correct value when it was deprecated (so the version it
returns is 6.3, that is, Windows 8).

However, replacing it was less than straightforward. The recommended
replacement is to use the "version helper functions", but these are not
capable of providing the version information of the system. The intent
of the deprecation and the new APIs is to prevent developers from using
the version of Windows as a feature check. The new APIs are all of the
form `bool IsWindows10OrGreater()`. While it is possible to use
`IsWindowsServer()` to determine if we're on a client or server version
of Windows, no current user-mode API is provided to query the build
information of the OS. This is unfortunate, as retrieving this
information is a valid use case of the now deprecated API.

An explored alternative was to query the registry, but this was
discarded as it was not consistently available.

Another alternative was to parse the output of `systeminfo`, which can
return CSV formatted version information. However, we chose not to do
this currently as it is slow (on the order of seconds to invoke the
command).

There still exists a kernel-mode API to retrieve the version
information. However, to use it would entail either including WDK (which
is massive and not something we're going to do), or to invoke it
dynamically by getting the address from `nt.dll`. This is possible, but
it would be hacky, and was not necessary.

The chosen route was to simply delete the use of `GetVersionEx`. After
reviewing the use of these functions, it turned out to be entirely
possible to `delete` them.

Note that this means the entirety of `systems_tests.cpp` was rendered
pointless for Windows.


Diffs
-

  3rdparty/stout/include/stout/os.hpp 1a81db61faa55d7043d75a012fe1e66b49bf601c 
  3rdparty/stout/include/stout/windows/os.hpp 
26a1a049c7a4e1b6a3b6767a9c2c86e7538f6012 
  3rdparty/stout/tests/CMakeLists.txt 940fc9f6997695cf6c181ecbc72d6fd6e72dc7e7 
  3rdparty/stout/tests/os/systems_tests.cpp 
286d34edacab932aaecacfa6259a0bc549f01b1b 


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


Testing
---

`ctest -V -C Debug` on Windows 10, `make check` on CentOS 7.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 64574: Ensured trailing '/' in URL is insignificant.

[Benjamin Mahler]

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




3rdparty/libprocess/src/process.cpp
Lines 3530-3531 (patched)


This approach doesn't seem quite right to me, since it prevents the user of 
libprocess from seeing what the original path was. Have you considered any 
other approaches? (also I think you only needed to trim from the tail?)


- Benjamin Mahler


On Dec. 13, 2017, 1:39 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64574/
> ---
> 
> (Updated Dec. 13, 2017, 1:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5333
> https://issues.apache.org/jira/browse/MESOS-5333
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, adding a trailing '/' to a valid URL path, e.g.,
> "/state/", yielded a 404 response. This patch ensures that two URLs
> which differ only in trailing '/' produce the same result.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 9daac715f0242921b7f9f5c20b3eb27f1be802d4 
> 
> 
> Diff: https://reviews.apache.org/r/64574/diff/1/
> 
> 
> Testing
> ---
> 
> Ensured the modified test fails without the fix.
> `make check` on Mac OS 10.11.6 and several Linux distributions.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 64695: Improved the comments describing the offer operation states.

[Gaston Kleiman]

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

Review request for mesos, Greg Mann and Jie Yu.


Repository: mesos


Description
---

Improved the comments describing the offer operation states.


Diffs
-

  include/mesos/mesos.proto ddb9d368cc793a4cb865686e672ab05e91c9ec48 
  include/mesos/v1/mesos.proto 309079309cd1f6d5dc5a2e7de4e0f86f434f6451 


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


Testing
---


Thanks,

Gaston Kleiman

Re: Review Request 64658: Modified SLRP to use the VolumeProfileAdaptor module.

[Chun-Hung Hsiao]

> On Dec. 19, 2017, 12:12 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2587-2588 (patched)
> > 
> >
> > Maybe we need a helper here?
> > 
> > `Result getDiskProfile(const Resource& resource)`

Alternatively, I'll have an `Option source` and do 
`source->disk().source().has_profile()`.
P.S. I don't feel that we need to do `has_disk()` and `disk().has_source()`, 
because the protobuf library will return an empty `Source` for 
`.disk().source()` if these messages don't exist.


- Chun-Hung


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


On Dec. 18, 2017, 2:25 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64658/
> ---
> 
> (Updated Dec. 18, 2017, 2:25 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes the Storage Local Resource Provider's source of profile
> information from only using the default to using a VolumeProfileAdaptor
> module.
> 
> This patch is based on https://reviews.apache.org/r/64616.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 158b6b408002209aa9a79a6772da30c984aad61a 
> 
> 
> Diff: https://reviews.apache.org/r/64658/diff/6/
> 
> 
> Testing
> ---
> 
> See later in chain. This patch will fail SLRP tests. They are fixed in a 
> followup patch.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64033: Terminated driver-based executors if kill arrives before launch task.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64033 was successfully built and tested.

Reviews applied: `['64032', '64069', '64070', '64033']`

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

- Mesos Reviewbot Windows


On Dec. 4, 2017, 6:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64033/
> ---
> 
> (Updated Dec. 4, 2017, 6:04 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Anand Mazumdar, Armand Grillet, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-8297
> https://issues.apache.org/jira/browse/MESOS-8297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `ExecutorRegisteredMessage` or `RunTaskMessage` may not be delivered
> to a driver-based executor. Since these messages are not retried,
> without this patch an executor never starts a task and remains idle,
> ignoring kill task request. This patch ensures all built-in driver-
> based executors eventually shut down if kill task arrives before
> the task has been started.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 3974f20052e3c12eb154a5146d19d4dc1759859f 
>   src/exec/exec.cpp cdbf149a9047ddad6beef64be1266e15e7643afc 
>   src/launcher/executor.cpp c688c04e598ac140421fd5e47359b0e48d30bcc5 
> 
> 
> Diff: https://reviews.apache.org/r/64033/diff/4/
> 
> 
> Testing
> ---
> 
> make check on MacOS 10.11.6
> 
> Manual testing using modified "exec.cpp" that drops executor registration 
> confirmation.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 64353: Added example VolumeProfile module.

[Jie Yu]

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


Fix it, then Ship it!





src/csi/uri_volume_profile.proto
Lines 21 (patched)


I'd suggest we remove `uri_volume_profile` from the package name  given the 
message name is pretty clear.


- Jie Yu


On Dec. 18, 2017, 11:54 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64353/
> ---
> 
> (Updated Dec. 18, 2017, 11:54 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
> 
> 
> Bugs: MESOS-8251
> https://issues.apache.org/jira/browse/MESOS-8251
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This example module shows how a VolumeProfile module might be
> implemented (and is a viable module in its own right).  The module
> can be configured to fetch a map of profiles from a URI (`file://` or
> `http(s)://`) and possibly cache this item for some time.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f5a4edd245e8d6535502e951ecaa526c2bae25f9 
>   src/csi/uri_volume_profile.proto PRE-CREATION 
>   src/resource_provider/uri_volume_profile.hpp PRE-CREATION 
>   src/resource_provider/uri_volume_profile.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64353/diff/7/
> 
> 
> Testing
> ---
> 
> See end of chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 64590: Stopped logging optional fields unconditionally in agent handler.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64590 was successfully built and tested.

Reviews applied: `['64590']`

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

- Mesos Reviewbot Windows


On Dec. 18, 2017, 11:24 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64590/
> ---
> 
> (Updated Dec. 18, 2017, 11:24 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `operation_id` and `framework_id` fields are optional, so they
> should only be logged if set.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e69d42a2bfae09e2defcd8333c0434a9fd8bac4c 
> 
> 
> Diff: https://reviews.apache.org/r/64590/diff/4/
> 
> 
> Testing
> ---
> 
> `make check` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 64658: Modified SLRP to use the VolumeProfileAdaptor module.

[Jie Yu]

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




src/resource_provider/storage/provider.cpp
Lines 897-912 (patched)


Looks like this is not needed to me?



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


what if `watch` failed? at least print some error message?



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


Looks like this function can be called in any state?



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


remove the extra space before `=`



src/resource_provider/storage/provider.cpp
Lines 2587-2588 (patched)


Maybe we need a helper here?

`Result getDiskProfile(const Resource& resource)`


- Jie Yu


On Dec. 18, 2017, 2:25 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64658/
> ---
> 
> (Updated Dec. 18, 2017, 2:25 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes the Storage Local Resource Provider's source of profile
> information from only using the default to using a VolumeProfileAdaptor
> module.
> 
> This patch is based on https://reviews.apache.org/r/64616.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 158b6b408002209aa9a79a6772da30c984aad61a 
> 
> 
> Diff: https://reviews.apache.org/r/64658/diff/6/
> 
> 
> Testing
> ---
> 
> See later in chain. This patch will fail SLRP tests. They are fixed in a 
> followup patch.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 64694: Cleaned up `ResourceProviderManagerHttpApiTest.ConvertResources`.

[Gaston Kleiman]

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

Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.


Repository: mesos


Description
---

This patch makes the test follow the pattern used by the Default
Executor tests.


Diffs
-

  src/tests/resource_provider_manager_tests.cpp 
4e86016a450c30fea90e9c6e5c1f2f1aab5f42c0 


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


Testing
---

The test passed 500 times on GNU/Linux.


Thanks,

Gaston Kleiman

Re: Review Request 64664: Updated logging for storage local resource provider.

[Chun-Hung Hsiao]

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

(Updated Dec. 18, 2017, 11:57 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Updated logging for storage local resource provider.


Diffs (updated)
-

  src/resource_provider/manager.cpp 2a167aa5c50598758cab4ab6c13f1b1e33e692dd 
  src/resource_provider/storage/provider.cpp 
158b6b408002209aa9a79a6772da30c984aad61a 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 64354: Added tests for UriVolumeProfile module.

[Joseph Wu]

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

(Updated Dec. 18, 2017, 3:55 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jie Yu.


Changes
---

Updated test strings (the JSON blobs) to match the scheme change in the 
previous review.


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


Repository: mesos


Description
---

These tests mostly exercise the parsing logic used by the module
and some of the interaction expected from callers of the module
(i.e. the Storage Local Resource Provider).


Diffs (updated)
-

  src/Makefile.am f5a4edd245e8d6535502e951ecaa526c2bae25f9 
  src/tests/volume_profile_tests.cpp PRE-CREATION 


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

Changes: https://reviews.apache.org/r/64354/diff/3-4/


Testing
---

make check


Thanks,

Joseph Wu

Re: Review Request 64353: Added example VolumeProfile module.

[Joseph Wu]

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

(Updated Dec. 18, 2017, 3:54 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jie Yu.


Changes
---

Replaced most of the custom parsing code with the Proto3 JSON->Protobuf utility 
method.


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


Repository: mesos


Description
---

This example module shows how a VolumeProfile module might be
implemented (and is a viable module in its own right).  The module
can be configured to fetch a map of profiles from a URI (`file://` or
`http(s)://`) and possibly cache this item for some time.


Diffs (updated)
-

  src/Makefile.am f5a4edd245e8d6535502e951ecaa526c2bae25f9 
  src/csi/uri_volume_profile.proto PRE-CREATION 
  src/resource_provider/uri_volume_profile.hpp PRE-CREATION 
  src/resource_provider/uri_volume_profile.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/64353/diff/7/

Changes: https://reviews.apache.org/r/64353/diff/6-7/


Testing
---

See end of chain.


Thanks,

Joseph Wu

Re: Review Request 64590: Stopped logging optional fields unconditionally in agent handler.

[Gaston Kleiman]

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

(Updated Dec. 18, 2017, 3:24 p.m.)


Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jie Yu.


Changes
---

Addressed feedback.


Repository: mesos


Description
---

The `operation_id` and `framework_id` fields are optional, so they
should only be logged if set.


Diffs (updated)
-

  src/slave/slave.cpp e69d42a2bfae09e2defcd8333c0434a9fd8bac4c 


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

Changes: https://reviews.apache.org/r/64590/diff/3-4/


Testing
---

`make check` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 64033: Terminated driver-based executors if kill arrives before launch task.

[Vinod Kone]

> On Dec. 5, 2017, 2:17 a.m., Anand Mazumdar wrote:
> > src/exec/exec.cpp
> > Lines 350-359 (patched)
> > 
> >
> > hmm, wondering how is this even possible? We explicitly `link()` the 
> > executor's PID when the executor registers with the agent. So all messages 
> > from the agent to the executor are sent on a persistent connection.
> > 
> > This means that the following can _only happen_ when the initial 
> > connection between the agent and the executor broke and instead it did not 
> > use a non-persistent socket. We currently don't do anything in the 
> > `exited()` function of the agent when an executor exits. We should rather 
> > shutdown the executor if we notice that the connection breaks.

Chatted with Anand offline. Can you link the JIRA here because the root cause 
on when this happens is still unknown?


- Vinod


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


On Dec. 4, 2017, 6:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64033/
> ---
> 
> (Updated Dec. 4, 2017, 6:04 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Anand Mazumdar, Armand Grillet, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-8297
> https://issues.apache.org/jira/browse/MESOS-8297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `ExecutorRegisteredMessage` or `RunTaskMessage` may not be delivered
> to a driver-based executor. Since these messages are not retried,
> without this patch an executor never starts a task and remains idle,
> ignoring kill task request. This patch ensures all built-in driver-
> based executors eventually shut down if kill task arrives before
> the task has been started.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 3974f20052e3c12eb154a5146d19d4dc1759859f 
>   src/exec/exec.cpp cdbf149a9047ddad6beef64be1266e15e7643afc 
>   src/launcher/executor.cpp c688c04e598ac140421fd5e47359b0e48d30bcc5 
> 
> 
> Diff: https://reviews.apache.org/r/64033/diff/4/
> 
> 
> Testing
> ---
> 
> make check on MacOS 10.11.6
> 
> Manual testing using modified "exec.cpp" that drops executor registration 
> confirmation.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 64033: Terminated driver-based executors if kill arrives before launch task.

[Vinod Kone]

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




src/docker/executor.cpp
Lines 389 (patched)


Let's do a `CHECK_SOME(run)` because this is not expected to happen as far 
as we can tell. Can you link the JIRA for posterity?



src/launcher/executor.cpp
Lines 781 (patched)


ditto. do a CHECK here.


- Vinod Kone


On Dec. 4, 2017, 6:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64033/
> ---
> 
> (Updated Dec. 4, 2017, 6:04 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Anand Mazumdar, Armand Grillet, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-8297
> https://issues.apache.org/jira/browse/MESOS-8297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `ExecutorRegisteredMessage` or `RunTaskMessage` may not be delivered
> to a driver-based executor. Since these messages are not retried,
> without this patch an executor never starts a task and remains idle,
> ignoring kill task request. This patch ensures all built-in driver-
> based executors eventually shut down if kill task arrives before
> the task has been started.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 3974f20052e3c12eb154a5146d19d4dc1759859f 
>   src/exec/exec.cpp cdbf149a9047ddad6beef64be1266e15e7643afc 
>   src/launcher/executor.cpp c688c04e598ac140421fd5e47359b0e48d30bcc5 
> 
> 
> Diff: https://reviews.apache.org/r/64033/diff/4/
> 
> 
> Testing
> ---
> 
> make check on MacOS 10.11.6
> 
> Manual testing using modified "exec.cpp" that drops executor registration 
> confirmation.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 64070: Ensured executor adapter propagates error and shutdown messages.

[Vinod Kone]

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




src/executor/v0_v1executor.cpp
Lines 155 (patched)


Is it safe for the executor to get `connected` callback after it is already 
connected? I'm thinking of the case where connect was just called, but the 
executor hasn't send a SUBSCRIBE call yet and we are in this situation. Just 
want to make sure.

Also, can you add a comment here explaining why you are doing a `connected` 
here for posterity?


- Vinod Kone


On Dec. 4, 2017, 6:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64070/
> ---
> 
> (Updated Dec. 4, 2017, 6:04 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Anand Mazumdar, Armand Grillet, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, if an error or shutdown occurred during
> subscription / registration with the agent, it was not propagated back
> to the executor if the v0_v1 executor adapter was used. This happened
> because the adapter did not call the `connected` callback until after
> successful registration and hence the executor did not even try to
> send the `SUBSCRIBE` call, without which the adapter did not send any
> events to the executor.
> 
> A fix is to call the `connected` callback if an error occurred or
> shutdown even arrived before the executor had subscribed.
> 
> 
> Diffs
> -
> 
>   src/executor/v0_v1executor.cpp 61d591993e6388ba3b4d64a3bdb63c3a3513fbeb 
> 
> 
> Diff: https://reviews.apache.org/r/64070/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/64033/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 64069: Ensured command executor always honors shutdown request.

[Vinod Kone]

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



What about the fix for other built-in executors (docker, default)?

Also, this only seems to fix the shutdown path, what about kill task path?


Regaring the fix for kill task when a task hasn't been launched from the 
perspective of the executor, I think doing a CHECK fail makes sense to me 
because there was violation of an external variant (kill comes after launch). 
Silently shutting down in such a scenario seems risky because it will mask the 
extent of the problem. Either way we need the system to heal and correct itself 
when variants fail; right now the executor ignores the kill and stays up which 
is a really bad UX. cc @anand

- Vinod Kone


On Dec. 15, 2017, 10:43 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64069/
> ---
> 
> (Updated Dec. 15, 2017, 10:43 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Anand Mazumdar, Armand Grillet, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp c688c04e598ac140421fd5e47359b0e48d30bcc5 
> 
> 
> Diff: https://reviews.apache.org/r/64069/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/64033/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 64685: Improved the comment around the union in ns::clone().

[Armand Grillet]

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


Ship it!




Ship It!

- Armand Grillet


On Dec. 18, 2017, 7:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64685/
> ---
> 
> (Updated Dec. 18, 2017, 7:06 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Benjamin Bannier, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/linux/ns.cpp 5e2df1ed56432c6c1bfa04a31dba7f9f547d6139 
> 
> 
> Diff: https://reviews.apache.org/r/64685/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/64686/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 64507: Added a master flag to disallow agents without domain.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 13, 2017, 2:49 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64507/
> ---
> 
> (Updated Dec. 13, 2017, 2:49 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-8115
> https://issues.apache.org/jira/browse/MESOS-8115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new `--require_agent_domain` flag and implementation. When
> set to true, it will cause the master to refuse (re-)registration
> attempts for agents with no configured domain.
> 
> This is intended as a safety net for operators, who could forget to
> configure the fault domain of a remote agent and let it join the
> cluster. If this happens, an agent in a remote region will be
> considered a local agent by the master and frameworks (because agent's
> fault domain is not configured), causing tasks to potentially land in a
> remote agent which is undesirable.
> 
> 
> Diffs
> -
> 
>   docs/configuration/master.md 6af16a08257680963884d07a325803487f677c37 
>   src/master/flags.hpp edda71af41a8d199bb24b84487300e9c85b12fec 
>   src/master/flags.cpp 18f405b7e9cc6bd0011284b631974eee52dd2bf3 
>   src/master/master.cpp b3e074cfe86600793310deb87932fa145e95055d 
>   src/tests/master_tests.cpp 7b087675200fe0ca69319b715997700a91d9f6b2 
> 
> 
> Diff: https://reviews.apache.org/r/64507/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64506: Correctly reset slave status when aborting a registration.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 14, 2017, 3:32 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64506/
> ---
> 
> (Updated Dec. 14, 2017, 3:32 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8341
> https://issues.apache.org/jira/browse/MESOS-8341
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the slave was not erased from the `registering`
> and `reregistering` sets in the master for some code paths
> that would result in a failed (re-)registration attempt.
> 
> This could lead to a state where the reason of the unsuccessful
> (re-)registration attempt is fixed on the agent, but the master
> ignores subsequent attempts because it assumes the previous
> operation is still in progress.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b3e074cfe86600793310deb87932fa145e95055d 
>   src/tests/master_tests.cpp 7b087675200fe0ca69319b715997700a91d9f6b2 
> 
> 
> Diff: https://reviews.apache.org/r/64506/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64686: Fixed the type-punned pointer and strict aliasing issue.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64686 was successfully built and tested.

Reviews applied: `['64685', '64686']`

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

- Mesos Reviewbot Windows


On Dec. 18, 2017, 7:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64686/
> ---
> 
> (Updated Dec. 18, 2017, 7:06 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Benjamin Bannier, and Michael Park.
> 
> 
> Bugs: MESOS-6616
> https://issues.apache.org/jira/browse/MESOS-6616
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/linux/ns.cpp 5e2df1ed56432c6c1bfa04a31dba7f9f547d6139 
> 
> 
> Diff: https://reviews.apache.org/r/64686/diff/1/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.11.6 and various Linux distros
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 64590: Stopped logging optional fields unconditionally in agent handler.

[Greg Mann]

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


Fix it, then Ship it!





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


Nit: could we use operation_uuid here instead? Otherwise it's ambiguous if 
this UUID is for the status update or the operation.


- Greg Mann


On Dec. 14, 2017, 6:22 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64590/
> ---
> 
> (Updated Dec. 14, 2017, 6:22 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `operation_id` and `framework_id` fields are optional, so they
> should only be logged if set.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 9d0e9debf2ecd142eb6179c12ef4a0ada24b1c6c 
> 
> 
> Diff: https://reviews.apache.org/r/64590/diff/3/
> 
> 
> Testing
> ---
> 
> `make check` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 64683: Updated support/mesos-style.py to build the virtualenv less often.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64683 was successfully built and tested.

Reviews applied: `['64683']`

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

- Mesos Reviewbot Windows


On Dec. 18, 2017, 8:35 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64683/
> ---
> 
> (Updated Dec. 18, 2017, 8:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benno Evers.
> 
> 
> Bugs: MESOS-8217
> https://issues.apache.org/jira/browse/MESOS-8217
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of just checking if the virtual environment exists, we now
> also check if it is required to lint the modified files or not.
> 
> The virtual environment will thus be built when using the script as
> a git hook only if JavaScript or Python files have been modified.
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 315955e3533fce0cbf820fad03b3707049cdffe0 
> 
> 
> Diff: https://reviews.apache.org/r/64683/diff/1/
> 
> 
> Testing
> ---
> 
> Tested the commit hook with multiple combinations and checked that the result 
> was the one expected.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Review Request 64685: Improved the comment around the union in ns::clone().

[Alexander Rukletsov]

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

Review request for mesos, Armand Grillet, Benjamin Bannier, and Michael Park.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/linux/ns.cpp 5e2df1ed56432c6c1bfa04a31dba7f9f547d6139 


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


Testing
---

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


Thanks,

Alexander Rukletsov

Review Request 64686: Fixed the type-punned pointer and strict aliasing issue.

[Alexander Rukletsov]

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

Review request for mesos, Armand Grillet, Benjamin Bannier, and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/linux/ns.cpp 5e2df1ed56432c6c1bfa04a31dba7f9f547d6139 


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


Testing
---

make check on Mac OS 10.11.6 and various Linux distros


Thanks,

Alexander Rukletsov

Review Request 64683: Updated support/mesos-style.py to build the virtualenv less often.

[Armand Grillet]

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

Review request for mesos, Alexander Rukletsov and Benno Evers.


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


Repository: mesos


Description
---

Instead of just checking if the virtual environment exists, we now
also check if it is required to lint the modified files or not.

The virtual environment will thus be built when using the script as
a git hook only if JavaScript or Python files have been modified.


Diffs
-

  support/mesos-style.py 315955e3533fce0cbf820fad03b3707049cdffe0 


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


Testing
---

Tested the commit hook with multiple combinations and checked that the result 
was the one expected.


Thanks,

Armand Grillet

Re: Review Request 64680: Fixed flaky `ROOT_CGROUPS_RecoverLauncherOrphans` test.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 64680 was successfully built and tested.

Reviews applied: `['64680']`

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

- Mesos Reviewbot Windows


On Dec. 18, 2017, 12:44 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64680/
> ---
> 
> (Updated Dec. 18, 2017, 12:44 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gilbert Song.
> 
> 
> Bugs: MESOS-8267
> https://issues.apache.org/jira/browse/MESOS-8267
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Containerizer recovery returns control to the caller before completion
> of destruction of orphaned containers. Previously, `wait` was called on
> a container right after calling `recover`, so `wait` was almost always
> successfull, because destruction of the orphaned container takes some
> time to complete.
> 
> This patch replaces check for the container existence with the check
> that a related freezer cgroup has been destroyed. The freezer cgroup
> is destroyed during container destruction initiated by a containerizer
> recovery process.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 22bd9955140ef1c75d8bb78faa6e6d50a71c2318 
> 
> 
> Diff: https://reviews.apache.org/r/64680/diff/1/
> 
> 
> Testing
> ---
> 
> 1. make check
> 2. `./src/mesos-tests 
> --gtest_filter=NestedMesosContainerizerTest.ROOT_CGROUPS_RecoverLauncherOrphans
>  --gtest_break_on_failure --gtest_repeat=1000`
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Review Request 64680: Fixed flaky `ROOT_CGROUPS_RecoverLauncherOrphans` test.

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov and Gilbert Song.


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


Repository: mesos


Description
---

Containerizer recovery returns control to the caller before completion
of destruction of orphaned containers. Previously, `wait` was called on
a container right after calling `recover`, so `wait` was almost always
successfull, because destruction of the orphaned container takes some
time to complete.

This patch replaces check for the container existence with the check
that a related freezer cgroup has been destroyed. The freezer cgroup
is destroyed during container destruction initiated by a containerizer
recovery process.


Diffs
-

  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
22bd9955140ef1c75d8bb78faa6e6d50a71c2318 


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


Testing
---

1. make check
2. `./src/mesos-tests 
--gtest_filter=NestedMesosContainerizerTest.ROOT_CGROUPS_RecoverLauncherOrphans 
--gtest_break_on_failure --gtest_repeat=1000`


Thanks,

Andrei Budnik

Re: Review Request 64506: Correctly reset slave status when aborting a registration.

[Benno Evers]

> On Dec. 14, 2017, 9:37 p.m., Benjamin Mahler wrote:
> > Could you please file a ticket that describes the bug from what a user 
> > would experience, and link that in to the review? I would like to target it 
> > for backporting, seems pretty bad.

Here you go: https://issues.apache.org/jira/browse/MESOS-8341

I think it looks worse than it is in practice, most of these code paths will 
almost never be hit, and it can be worked-around with a master restart.


- Benno


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


On Dec. 14, 2017, 3:32 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64506/
> ---
> 
> (Updated Dec. 14, 2017, 3:32 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the slave was not erased from the `registering`
> and `reregistering` sets in the master for some code paths
> that would result in a failed (re-)registration attempt.
> 
> This could lead to a state where the reason of the unsuccessful
> (re-)registration attempt is fixed on the agent, but the master
> ignores subsequent attempts because it assumes the previous
> operation is still in progress.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b3e074cfe86600793310deb87932fa145e95055d 
>   src/tests/master_tests.cpp 7b087675200fe0ca69319b715997700a91d9f6b2 
> 
> 
> Diff: https://reviews.apache.org/r/64506/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 63953: Added logging based on container class.

[Armand Grillet]

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

(Updated Dec. 18, 2017, 10:05 a.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Fixed issues.


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


Repository: mesos


Description
---

This change adjusts log level based on the container class.
If the class is `DEBUG` we print the log entry at a verbose
level 1, otherwise we print it at the `INFO` level.

We use the added macro in mesos containerizer so that COMMAND
checks cause less INFO logs (15 lines instead of 26 before).


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
85cb325fba2e49b78a1f381183a61f03ccd5194a 
  src/slave/containerizer/mesos/containerizer.cpp 
1a398a8ea7f3e49273459b318ae2e8a3eaae2d9c 


Diff: https://reviews.apache.org/r/63953/diff/9/

Changes: https://reviews.apache.org/r/63953/diff/8-9/


Testing
---

Started a Mesos cluster and used `mesos-execute` with this task group to check 
that the behaviour after this patch is the one expected:

```
{
  "tasks": [
{
  "name": "Name of the task",
  "task_id": {
"value": "task-group"
  },
  "agent_id": {
"value": ""
  },
  "resources": [
{
  "name": "cpus",
  "type": "SCALAR",
  "scalar": {
"value": 0.01
  }
},
{
  "name": "mem",
  "type": "SCALAR",
  "scalar": {
"value": 2
  }
}
  ],
  "command": {
"value": "sleep 1000"
  },
  "check": {
"type": "COMMAND",
"command": {
  "command": {
"value": "echo \"Bonjour\""
  },
  "uris": []
}
  }
}
  ]
}
```

And:
```
$ nice make check
```


Thanks,

Armand Grillet

Re: Review Request 64575: Added containerClass() to MesosContainerizerProcess::Container.

[Armand Grillet]

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

(Updated Dec. 18, 2017, 10:03 a.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Fixed issues.


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


Repository: mesos


Description
---

This function always returns a ContainerClass, `DEFAULT` being the
default value returned. Also simplifies the conditional statements
in mesos/containerizer.cpp to use the function added.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
85cb325fba2e49b78a1f381183a61f03ccd5194a 
  src/slave/containerizer/mesos/containerizer.cpp 
1a398a8ea7f3e49273459b318ae2e8a3eaae2d9c 


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

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


Testing
---

```
GTEST_FILTER="" nice make check -j16 V=0
```


Thanks,

Armand Grillet