date:20171220

Re: Review Request 64762: Renamed offer operation to operation.

2017-12-20 Thread Jie Yu


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


Ship it!




Ship It!

- Jie Yu


On Dec. 21, 2017, 2:25 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64762/
> ---
> 
> (Updated Dec. 21, 2017, 2:25 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed offer operation to operation.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp 4d012ccb65af58dcac2c945bff304176a4141408 
>   include/mesos/v1/mesos.hpp d163f0bf79a570f6f61a5c16310b1683a945b3ef 
>   src/CMakeLists.txt 5b541530035092794f6a98a1be299f652c014877 
>   src/Makefile.am 5c6f180aed33969a82b28f74d42d2e2608548501 
>   src/cli/execute.cpp 13544613d0ce05d12e426e66c5996c05f525e8fb 
>   src/common/protobuf_utils.hpp e112f1984ed39137b9dfec542b4cfd0d358bfe1a 
>   src/common/protobuf_utils.cpp f4380e94cdafb3f05c45d0d507f63bfbe40d8317 
>   src/common/resources_utils.cpp 50651dcf2942aecc1564442f14afbd90ee91cb52 
>   src/common/type_utils.cpp 2ba5302e2577099a39aef2ab84e02b6db2c1faaf 
>   src/examples/long_lived_framework.cpp 
> bb9fc2d775004ed237a0374f193b4c5fa731afc7 
>   src/examples/test_http_framework.cpp 
> e83aded0e98d8c92472c6122e52a581bcb389aa6 
>   src/internal/devolve.hpp 9e56fd974161fe87a172e3376df41fe18e5b8204 
>   src/internal/devolve.cpp 60a768dd9ee382fd55fad0892bead50cf15ef251 
>   src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
> d28ae5e540ec739a831df468cebe876b6356afe8 
>   src/master/http.cpp ec170a257c2d309712f3c4b2fce756eb0b530ad6 
>   src/master/maintenance.hpp b1e176c0b792bdb2a8e7175a14d5d9ab6a90729b 
>   src/master/master.hpp b800cda3c82ba7c471147889075cdc4f915f16b1 
>   src/master/master.cpp cf8a22b636210e9f8d326592d8b64eab2c97caa4 
>   src/master/quota.hpp 479112d1a94ede34e2d46a27ff03999fc1634b8c 
>   src/master/quota_handler.cpp 26b2dc86fa44dfdf6e0a6c4993c754a6384dd851 
>   src/master/registrar.hpp c439f6a11274c081059d666f07ab0ddc47208a7d 
>   src/master/registrar.cpp 488b0896b8bfbb02583d614376053bde35156dee 
>   src/master/registry_operations.hpp 06f68a3112e00b39e228d2c60b880259a07988b5 
>   src/master/validation.cpp 7f5a67d98dbbd5a5d64bc73d530f3d37b3cdfec6 
>   src/master/weights.hpp 4b9c438585c3464a46c5f6b4b114759bd430d7fa 
>   src/master/weights_handler.cpp a4d2fed758878f3e2b9557a61965816aa9e0399c 
>   src/messages/messages.hpp 8e428933bbd96c7a09c9cd703169e0d15404bd82 
>   src/messages/messages.cpp ac8f6c35556ccc04fb94c76b4c255f7ebff449b7 
>   src/resource_provider/manager.hpp c5c2d5297b787822411e83dc8279e7e0fd8f572c 
>   src/resource_provider/manager.cpp e3fcb64b630924e1bb497625708cad3f0fdc064a 
>   src/resource_provider/message.hpp 6236693f1a6a53d347020ffec3e303843f431d3a 
>   src/resource_provider/registrar.cpp 
> ef66c126d3127481388a3c4691538d4bd07e6c1d 
>   src/resource_provider/storage/provider.cpp 
> 833d929e93afd0bb99a9574df24168e82269bc81 
>   src/resource_provider/validation.cpp 
> 5b7de741b866edf90fc5c990378fd65a4a551fad 
>   src/sched/sched.cpp cce4633963bb253fe697e846c4a55abc465eb04e 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp 
> bbbe766f3770b5640e21bcedee7a9395c6dac412 
>   src/slave/paths.hpp 9cbacd8da62e7c7386dca7031fc09a46ae773161 
>   src/slave/paths.cpp fca2a0eec2a75ed76028ea54dc992502275d4bce 
>   src/slave/slave.hpp 9e2663e631c07831423fea9086301c07f80c42e4 
>   src/slave/slave.cpp 6271cfe56733ec121d0d7c691e3dd0792435bf2d 
>   src/status_update_manager/offer_operation.hpp 
> 0ab6be4c6d57410460e0cf4422f61cb2ff28a7db 
>   src/status_update_manager/offer_operation.cpp 
> 9df4973d5d2d331ce93dc82f9b1dc35ae8ea11b8 
>   src/status_update_manager/status_update_manager_process.hpp 
> 4a23573f65d54468319bb2f559593d0866ee299b 
>   src/tests/CMakeLists.txt 42387f7e1aa6ffb7f307ac53d69a1de92d68afa6 
>   src/tests/master_tests.cpp 60718b4f76f52064fe550229db4ebbdff2c4b64f 
>   src/tests/mesos.hpp 41f47cf1b10fddd51e260e2127e2695ffe1aeba5 
>   src/tests/mock_registrar.hpp 92c39940a79a40da9ee222b05b810f11bb70bb58 
>   src/tests/mock_registrar.cpp 0a877b20af7ad693a41e1b8585066d5d43065344 
>   src/tests/offer_operation_status_update_manager_tests.cpp 
> 37bea36c4306411c82824e8d9a4a214d453692e5 
>   src/tests/partition_tests.cpp 9fcc0a8aae034808ff222150d7068e9453e12160 
>   src/tests/persistent_volume_tests.cpp 
> 854676918dba497e1a8c21d877631c6e5c75ee2e 
>   src/tests/reconciliation_tests.cpp 9ee91c9df1017e3884ba280cccbc4614aa41acf7 
>   src/tests/registrar_tests.cpp 34365c799be599fda1681cfc61ec9b7d00296b5b 
>   src/tests/reservation_tests.cpp 34fa44188c4f220b050f2a1ff968cd76de523426 
>   src/tests/resource_provider_manager

Re: Review Request 64763: Fixed compilation for python eggs when gRPC is enabled.

2017-12-20 Thread Mesos Reviewbot Windows


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



PASS: Mesos patch 64763 was successfully built and tested.

Reviews applied: `['64754', '64755', '64756', '64763']`

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

- Mesos Reviewbot Windows


On Dec. 21, 2017, 3:41 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64763/
> ---
> 
> (Updated Dec. 21, 2017, 3:41 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Kapil Arya.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed compilation for python eggs when gRPC is enabled.
> 
> 
> Diffs
> -
> 
>   src/python/native_common/ext_modules.py.in 
> 47c2d24a6e5e49ad5860d96108bbb1e2eeb8dd5b 
> 
> 
> Diff: https://reviews.apache.org/r/64763/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64765: Removed a stale TODO in the allocator.

2017-12-20 Thread Mesos Reviewbot Windows

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

FAIL: Some Mesos libprocess-tests failed.

Reviews applied: `['64765']`

Failed command: 
`D:\DCOS\mesos\3rdparty\libprocess\src\tests\Debug\libprocess-tests.exe`

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

Relevant logs:

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

```
[ RUN  ] SSLVerifyIPAdd/SSLTest.RequireCertificate/0
D:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\ssl_tests.cpp(446): error: 
(await_subprocess(client.get(), 0)).failure(): 
[++] Subprocess output.
[==] Running 1 test from 1 test case.

[--] Global test environment set-up.

[--] 1 test from SSLClientTest

[ RUN  ] SSLClientTest.client

[   OK ] SSLClientTest.client (8 ms)

[--] 1 test from SSLClientTest (9 ms total)

[--] Global test environment tear-down

[==] 1 test from 1 test case ran. (9 ms total)

[  PASSED  ] 1 test.

[++]

[  FAILED  ] SSLVerifyIPAdd/SSLTest.RequireCertificate/0, where GetParam() = 
"false" (1083 ms)
[ RUN  ] SSLVerifyIPAdd/SSLTest.RequireCertificate/1
[   OK ] SSLVerifyIPAdd/SSLTest.RequireCertificate/1 (1071 ms)
[--] 4 tests from SSLVerifyIPAdd/SSLTest (3171 ms total)

[--] Global test environment tear-down
[==] 223 tests from 35 test cases ran. (36579 ms total)
[  PASSED  ] 222 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] SSLVerifyIPAdd/SSLTest.RequireCertificate/0, where GetParam() = 
"false"

 1 FAILED TEST
  YOU HAVE 19 DISABLED TESTS

```

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

```
LIBPROCESS_SSL_VERIFY_CERT set to true
I1221 04:21:19.810196  6652 openssl.cpp:563] Using CA file: 
C:\Users\mesos\AppData\Local\Temp\0qptEg\cert.pem
WARNING: Logging before InitGoogleLogging() is written to STDERR
I1221 04:21:21.022442  7396 openssl.cpp:509] CA directory path unspecified! 
NOTE: Set CA directory path with LIBPROCESS_SSL_CA_DIR=
I1221 04:21:21.023442  7396 openssl.cpp:514] Will not verify peer certificate!
NOTE: Set LIBPROCESS_SSL_VERIFY_CERT=1 to enable peer certificate verification
I1221 04:21:21.023442  7396 openssl.cpp:526] Will use IP address verification 
in subject alternative name certificate extension.
I1221 04:21:21.023442  7396 openssl.cpp:534] LIBPROCESS_SSL_REQUIRE_CERT 
implies peer certificate verification.
LIBPROCESS_SSL_VERIFY_CERT set to true
I1221 04:21:21.023442  7396 openssl.cpp:563] Using CA file: 
C:\Users\mesos\AppData\Local\Temp\r4xmr1\cert.pem
bject alternative name certificate extension.
I1221 04:21:18.947185  8496 openssl.cpp:534] LIBPROCESS_SSL_REQUIRE_CERT 
implies peer certificate verification.
LIBPROCESS_SSL_VERIFY_CERT set to true
I1221 04:21:18.947185  8496 openssl.cpp:563] Using CA file: 
C:\Users\mesos\AppData\Local\Temp\sDeMmo\cert.pem
I1221 04:21:18.947185  8496 openssl.cpp:566] Using CA dir: 
C:\Users\mesos\AppData\Local\Temp\sDeMmo
I1221 04:21:19.277227  8496 openssl.cpp:509] CA directory path unspecified! 
NOTE: Set CA directory path with LIBPROCESS_SSL_CA_DIR=
I1221 04:21:19.277227  8496 openssl.cpp:514] Will not verify peer certificate!
NOTE: Set LIBPROCESS_SSL_VERIFY_CERT=1 to enable peer certificate verification
I1221 04:21:19.277227  8496 openssl.cpp:534] LIBPROCESS_SSL_REQUIRE_CERT 
implies peer certificate verification.
LIBPROCESS_SSL_VERIFY_CERT set to true
I1221 04:21:19.278228  8496 openssl.cpp:563] Using CA file: 
C:\Users\mesos\AppData\Local\Temp\0qptEg\cert.pem
I1221 04:21:20.490447  8496 openssl.cpp:509] CA directory path unspecified! 
NOTE: Set CA directory path with LIBPROCESS_SSL_CA_DIR=
I1221 04:21:20.490447  8496 openssl.cpp:514] Will not verify peer certificate!
NOTE: Set LIBPROCESS_SSL_VERIFY_CERT=1 to enable peer certificate verification
I1221 04:21:20.490447  8496 openssl.cpp:526] Will use IP address verification 
in subject alternative name certificate extension.
I1221 04:21:20.490447  8496 openssl.cpp:534] LIBPROCESS_SSL_REQUIRE_CERT 
implies peer certificate verification.
LIBPROCESS_SSL_VERIFY_CERT set to true
I1221 04:21:20.490447  8496 openssl.cpp:563] Using CA file: 
C:\Users\mesos\AppData\Local\Temp\r4xmr1\cert.pem
I1221 04:21:21.135428  1468 process.cpp:887] Failed to accept socket: future 
discarded
```

- Mesos Reviewbot Windows

On Dec. 21, 2017, 3:49 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64765/
> ---
> 
> (Updated Dec. 21, 2017, 3:49 a.m.)

Re: Review Request 64762: Renamed offer operation to operation.

2017-12-20 Thread Mesos Reviewbot Windows


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



PASS: Mesos patch 64762 was successfully built and tested.

Reviews applied: `['64726', '64762']`

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

- Mesos Reviewbot Windows


On Dec. 21, 2017, 2:25 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64762/
> ---
> 
> (Updated Dec. 21, 2017, 2:25 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed offer operation to operation.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp 4d012ccb65af58dcac2c945bff304176a4141408 
>   include/mesos/v1/mesos.hpp d163f0bf79a570f6f61a5c16310b1683a945b3ef 
>   src/CMakeLists.txt 5b541530035092794f6a98a1be299f652c014877 
>   src/Makefile.am 5c6f180aed33969a82b28f74d42d2e2608548501 
>   src/cli/execute.cpp 13544613d0ce05d12e426e66c5996c05f525e8fb 
>   src/common/protobuf_utils.hpp e112f1984ed39137b9dfec542b4cfd0d358bfe1a 
>   src/common/protobuf_utils.cpp f4380e94cdafb3f05c45d0d507f63bfbe40d8317 
>   src/common/resources_utils.cpp 50651dcf2942aecc1564442f14afbd90ee91cb52 
>   src/common/type_utils.cpp 2ba5302e2577099a39aef2ab84e02b6db2c1faaf 
>   src/examples/long_lived_framework.cpp 
> bb9fc2d775004ed237a0374f193b4c5fa731afc7 
>   src/examples/test_http_framework.cpp 
> e83aded0e98d8c92472c6122e52a581bcb389aa6 
>   src/internal/devolve.hpp 9e56fd974161fe87a172e3376df41fe18e5b8204 
>   src/internal/devolve.cpp 60a768dd9ee382fd55fad0892bead50cf15ef251 
>   src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
> d28ae5e540ec739a831df468cebe876b6356afe8 
>   src/master/http.cpp ec170a257c2d309712f3c4b2fce756eb0b530ad6 
>   src/master/maintenance.hpp b1e176c0b792bdb2a8e7175a14d5d9ab6a90729b 
>   src/master/master.hpp b800cda3c82ba7c471147889075cdc4f915f16b1 
>   src/master/master.cpp cf8a22b636210e9f8d326592d8b64eab2c97caa4 
>   src/master/quota.hpp 479112d1a94ede34e2d46a27ff03999fc1634b8c 
>   src/master/quota_handler.cpp 26b2dc86fa44dfdf6e0a6c4993c754a6384dd851 
>   src/master/registrar.hpp c439f6a11274c081059d666f07ab0ddc47208a7d 
>   src/master/registrar.cpp 488b0896b8bfbb02583d614376053bde35156dee 
>   src/master/registry_operations.hpp 06f68a3112e00b39e228d2c60b880259a07988b5 
>   src/master/validation.cpp 7f5a67d98dbbd5a5d64bc73d530f3d37b3cdfec6 
>   src/master/weights.hpp 4b9c438585c3464a46c5f6b4b114759bd430d7fa 
>   src/master/weights_handler.cpp a4d2fed758878f3e2b9557a61965816aa9e0399c 
>   src/messages/messages.hpp 8e428933bbd96c7a09c9cd703169e0d15404bd82 
>   src/messages/messages.cpp ac8f6c35556ccc04fb94c76b4c255f7ebff449b7 
>   src/resource_provider/manager.hpp c5c2d5297b787822411e83dc8279e7e0fd8f572c 
>   src/resource_provider/manager.cpp e3fcb64b630924e1bb497625708cad3f0fdc064a 
>   src/resource_provider/message.hpp 6236693f1a6a53d347020ffec3e303843f431d3a 
>   src/resource_provider/registrar.cpp 
> ef66c126d3127481388a3c4691538d4bd07e6c1d 
>   src/resource_provider/storage/provider.cpp 
> 833d929e93afd0bb99a9574df24168e82269bc81 
>   src/resource_provider/validation.cpp 
> 5b7de741b866edf90fc5c990378fd65a4a551fad 
>   src/sched/sched.cpp cce4633963bb253fe697e846c4a55abc465eb04e 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp 
> bbbe766f3770b5640e21bcedee7a9395c6dac412 
>   src/slave/paths.hpp 9cbacd8da62e7c7386dca7031fc09a46ae773161 
>   src/slave/paths.cpp fca2a0eec2a75ed76028ea54dc992502275d4bce 
>   src/slave/slave.hpp 9e2663e631c07831423fea9086301c07f80c42e4 
>   src/slave/slave.cpp 6271cfe56733ec121d0d7c691e3dd0792435bf2d 
>   src/status_update_manager/offer_operation.hpp 
> 0ab6be4c6d57410460e0cf4422f61cb2ff28a7db 
>   src/status_update_manager/offer_operation.cpp 
> 9df4973d5d2d331ce93dc82f9b1dc35ae8ea11b8 
>   src/status_update_manager/status_update_manager_process.hpp 
> 4a23573f65d54468319bb2f559593d0866ee299b 
>   src/tests/CMakeLists.txt 42387f7e1aa6ffb7f307ac53d69a1de92d68afa6 
>   src/tests/master_tests.cpp 60718b4f76f52064fe550229db4ebbdff2c4b64f 
>   src/tests/mesos.hpp 41f47cf1b10fddd51e260e2127e2695ffe1aeba5 
>   src/tests/mock_registrar.hpp 92c39940a79a40da9ee222b05b810f11bb70bb58 
>   src/tests/mock_registrar.cpp 0a877b20af7ad693a41e1b8585066d5d43065344 
>   src/tests/offer_operation_status_update_manager_tests.cpp 
> 37bea36c4306411c82824e8d9a4a214d453692e5 
>   src/tests/partition_tests.cpp 9fcc0a8aae034808ff222150d7068e9453e12160 
>   src/tests/persistent_volume_tests.cpp 
> 854676918dba497e1a8c21d877631c6e5c75ee2e 
>   src/tests/reconciliation_tests.cpp 9ee91c9df1017e3884ba280cccbc4614aa

Review Request 64765: Removed a stale TODO in the allocator.

2017-12-20 Thread Benjamin Mahler


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

Review request for mesos, Alexander Rukletsov and Meng Zhu.


Repository: mesos


Description
---

This TODO was added when we used to skip roles with satisfied
quota during the first phase of allocation. We no longer do
this since we now allocate reservations for these roles.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
7f7dae1950bebd22191189c2db0f6678d2e8fa3c 


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


Testing
---

N/A


Thanks,

Benjamin Mahler

Review Request 64763: Fixed compilation for python eggs when gRPC is enabled.

2017-12-20 Thread Chun-Hung Hsiao


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

Review request for mesos, Jie Yu and Kapil Arya.


Repository: mesos


Description
---

Fixed compilation for python eggs when gRPC is enabled.


Diffs
-

  src/python/native_common/ext_modules.py.in 
47c2d24a6e5e49ad5860d96108bbb1e2eeb8dd5b 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 64755: Removed the usage of `Bytes::kilobytes()` and `Bytes::megabytes()`.

2017-12-20 Thread Chun-Hung Hsiao


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

(Updated Dec. 21, 2017, 3:41 a.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Vinod 
Kone.


Changes
---

Removed all calls to the `Bytes` helpers.


Summary (updated)
-

Removed the usage of `Bytes::kilobytes()` and `Bytes::megabytes()`.


Repository: mesos


Description (updated)
---

Removed the usage of `Bytes::kilobytes()` and `Bytes::megabytes()`.


Diffs (updated)
-

  src/examples/balloon_executor.cpp 7343ee72c1fc8d6e58527809ffb74fc5dd09ee0e 
  src/examples/balloon_framework.cpp a36e04b033ba66ba911abde2ad3d6f58decae18a 
  src/examples/disk_full_framework.cpp d9d2d3513529f3a19f392b314001930746d5c91d 
  src/master/validation.cpp 7f5a67d98dbbd5a5d64bc73d530f3d37b3cdfec6 
  src/resource_provider/storage/provider.cpp 
833d929e93afd0bb99a9574df24168e82269bc81 
  src/slave/containerizer/containerizer.cpp 
935dfc9ea787ae714a86c7bc54811703c23267c8 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
113e908743b718e7ad645e8e08b481ed97a4021f 
  src/slave/slave.cpp 6271cfe56733ec121d0d7c691e3dd0792435bf2d 
  src/tests/container_logger_tests.cpp b65cf6a9096dfaa418fd7eb400771cc6df642ea2 
  src/tests/containerizer/docker_containerizer_tests.cpp 
630bb2e352dd55ecb401730c84f823e0a1f2d310 
  src/tests/hierarchical_allocator_tests.cpp 
173e4fbac184ad8d40c8adba19ad64225f11f1f2 
  src/tests/mesos.hpp 41f47cf1b10fddd51e260e2127e2695ffe1aeba5 
  src/tests/persistent_volume_tests.cpp 
854676918dba497e1a8c21d877631c6e5c75ee2e 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 64754: Removed `kilobytes()`, `megabytes()`, etc from `Bytes`.

2017-12-20 Thread Chun-Hung Hsiao


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

(Updated Dec. 21, 2017, 3:41 a.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Vinod 
Kone.


Changes
---

Addressed Ben's comment: killing the helpers.


Summary (updated)
-

Removed `kilobytes()`, `megabytes()`, etc from `Bytes`.


Repository: mesos


Description (updated)
---

These functios are removed because they are lossy. The unit conversion
for `Resource` should be explicit. For example:
  `resource.scalar().set_value(size.bytes() * 1.0 / Bytes::MEGABYTES)`


Diffs (updated)
-

  3rdparty/stout/include/stout/bytes.hpp 
cbe953662bb86c2f1eef3453f557ea17c0c6d13e 


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

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


Testing
---

See later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 64760: Fixed a few comment typos in the `hierarchical.cpp`.

2017-12-20 Thread Benjamin Mahler


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


Ship it!




Ship It!

- Benjamin Mahler


On Dec. 21, 2017, 12:50 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64760/
> ---
> 
> (Updated Dec. 21, 2017, 12:50 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a few comment typos in the `hierarchical.cpp`.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 1c767f7f778819dcfa6eff51765163aec1b70a2d 
> 
> 
> Diff: https://reviews.apache.org/r/64760/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 64743: Enabled function sections.

2017-12-20 Thread Mesos Reviewbot Windows


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



PASS: Mesos patch 64743 was successfully built and tested.

Reviews applied: `['64743']`

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

- Mesos Reviewbot Windows


On Dec. 21, 2017, 1:47 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64743/
> ---
> 
> (Updated Dec. 21, 2017, 1:47 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Bugs: MESOS-8348
> https://issues.apache.org/jira/browse/MESOS-8348
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If we tell the compiler to place each function in a separate
> section, this allows the linker to garbage collect unused
> sections. This significantly decreases the size of the final
> build artifacts and provides some modest improvements in build
> times.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake dc9dc161dce1c714748bcec2fc15c4fbfbccaec2 
>   configure.ac 5ffc33a6b9495db64f1ec164ed96f3faea620348 
> 
> 
> Diff: https://reviews.apache.org/r/64743/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 64758: Added a test to ensure MOUNT disk is not chopped during allocation.

2017-12-20 Thread Benjamin Mahler


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


Ship it!




Ship It!

- Benjamin Mahler


On Dec. 21, 2017, 2:48 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64758/
> ---
> 
> (Updated Dec. 21, 2017, 2:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While quota allocation should be fine-grained, some resources are
> unchoppable. They have to be offered entirely. This test verifies
> one of the cases: disk resource of type MOUNT.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 173e4fbac184ad8d40c8adba19ad64225f11f1f2 
> 
> 
> Diff: https://reviews.apache.org/r/64758/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 64758: Added a test to ensure MOUNT disk is not chopped during allocation.

2017-12-20 Thread Meng Zhu


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

(Updated Dec. 20, 2017, 6:48 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Summary (updated)
-

Added a test to ensure MOUNT disk is not chopped during allocation.


Repository: mesos


Description (updated)
---

While quota allocation should be fine-grained, some resources are
unchoppable. They have to be offered entirely. This test verifies
one of the cases: disk resource of type MOUNT.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
173e4fbac184ad8d40c8adba19ad64225f11f1f2 


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

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


Testing
---

make check


Thanks,

Meng Zhu

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

2017-12-20 Thread Benjamin Mahler


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


Ship it!




Ship It!

- Benjamin Mahler


On Dec. 21, 2017, 2:25 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64003/
> ---
> 
> (Updated Dec. 21, 2017, 2:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, 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. And a role's quota is only
> considered to be satisfied if all of its quota resources
> are satisfied. Previously, a role's quota is considered
> to be met if any one of its quota resources reaches the limit.
> 
> Also fixed a few affected allocator tests.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 1c767f7f778819dcfa6eff51765163aec1b70a2d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 173e4fbac184ad8d40c8adba19ad64225f11f1f2 
> 
> 
> Diff: https://reviews.apache.org/r/64003/diff/6/
> 
> 
> Testing
> ---
> 
> Fixed a few broken tests due to this patch.
> make check.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 64761: Moved the quota headroom tracking before quota allocation.

2017-12-20 Thread Meng Zhu


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

(Updated Dec. 20, 2017, 6:39 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This paves the way for improved quota headroom enforcement
for the quota role allocation stage.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
1c767f7f778819dcfa6eff51765163aec1b70a2d 


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

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


Testing
---

make check


Thanks,

Meng Zhu

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

2017-12-20 Thread Meng Zhu


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

(Updated Dec. 20, 2017, 6:25 p.m.)


Review request for mesos, Benjamin Mahler, Jie Yu, and Michael Park.


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


Repository: mesos


Description (updated)
---

Allocator now does fine-grained resource allocation up
to the role's quota limit. And a role's quota is only
considered to be satisfied if all of its quota resources
are satisfied. Previously, a role's quota is considered
to be met if any one of its quota resources reaches the limit.

Also fixed a few affected allocator tests.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
1c767f7f778819dcfa6eff51765163aec1b70a2d 
  src/tests/hierarchical_allocator_tests.cpp 
173e4fbac184ad8d40c8adba19ad64225f11f1f2 


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

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


Testing
---

Fixed a few broken tests due to this patch.
make check.


Thanks,

Meng Zhu

Review Request 64762: Renamed offer operation to operation.

2017-12-20 Thread Gaston Kleiman


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

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


Repository: mesos


Description
---

Renamed offer operation to operation.


Diffs
-

  include/mesos/type_utils.hpp 4d012ccb65af58dcac2c945bff304176a4141408 
  include/mesos/v1/mesos.hpp d163f0bf79a570f6f61a5c16310b1683a945b3ef 
  src/CMakeLists.txt 5b541530035092794f6a98a1be299f652c014877 
  src/Makefile.am 5c6f180aed33969a82b28f74d42d2e2608548501 
  src/cli/execute.cpp 13544613d0ce05d12e426e66c5996c05f525e8fb 
  src/common/protobuf_utils.hpp e112f1984ed39137b9dfec542b4cfd0d358bfe1a 
  src/common/protobuf_utils.cpp f4380e94cdafb3f05c45d0d507f63bfbe40d8317 
  src/common/resources_utils.cpp 50651dcf2942aecc1564442f14afbd90ee91cb52 
  src/common/type_utils.cpp 2ba5302e2577099a39aef2ab84e02b6db2c1faaf 
  src/examples/long_lived_framework.cpp 
bb9fc2d775004ed237a0374f193b4c5fa731afc7 
  src/examples/test_http_framework.cpp e83aded0e98d8c92472c6122e52a581bcb389aa6 
  src/internal/devolve.hpp 9e56fd974161fe87a172e3376df41fe18e5b8204 
  src/internal/devolve.cpp 60a768dd9ee382fd55fad0892bead50cf15ef251 
  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
d28ae5e540ec739a831df468cebe876b6356afe8 
  src/master/http.cpp ec170a257c2d309712f3c4b2fce756eb0b530ad6 
  src/master/maintenance.hpp b1e176c0b792bdb2a8e7175a14d5d9ab6a90729b 
  src/master/master.hpp b800cda3c82ba7c471147889075cdc4f915f16b1 
  src/master/master.cpp cf8a22b636210e9f8d326592d8b64eab2c97caa4 
  src/master/quota.hpp 479112d1a94ede34e2d46a27ff03999fc1634b8c 
  src/master/quota_handler.cpp 26b2dc86fa44dfdf6e0a6c4993c754a6384dd851 
  src/master/registrar.hpp c439f6a11274c081059d666f07ab0ddc47208a7d 
  src/master/registrar.cpp 488b0896b8bfbb02583d614376053bde35156dee 
  src/master/registry_operations.hpp 06f68a3112e00b39e228d2c60b880259a07988b5 
  src/master/validation.cpp 7f5a67d98dbbd5a5d64bc73d530f3d37b3cdfec6 
  src/master/weights.hpp 4b9c438585c3464a46c5f6b4b114759bd430d7fa 
  src/master/weights_handler.cpp a4d2fed758878f3e2b9557a61965816aa9e0399c 
  src/messages/messages.hpp 8e428933bbd96c7a09c9cd703169e0d15404bd82 
  src/messages/messages.cpp ac8f6c35556ccc04fb94c76b4c255f7ebff449b7 
  src/resource_provider/manager.hpp c5c2d5297b787822411e83dc8279e7e0fd8f572c 
  src/resource_provider/manager.cpp e3fcb64b630924e1bb497625708cad3f0fdc064a 
  src/resource_provider/message.hpp 6236693f1a6a53d347020ffec3e303843f431d3a 
  src/resource_provider/registrar.cpp ef66c126d3127481388a3c4691538d4bd07e6c1d 
  src/resource_provider/storage/provider.cpp 
833d929e93afd0bb99a9574df24168e82269bc81 
  src/resource_provider/validation.cpp 5b7de741b866edf90fc5c990378fd65a4a551fad 
  src/sched/sched.cpp cce4633963bb253fe697e846c4a55abc465eb04e 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp 
bbbe766f3770b5640e21bcedee7a9395c6dac412 
  src/slave/paths.hpp 9cbacd8da62e7c7386dca7031fc09a46ae773161 
  src/slave/paths.cpp fca2a0eec2a75ed76028ea54dc992502275d4bce 
  src/slave/slave.hpp 9e2663e631c07831423fea9086301c07f80c42e4 
  src/slave/slave.cpp 6271cfe56733ec121d0d7c691e3dd0792435bf2d 
  src/status_update_manager/offer_operation.hpp 
0ab6be4c6d57410460e0cf4422f61cb2ff28a7db 
  src/status_update_manager/offer_operation.cpp 
9df4973d5d2d331ce93dc82f9b1dc35ae8ea11b8 
  src/status_update_manager/status_update_manager_process.hpp 
4a23573f65d54468319bb2f559593d0866ee299b 
  src/tests/CMakeLists.txt 42387f7e1aa6ffb7f307ac53d69a1de92d68afa6 
  src/tests/master_tests.cpp 60718b4f76f52064fe550229db4ebbdff2c4b64f 
  src/tests/mesos.hpp 41f47cf1b10fddd51e260e2127e2695ffe1aeba5 
  src/tests/mock_registrar.hpp 92c39940a79a40da9ee222b05b810f11bb70bb58 
  src/tests/mock_registrar.cpp 0a877b20af7ad693a41e1b8585066d5d43065344 
  src/tests/offer_operation_status_update_manager_tests.cpp 
37bea36c4306411c82824e8d9a4a214d453692e5 
  src/tests/partition_tests.cpp 9fcc0a8aae034808ff222150d7068e9453e12160 
  src/tests/persistent_volume_tests.cpp 
854676918dba497e1a8c21d877631c6e5c75ee2e 
  src/tests/reconciliation_tests.cpp 9ee91c9df1017e3884ba280cccbc4614aa41acf7 
  src/tests/registrar_tests.cpp 34365c799be599fda1681cfc61ec9b7d00296b5b 
  src/tests/reservation_tests.cpp 34fa44188c4f220b050f2a1ff968cd76de523426 
  src/tests/resource_provider_manager_tests.cpp 
4e86016a450c30fea90e9c6e5c1f2f1aab5f42c0 
  src/tests/resource_provider_validation_tests.cpp 
db7b431555cae5599ca637fff9ca04af2594b7a7 
  src/tests/slave_tests.cpp b87015d4b3f3fcd1948978d42e20de8108e8f93a 
  src/tests/state_tests.cpp 5366b8bea96e2a317fede22b6397b6adc278d981 
  src/tests/storage_local_resource_provider_tests.cpp 
516e56a116dcf5b2209a522adcd37f3f3a3be4f5 
  src/v1/mesos.cpp 190c999dc06cecf4a57b79a3af395c9f5a7c5a67 


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


Testing
---


Thanks,

Gaston Kleima

Re: Review Request 63976: Modified `downgradeResources` to use protobuf reflection.

2017-12-20 Thread Benjamin Mahler

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

Fix it, then Ship it!

src/common/resources_utils.hpp
Line 175 (original), 175-176 (patched)

s/does/do/?

Maybe remove the first sentence and just directly say "all-or-nothing"? 
Atomicity seems a little obscure here to me

src/common/resources_utils.hpp
Lines 181-182 (patched)

Does it keep going and finish the rest in the error case? Or does it stop 
converting and return the error? From reading the above comments, it sounds 
like it wouldn't break early.

src/common/resources_utils.hpp
Lines 184-185 (patched)

Are you saying that once a component has refined reservations, it cannot be 
downgraded back to.. the version before we had reservation refinement? As 
written, it seems to be speaking too generally about downgrades: whether 1.9 
could be downgraded to 1.8 is a separate question?

src/common/resources_utils.hpp
Lines 186 (patched)

s/message/resource/ ?

s/Resources/Resource/ ?

src/common/resources_utils.cpp
Lines 743 (patched)

s/Resources/Resource/ ?

src/common/resources_utils.cpp
Lines 779-781 (patched)

Hm.. is returning bool here an optimization? I would imagine the resultant 
map could (or I guess already does) contain the top level descriptor?

Then, downgradeResourcesImpl would first check the passed in message 
descriptor and bail if not contained?

```
static Try downgradeResourcesImpl(
Message* message,
const hashmap& resourcesContainment)
{
  CHECK_NOTNULL(message);

  const Descriptor* descriptor = message->GetDescriptor();

  if (!resourcesContainment.at(descriptor)) {
return; // Done.
  }

  if (descriptor == mesos::Resource::descriptor()) {
return downgradeResources(static_cast(message));
  }

  // When recursing into fields, I guess we don't bother checking
  // containement before recursing since the recursive call will check?
```

Then the top level function gets a bit simpler?

```
Try downgradeResources(Message* message)
{
  const Descriptor* descriptor = message->GetDescriptor();

  hashmap resourcesContainment;
  internal::precomputeResourcesContainment(descriptor, 
&resourcesContainment);

  return internal::downgradeResourcesImpl(message, resourcesContainment);
```

Just curious to get your thoughts on the two approaches.

src/common/resources_utils.cpp
Lines 822 (patched)

newline?

src/common/resources_utils.cpp
Lines 825 (patched)

newline?

src/tests/resources_tests.cpp
Lines 3144-3162 (patched)

These arent pre- right? Maybe you want to add a comment saying that you're 
expecting a partial downgrade after the error?

- Benjamin Mahler

On Nov. 21, 2017, 6:07 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63976/
> ---
> 
> (Updated Nov. 21, 2017, 6:07 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8221
> https://issues.apache.org/jira/browse/MESOS-8221
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, our `downgradeResources` function only took a pointer to
> `RepeatedPtrField`. This wasn't great since we needed manually
> invoke `downgradeResources` on every `Resource`s field of every message.
> 
> This patch makes it such that `downgradeResources` can take any
> `protobuf::Message` or `protobuf::RepeatedPtrField`.
> 
> 
> Diffs
> -
> 
>   src/common/resources_utils.hpp 5b74ff2dd3ecb1a0101671d11ea10e29a43524b0 
>   src/common/resources_utils.cpp 1676b72a9ad15bf8b131698a0600a1b0937c00b4 
>   src/tests/resources_tests.cpp 64bde85e0e7f0bd395189eb6a8635383095b2f07 
> 
> 
> Diff: https://reviews.apache.org/r/63976/diff/2/
> 
> 
> Testing
> ---
> 
> Added new tests + `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 59987: Added protobuf map support.

2017-12-20 Thread Qian Zhang


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

(Updated Dec. 21, 2017, 10:07 a.m.)


Review request for mesos, Anand Mazumdar, Chun-Hung Hsiao, and Zhitao Li.


Changes
---

Fixed a comment and code errors.


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


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with proto3 compiler, see the following discussion for details:
https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228, however, to use protobuf map in Mesos code, we also
need to add the protobuf map support to the code in Mesos for
converting protobuf message to JSON object and parsing JSON
object as protobuf message, that is what I have done in this patch.


Diffs (updated)
-

  3rdparty/stout/include/stout/protobuf.hpp 
baad12648dd78ab72ea4277f4c7f99da16696a40 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 64743: Enabled function sections.

2017-12-20 Thread James Peach



> On Dec. 20, 2017, 10:52 a.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 267 (patched)
> > 
> >
> > What about static archives? Last time I looked this was broken in the 
> > cmake setup, but it would be great to prepare to also make use of this flag 
> > there once its ready (if applicable).

GC sections can't really apply to archives since you don't know which sections 
it is safe to GC.


> On Dec. 20, 2017, 10:52 a.m., Benjamin Bannier wrote:
> > configure.ac
> > Lines 507-508 (patched)
> > 
> >
> > Why are we disabling this on macos? These flags do work with the latest 
> > LLVM toolchain (clang & lld), so let's default-enable it on macos as well.
> > 
> > If you worry about some Apple clang fork not supporting these flags, 
> > let's please add some feature detection instead of branching on the 
> > platform here.

Switched to just enabling it wherever it works.


- James


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


On Dec. 21, 2017, 1:47 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64743/
> ---
> 
> (Updated Dec. 21, 2017, 1:47 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Bugs: MESOS-8348
> https://issues.apache.org/jira/browse/MESOS-8348
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If we tell the compiler to place each function in a separate
> section, this allows the linker to garbage collect unused
> sections. This significantly decreases the size of the final
> build artifacts and provides some modest improvements in build
> times.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake dc9dc161dce1c714748bcec2fc15c4fbfbccaec2 
>   configure.ac 5ffc33a6b9495db64f1ec164ed96f3faea620348 
> 
> 
> Diff: https://reviews.apache.org/r/64743/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 64761: Moved the quota headroom tracking before quota allocation.

2017-12-20 Thread Mesos Reviewbot Windows


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



FAIL: Failed to apply the dependent review: 64003.

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

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

Relevant logs:

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

```
Traceback (most recent call last):
  File ".\support\apply-reviews.py", line 434, in 
main()
  File ".\support\apply-reviews.py", line 429, in main
reviewboard(options)
  File ".\support\apply-reviews.py", line 419, in reviewboard
apply_review(options)
  File ".\support\apply-reviews.py", line 160, in apply_review
commit_patch(options)
  File ".\support\apply-reviews.py", line 271, in commit_patch
message.write(data['message'])
UnicodeEncodeError: 'ascii' codec can't encode character u'\u2019' in position 
112: ordinal not in range(128)
```

- Mesos Reviewbot Windows


On Dec. 21, 2017, 9:11 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64761/
> ---
> 
> (Updated Dec. 21, 2017, 9:11 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7099
> https://issues.apache.org/jira/browse/MESOS-7099
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This paves the way for improved quota headroom enforcement
> for the quota role allocation stage.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 1c767f7f778819dcfa6eff51765163aec1b70a2d 
> 
> 
> Diff: https://reviews.apache.org/r/64761/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 64760: Fixed a few comment typos in the `hierarchical.cpp`.

2017-12-20 Thread Mesos Reviewbot Windows


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



PASS: Mesos patch 64760 was successfully built and tested.

Reviews applied: `['64760']`

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

- Mesos Reviewbot Windows


On Dec. 21, 2017, 12:50 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64760/
> ---
> 
> (Updated Dec. 21, 2017, 12:50 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a few comment typos in the `hierarchical.cpp`.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 1c767f7f778819dcfa6eff51765163aec1b70a2d 
> 
> 
> Diff: https://reviews.apache.org/r/64760/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 64761: Moved the quota headroom tracking before quota allocation.

2017-12-20 Thread Benjamin Mahler


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


Fix it, then Ship it!




Looks good, modulo stripping the resources when decrementing the required & 
available headroom.


src/master/allocator/mesos/hierarchical.cpp
Lines 1955-1956 (patched)


Should these be stripped?


- Benjamin Mahler


On Dec. 21, 2017, 1:11 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64761/
> ---
> 
> (Updated Dec. 21, 2017, 1:11 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7099
> https://issues.apache.org/jira/browse/MESOS-7099
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This paves the way for improved quota headroom enforcement
> for the quota role allocation stage.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 1c767f7f778819dcfa6eff51765163aec1b70a2d 
> 
> 
> Diff: https://reviews.apache.org/r/64761/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 64758: Added a test to ensure MOUNT disk resource is allocated exclusively.

2017-12-20 Thread Benjamin Mahler


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



Looks good!

Rather than "exclusively" how about all-or-nothing or is not chopped during 
allocation? Exclusively seems to suggest on its own without anything else.


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


Maybe indivisible or unchoppable instead of exclusive?



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


Ditto here.



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


seems like agentResources should include the mount disk?



src/tests/hierarchical_allocator_tests.cpp
Lines 3476-3478 (patched)


This subtraction a little puzzling to follow, is it possible to just use 
agent3.resources() - some disk due to chopping?


- Benjamin Mahler


On Dec. 20, 2017, 10:07 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64758/
> ---
> 
> (Updated Dec. 20, 2017, 10:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While quota allocation should be fine-grained, some resources are
> exclusive. They cannot be chopped into finer granularity and have
> to be offered entirely. This test verifies one of the cases: disk
> resource of type MOUNT.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 173e4fbac184ad8d40c8adba19ad64225f11f1f2 
> 
> 
> Diff: https://reviews.apache.org/r/64758/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 64743: Enabled function sections.

2017-12-20 Thread James Peach


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

(Updated Dec. 21, 2017, 1:47 a.m.)


Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.


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


Repository: mesos


Description (updated)
---

If we tell the compiler to place each function in a separate
section, this allows the linker to garbage collect unused
sections. This significantly decreases the size of the final
build artifacts and provides some modest improvements in build
times.


Diffs (updated)
-

  cmake/CompilationConfigure.cmake dc9dc161dce1c714748bcec2fc15c4fbfbccaec2 
  configure.ac 5ffc33a6b9495db64f1ec164ed96f3faea620348 


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

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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 64741: Eliminated some unnecessary copying in the HTTP operator API.

2017-12-20 Thread Benjamin Mahler



> On Dec. 20, 2017, 7:52 p.m., Meng Zhu wrote:
> > src/master/http.cpp
> > Lines 4232-4235 (original), 4232-4234 (patched)
> > 
> >
> > how about:
> > 
> > `*getTasks.add_pending_tasks() = protobuf::createTask(taskInfo, 
> > TASK_STAGING, framework->id());`

Sounds good!


- Benjamin


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


On Dec. 20, 2017, 1:47 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64741/
> ---
> 
> (Updated Dec. 20, 2017, 1:47 a.m.)
> 
> 
> Review request for mesos, Michael Park and Meng Zhu.
> 
> 
> Bugs: MESOS-8344
> https://issues.apache.org/jira/browse/MESOS-8344
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is only a minor portion of the performance improvements
> needed to bring the v1 operator API close to the v0 API
> performance.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp ec170a257c2d309712f3c4b2fce756eb0b530ad6 
> 
> 
> Diff: https://reviews.apache.org/r/64741/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

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

2017-12-20 Thread Benjamin Mahler


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



Tests look good, do we want to add a test to ensure unchoppable resources don't 
get chopped?


src/tests/hierarchical_allocator_tests.cpp
Line 3831 (original), 3834 (patched)


Is this comment still accurate? More than its quota?



src/tests/hierarchical_allocator_tests.cpp
Lines 5198-5200 (original)


Should we keep the content after the first comma?


- Benjamin Mahler


On Dec. 20, 2017, 2:06 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64003/
> ---
> 
> (Updated Dec. 20, 2017, 2:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, 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. And a role's quota is only
> considered to be satisfied if all of its quota resources
> are satisfied. Previously, a role's quota is considered
> to be met if any one of its quota resources reaches the limit.
> 
> Also fixed a few affected allocator tests.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 1c767f7f778819dcfa6eff51765163aec1b70a2d 
>   src/tests/hierarchical_allocator_tests.cpp 
> 173e4fbac184ad8d40c8adba19ad64225f11f1f2 
> 
> 
> Diff: https://reviews.apache.org/r/64003/diff/4/
> 
> 
> Testing
> ---
> 
> Fixed a few broken tests due to this patch.
> make check.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 64761: Moved the quota headroom tracking before quota allocation.

2017-12-20 Thread Meng Zhu


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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This paves the way for improved quota headroom enforcement
for the quota role allocation stage.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
1c767f7f778819dcfa6eff51765163aec1b70a2d 


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


Testing
---

make check


Thanks,

Meng Zhu

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

2017-12-20 Thread Benjamin Mahler


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




src/master/allocator/mesos/hierarchical.cpp
Lines 1681-1684 (original), 1671-1674 (patched)


I asked alexr about this TODO, it has become stale because we now have to 
allocate the reservations and can't "skip" satisfied roles anymore. Feel free 
to remove it in a different patch.



src/master/allocator/mesos/hierarchical.cpp
Line 1686 (original), 1676 (patched)


is -> is a



src/master/allocator/mesos/hierarchical.cpp
Lines 1741-1752 (original), 1728-1745 (patched)


Hm.. perhaps a single overall explanation might be more readable? Here's a 
suggestion with a TODO for checking headroom:

```
// We allocate the role's reservations as well as any unreserved
// resources while ensuring the role stays within its quota limits.
// This means that we'll "chop" the unreserved resources up to
// the quota limit if necessary.
//
// E.g. A role has no allocations or reservations yet and a 10 cpu
//  quota limit. We'll chop a 15 cpu agent down to only
//  allocate 10 cpus to the role to keep it within its limit.
//
// In the case that the role needs some of the resources on this
// agent to make progress towards its quota, we'll *also* allocate
// all of the resources for which it does not have quota.
//
// E.g. The agent has 1 cpu, 1024 mem, 1024 disk, 1 gpu, 5 ports
//  and the role has quota for 1 cpu, 1024 mem. We'll include
//  the disk, gpu, and ports in the allocation, despite the
//  role not having any quota guarantee for them.
//
// We have to do this for now because it's not possible to set
// quota on non-scalar resources, like ports. However, for scalar
// resources that can have quota set, we should ideally make
// sure that when we include them, we're not violating some
// other role's guarantes!
//
// TODO(mzhu): Check the headroom when we're including scalar
// resources that this role does not have quota for.
//
// TODO(mzhu): Since we're treating the resources with unset
// quota as having no guarantee and no limit, these should be
// also be allocated further in the second allocation "phase"
// below (above guarantee up to limit).
```



src/master/allocator/mesos/hierarchical.cpp
Line 1752 (original), 1737-1742 (patched)


Hm.. I'm kind of surprised we have to distinctly track this, isn't this 
always just equal to the unreserved non-revocable resources within `resources`?



src/master/allocator/mesos/hierarchical.cpp
Lines 1740-1741 (patched)


Don't think we need this note, given that it applies to `resources` 
already? Especially if we don't need to track this in a separate variable



src/master/allocator/mesos/hierarchical.cpp
Lines 1744-1746 (patched)


Hm.. this if condition means that if I have:

Quota Guarantee/Limit: 1 cpu, 1024 mem
Agent has: 1 cpu, 1024 mem, 1 gpu, 5 ports

Case (1):
Existing Allocation: 1 cpu, 1024 mem
I'll be allocated: (nothing)

Case (2):
Existing Allocation: (nothing)
I'll be allocated: 1 cpu, 1024 mem, 1 gpu, 5 ports

In other words, we are deciding that if you need some resources on this 
agent to sastisfy your quota, then we'll give you everything else that you 
don't have quota for.



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


Updating available seems a little odd? Maybe this should be `Resources 
unreserved = ...`?



src/master/allocator/mesos/hierarchical.cpp
Lines 1752-1754 (patched)


Hm.. we may want to shed some light on the disk case that motivated this?

```
  // When "chopping" resources, there is more than 1 "chop" that
  // can be done to satisfy the limits. Consider the case with
  // two `RAW` disks of 1GB and a "disk" quota of 1GB. We could
  // pick either of the disks here, but not both.
  //
  // In order to avoid repeatedly choosing the same "chop" of
  // the resources each time we allocate, we introduce some

Review Request 64760: Fixed a few comment typos in the `hierarchical.cpp`.

2017-12-20 Thread Meng Zhu


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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Fixed a few comment typos in the `hierarchical.cpp`.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
1c767f7f778819dcfa6eff51765163aec1b70a2d 


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


Testing
---


Thanks,

Meng Zhu

Re: Review Request 64741: Eliminated some unnecessary copying in the HTTP operator API.

2017-12-20 Thread Meng Zhu


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


Ship it!




Ship It!

- Meng Zhu


On Dec. 19, 2017, 5:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64741/
> ---
> 
> (Updated Dec. 19, 2017, 5:47 p.m.)
> 
> 
> Review request for mesos, Michael Park and Meng Zhu.
> 
> 
> Bugs: MESOS-8344
> https://issues.apache.org/jira/browse/MESOS-8344
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is only a minor portion of the performance improvements
> needed to bring the v1 operator API close to the v0 API
> performance.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp ec170a257c2d309712f3c4b2fce756eb0b530ad6 
> 
> 
> Diff: https://reviews.apache.org/r/64741/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 64756: Fixed a bug that SLRP would fail if the state files do not exist.

2017-12-20 Thread Mesos Reviewbot Windows


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



PASS: Mesos patch 64756 was successfully built and tested.

Reviews applied: `['64754', '64755', '64756']`

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

- Mesos Reviewbot Windows


On Dec. 20, 2017, 10:14 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64756/
> ---
> 
> (Updated Dec. 20, 2017, 10:14 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug that SLRP would fail if the state files do not exist.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 833d929e93afd0bb99a9574df24168e82269bc81 
> 
> 
> Diff: https://reviews.apache.org/r/64756/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64754: Made helper functions of `Bytes` return a double value.

2017-12-20 Thread Chun-Hung Hsiao



> On Dec. 20, 2017, 10:25 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/bytes.hpp
> > Lines 82-85 (original), 82-85 (patched)
> > 
> >
> > I'm not so sure we need these at all, is it possible to remove them?
> > 
> > Doubles are a little concerning, since there is a base unit of bytes 
> > involved here but doubles can represent values smaller than a single byte? 
> > E.g. 1.01 kilobytes

It seems to me that we don't have such a problem because the constructor of 
`Bytes` always takes a `uint64_t`, so `Bytes` does not provide such a 
semantics. I'm using these helper functions because CSI uses bytes but our 
`Resource` protobuf uses double, so I need to either use the helper or do my 
own unit conversion. Would you prefer the latter?


- Chun-Hung


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


On Dec. 20, 2017, 10:13 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64754/
> ---
> 
> (Updated Dec. 20, 2017, 10:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes `kilobytes()`, `megabytes()`, `gigabytes()` and
> `terabytes()` return doubles to keep the precisions.
> 
> NOTE: In the Mesos codebase, the returned values of these helper
> functions except for the ones in the tests and SLRPs are used in the
> following 3 ways:
> 
> 1. Comparing with some constant value.
> 2. Stringified as part of a log message.
> 3. Stringified then onsumed by `Resources::parse()`, which will do fixed
>point normalization.
> 
> In all cases, the changes introduced in this patch should make no harm,
> but just make the result more accurate.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/bytes.hpp 
> cbe953662bb86c2f1eef3453f557ea17c0c6d13e 
> 
> 
> Diff: https://reviews.apache.org/r/64754/diff/1/
> 
> 
> Testing
> ---
> 
> See later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64754: Made helper functions of `Bytes` return a double value.

2017-12-20 Thread Benjamin Mahler


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




3rdparty/stout/include/stout/bytes.hpp
Lines 82-85 (original), 82-85 (patched)


I'm not so sure we need these at all, is it possible to remove them?

Doubles are a little concerning, since there is a base unit of bytes 
involved here but doubles can represent values smaller than a single byte? E.g. 
1.01 kilobytes


- Benjamin Mahler


On Dec. 20, 2017, 10:13 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64754/
> ---
> 
> (Updated Dec. 20, 2017, 10:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes `kilobytes()`, `megabytes()`, `gigabytes()` and
> `terabytes()` return doubles to keep the precisions.
> 
> NOTE: In the Mesos codebase, the returned values of these helper
> functions except for the ones in the tests and SLRPs are used in the
> following 3 ways:
> 
> 1. Comparing with some constant value.
> 2. Stringified as part of a log message.
> 3. Stringified then onsumed by `Resources::parse()`, which will do fixed
>point normalization.
> 
> In all cases, the changes introduced in this patch should make no harm,
> but just make the result more accurate.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/bytes.hpp 
> cbe953662bb86c2f1eef3453f557ea17c0c6d13e 
> 
> 
> Diff: https://reviews.apache.org/r/64754/diff/1/
> 
> 
> Testing
> ---
> 
> See later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 64755: Adjusted the usage of `Bytes::kilobytes()` and `Bytes::megabytes()`.

2017-12-20 Thread Chun-Hung Hsiao


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

Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Vinod 
Kone.


Repository: mesos


Description
---

Adjusted the usage of `Bytes::kilobytes()` and `Bytes::megabytes()`.


Diffs
-

  src/resource_provider/storage/provider.cpp 
833d929e93afd0bb99a9574df24168e82269bc81 
  src/tests/container_logger_tests.cpp b65cf6a9096dfaa418fd7eb400771cc6df642ea2 
  src/tests/containerizer/docker_containerizer_tests.cpp 
630bb2e352dd55ecb401730c84f823e0a1f2d310 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Review Request 64756: Fixed a bug that SLRP would fail if the state files do not exist.

2017-12-20 Thread Chun-Hung Hsiao


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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Fixed a bug that SLRP would fail if the state files do not exist.


Diffs
-

  src/resource_provider/storage/provider.cpp 
833d929e93afd0bb99a9574df24168e82269bc81 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Review Request 64754: Made helper functions of `Bytes` return a double value.

2017-12-20 Thread Chun-Hung Hsiao


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

Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Vinod 
Kone.


Repository: mesos


Description
---

This patch makes `kilobytes()`, `megabytes()`, `gigabytes()` and
`terabytes()` return doubles to keep the precisions.

NOTE: In the Mesos codebase, the returned values of these helper
functions except for the ones in the tests and SLRPs are used in the
following 3 ways:

1. Comparing with some constant value.
2. Stringified as part of a log message.
3. Stringified then onsumed by `Resources::parse()`, which will do fixed
   point normalization.

In all cases, the changes introduced in this patch should make no harm,
but just make the result more accurate.


Diffs
-

  3rdparty/stout/include/stout/bytes.hpp 
cbe953662bb86c2f1eef3453f557ea17c0c6d13e 


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


Testing
---

See later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 64758: Added a test to ensure MOUNT disk resource is allocated exclusively.

2017-12-20 Thread Mesos Reviewbot Windows


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



FAIL: Failed to apply the dependent review: 64003.

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

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

Relevant logs:

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

```
Traceback (most recent call last):
  File ".\support\apply-reviews.py", line 434, in 
main()
  File ".\support\apply-reviews.py", line 429, in main
reviewboard(options)
  File ".\support\apply-reviews.py", line 419, in reviewboard
apply_review(options)
  File ".\support\apply-reviews.py", line 160, in apply_review
commit_patch(options)
  File ".\support\apply-reviews.py", line 271, in commit_patch
message.write(data['message'])
UnicodeEncodeError: 'ascii' codec can't encode character u'\u2019' in position 
112: ordinal not in range(128)
```

- Mesos Reviewbot Windows


On Dec. 20, 2017, 2:07 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64758/
> ---
> 
> (Updated Dec. 20, 2017, 2:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While quota allocation should be fine-grained, some resources are
> exclusive. They cannot be chopped into finer granularity and have
> to be offered entirely. This test verifies one of the cases: disk
> resource of type MOUNT.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 173e4fbac184ad8d40c8adba19ad64225f11f1f2 
> 
> 
> Diff: https://reviews.apache.org/r/64758/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 64758: Added a test to ensure MOUNT disk resource is allocated exclusively.

2017-12-20 Thread Meng Zhu


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

Review request for mesos, Benjamin Mahler and Michael Park.


Repository: mesos


Description
---

While quota allocation should be fine-grained, some resources are
exclusive. They cannot be chopped into finer granularity and have
to be offered entirely. This test verifies one of the cases: disk
resource of type MOUNT.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
173e4fbac184ad8d40c8adba19ad64225f11f1f2 


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


Testing
---

make check


Thanks,

Meng Zhu

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

2017-12-20 Thread Chun-Hung Hsiao


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




src/slave/slave.cpp
Line 7428 (original), 7439 (patched)


Don't we need to check `operation->info().has_id()` here?


- Chun-Hung Hsiao


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 64033: Terminated driver-based executors if kill arrives before launch task.

2017-12-20 Thread Mesos Reviewbot Windows


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



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. 20, 2017, 8:35 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64033/
> ---
> 
> (Updated Dec. 20, 2017, 8:35 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 7fc46daa633ca42944123a7546ac4eceea93956d 
>   src/launcher/executor.cpp 31a47106d7f511220afba4fb382c9252d4671f6e 
> 
> 
> Diff: https://reviews.apache.org/r/64033/diff/6/
> 
> 
> 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.

2017-12-20 Thread Alexander Rukletsov


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

(Updated Dec. 20, 2017, 8:35 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 (updated)
-

  src/docker/executor.cpp 3974f20052e3c12eb154a5146d19d4dc1759859f 
  src/exec/exec.cpp 7fc46daa633ca42944123a7546ac4eceea93956d 
  src/launcher/executor.cpp 31a47106d7f511220afba4fb382c9252d4671f6e 


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

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


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.

2017-12-20 Thread Alexander Rukletsov


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

(Updated Dec. 20, 2017, 8:17 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 (updated)
-

  src/docker/executor.cpp 3974f20052e3c12eb154a5146d19d4dc1759859f 
  src/exec/exec.cpp 7fc46daa633ca42944123a7546ac4eceea93956d 
  src/launcher/executor.cpp 31a47106d7f511220afba4fb382c9252d4671f6e 


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

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


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.

2017-12-20 Thread Alexander Rukletsov


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

(Updated Dec. 20, 2017, 8:16 p.m.)


Review request for mesos, Andrei Budnik, Anand Mazumdar, Armand Grillet, and 
Vinod Kone.


Summary (updated)
-

Ensured executor adapter propagates error and shutdown messages.


Repository: mesos


Description (updated)
---

Prior to this patch, if an error, kill, 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 / kill event arrived before the executor had subscribed.


Diffs (updated)
-

  src/executor/v0_v1executor.cpp 61d591993e6388ba3b4d64a3bdb63c3a3513fbeb 


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

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


Testing
---


Thanks,

Alexander Rukletsov

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

2017-12-20 Thread Alexander Rukletsov


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

(Updated Dec. 20, 2017, 8:14 p.m.)


Review request for mesos, Andrei Budnik, Anand Mazumdar, Armand Grillet, and 
Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/executor.cpp 31a47106d7f511220afba4fb382c9252d4671f6e 


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

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


Testing
---

Writing a proper test is tricky: our test harness does not allow to drop 
outgoing messages and hence `RunTaskMessage` from the agent to the executor 
can't be dropped (since executor is running in a separate process it can't be 
dropped there as incoming message either). So I've tested this manually via 
dropping `RunTaskMessage` in `exec.cpp` and using the following test:
```
TEST_P(CommandExecutorTest, ShutdownWithNoLaunch)
{
  Try> master = StartMaster();
  ASSERT_SOME(master);

  Owned detector = master.get()->createDetector();

  slave::Flags flags = CreateSlaveFlags();
  flags.http_command_executor = false;

  Try> slave = StartSlave(detector.get(), flags);
  ASSERT_SOME(slave);

  MockScheduler sched;
  MesosSchedulerDriver driver(
  &sched, DEFAULT_FRAMEWORK_INFO, master.get()->pid, DEFAULT_CREDENTIAL);

  EXPECT_CALL(sched, registered(&driver, _, _));

  Future> offers;
  EXPECT_CALL(sched, resourceOffers(&driver, _))
.WillOnce(FutureArg<1>(&offers))
.WillRepeatedly(Return()); // Ignore subsequent offers.

  driver.start();

  AWAIT_READY(offers);
  EXPECT_EQ(1u, offers->size());

  // Launch a task with the command executor.
  TaskInfo task = createTask(
  offers->front().slave_id(),
  offers->front().resources(),
  SLEEP_COMMAND(1000));

  driver.launchTasks(offers->front().id(), {task});

  driver.stop();
  driver.join();
}
```


Thanks,

Alexander Rukletsov

Re: Review Request 64032: Promoted log level to warning for disconnected events in exec.cpp.

2017-12-20 Thread Alexander Rukletsov


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

(Updated Dec. 20, 2017, 8:14 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
---

When the executor library receives messages while being disconnected,
it might indicate an out-of-order message delivery or lost messages.
This should be logged at the warning level to simplify triaging.


Diffs (updated)
-

  src/exec/exec.cpp 7fc46daa633ca42944123a7546ac4eceea93956d 


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

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 64741: Eliminated some unnecessary copying in the HTTP operator API.

2017-12-20 Thread Meng Zhu


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




src/master/http.cpp
Lines 4232-4235 (original), 4232-4234 (patched)


how about:

`*getTasks.add_pending_tasks() = protobuf::createTask(taskInfo, 
TASK_STAGING, framework->id());`


- Meng Zhu


On Dec. 19, 2017, 5:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64741/
> ---
> 
> (Updated Dec. 19, 2017, 5:47 p.m.)
> 
> 
> Review request for mesos, Michael Park and Meng Zhu.
> 
> 
> Bugs: MESOS-8344
> https://issues.apache.org/jira/browse/MESOS-8344
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is only a minor portion of the performance improvements
> needed to bring the v1 operator API close to the v0 API
> performance.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp ec170a257c2d309712f3c4b2fce756eb0b530ad6 
> 
> 
> Diff: https://reviews.apache.org/r/64741/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 64726: Renamed operation protos for consistency.

2017-12-20 Thread Mesos Reviewbot Windows


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



FAIL: mesos-java failed to build.

Reviews applied: `['64726']`

Failed command: `cmake.exe --build . --target mesos-java`

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

Relevant logs:

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

```
  D:\DCOS\mesos\mesos\src\messages/messages.hpp(63): error C4430: missing type 
specifier - int assumed. Note: C++ does not support default-int (compiling 
source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\messages/messages.hpp(63): error C2143: syntax error: 
missing ',' before '&' (compiling source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\messages/messages.hpp(73): error C2653: 
'OfferOperationStatusUpdateRecord': is not a class or namespace name (compiling 
source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\messages/messages.hpp(73): error C4430: missing type 
specifier - int assumed. Note: C++ does not support default-int (compiling 
source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\messages/messages.hpp(73): error C2143: syntax error: 
missing ',' before '&' (compiling source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\common/protobuf_utils.hpp(153): error C4430: missing 
type specifier - int assumed. Note: C++ does not support default-int (compiling 
source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\common/protobuf_utils.hpp(153): error C2143: syntax 
error: missing ',' before '&' (compiling source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\common/protobuf_utils.hpp(156): error C4430: missing 
type specifier - int assumed. Note: C++ does not support default-int (compiling 
source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\common/protobuf_utils.hpp(156): error C2146: syntax 
error: missing ';' before identifier 'createOfferOperationStatus' (compiling 
source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\common/protobuf_utils.hpp(158): error C2065: 
'OfferOperationID': undeclared identifier (compiling source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\common/protobuf_utils.hpp(158): error C2923: 
'Option': 'OfferOperationID' is not a valid template type argument for 
parameter 'T' (compiling source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\common/protobuf_utils.hpp(164): error C4430: missing 
type specifier - int assumed. Note: C++ does not support default-int (compiling 
source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\common/protobuf_utils.hpp(164): error C2146: syntax 
error: missing ';' before identifier 'createOfferOperation' (compiling source 
file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\common/protobuf_utils.hpp(172): error C4430: missing 
type specifier - int assumed. Note: C++ does not support default-int (compiling 
source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\common/protobuf_utils.hpp(172): error C2146: syntax 
error: missing ';' before identifier 'createOfferOperationStatusUpdate' 
(compiling source file 
D:\DCOS\mesos\mesos\src\java\jni\org_apache_mesos_v1_scheduler_V0Mesos.cpp) 
[D:\DCOS\mesos\src\java\mesos-java.vcxproj]
  D:\DCOS\mesos\mesos\src\common/protobuf_utils.hpp(175): error C2923: 
'Option': 'mesos::internal::protobuf::OfferOperationStatus' is not

Re: Review Request 64726: Renamed operation protos for consistency.

2017-12-20 Thread Greg Mann


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


Ship it!




Ship It!

- Greg Mann


On Dec. 20, 2017, 7:21 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64726/
> ---
> 
> (Updated Dec. 20, 2017, 7:21 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed operation protos for consistency.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto bf2ec8f4630fb32a36a1a0d49de23e0ddc0c6c84 
>   include/mesos/resource_provider/resource_provider.proto 
> 4534049bce3948f20a00cd704f09173077c8cdf8 
>   include/mesos/scheduler/scheduler.proto 
> a7907e2c0a8e0f23690179b9e334377c4acc9068 
>   include/mesos/v1/mesos.proto 9e240c0756ee9747f1ef226db963dfc6ab5c699a 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> beb9f1734b8d916150ecc6ed1c5b86f0672cd941 
>   include/mesos/v1/scheduler/scheduler.proto 
> e115bca04646133cfa3bbd6e7ba190b69f807f2b 
>   src/messages/messages.proto baefbe97b57d28d70d8952604d12cdc543eaced4 
>   src/resource_provider/state.proto e3362a958851cefc175ee9ba0563da2b8bf8bb08 
> 
> 
> Diff: https://reviews.apache.org/r/64726/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 64726: Renamed operation protos for consistency.

2017-12-20 Thread Jie Yu


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


Ship it!




Ship It!

- Jie Yu


On Dec. 20, 2017, 7:21 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64726/
> ---
> 
> (Updated Dec. 20, 2017, 7:21 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed operation protos for consistency.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto bf2ec8f4630fb32a36a1a0d49de23e0ddc0c6c84 
>   include/mesos/resource_provider/resource_provider.proto 
> 4534049bce3948f20a00cd704f09173077c8cdf8 
>   include/mesos/scheduler/scheduler.proto 
> a7907e2c0a8e0f23690179b9e334377c4acc9068 
>   include/mesos/v1/mesos.proto 9e240c0756ee9747f1ef226db963dfc6ab5c699a 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> beb9f1734b8d916150ecc6ed1c5b86f0672cd941 
>   include/mesos/v1/scheduler/scheduler.proto 
> e115bca04646133cfa3bbd6e7ba190b69f807f2b 
>   src/messages/messages.proto baefbe97b57d28d70d8952604d12cdc543eaced4 
>   src/resource_provider/state.proto e3362a958851cefc175ee9ba0563da2b8bf8bb08 
> 
> 
> Diff: https://reviews.apache.org/r/64726/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 64726: Renamed operation protos for consistency.

2017-12-20 Thread Gaston Kleiman


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

(Updated Dec. 20, 2017, 11:21 a.m.)


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


Changes
---

Addressed Jie's comments.


Repository: mesos


Description
---

Renamed operation protos for consistency.


Diffs (updated)
-

  include/mesos/mesos.proto bf2ec8f4630fb32a36a1a0d49de23e0ddc0c6c84 
  include/mesos/resource_provider/resource_provider.proto 
4534049bce3948f20a00cd704f09173077c8cdf8 
  include/mesos/scheduler/scheduler.proto 
a7907e2c0a8e0f23690179b9e334377c4acc9068 
  include/mesos/v1/mesos.proto 9e240c0756ee9747f1ef226db963dfc6ab5c699a 
  include/mesos/v1/resource_provider/resource_provider.proto 
beb9f1734b8d916150ecc6ed1c5b86f0672cd941 
  include/mesos/v1/scheduler/scheduler.proto 
e115bca04646133cfa3bbd6e7ba190b69f807f2b 
  src/messages/messages.proto baefbe97b57d28d70d8952604d12cdc543eaced4 
  src/resource_provider/state.proto e3362a958851cefc175ee9ba0563da2b8bf8bb08 


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

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


Testing
---


Thanks,

Gaston Kleiman

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

2017-12-20 Thread Mesos Reviewbot Windows


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



PASS: Mesos patch 64033 was successfully built and tested.

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

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

- Mesos Reviewbot Windows


On Dec. 20, 2017, 4:27 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64033/
> ---
> 
> (Updated Dec. 20, 2017, 4:27 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: WIP: Ensured executor adapter propagates error and shutdown messages.

2017-12-20 Thread Mesos Reviewbot Windows


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



PASS: Mesos patch 64070 was successfully built and tested.

Reviews applied: `['64070']`

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

- Mesos Reviewbot Windows


On Dec. 20, 2017, 4:27 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64070/
> ---
> 
> (Updated Dec. 20, 2017, 4:27 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
> ---
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 64726: Renamed operation protos for consistency.

2017-12-20 Thread Jie Yu


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




include/mesos/mesos.proto
Line 2279 (original), 2277 (patched)


Should we call this `uuid` because the context is Operation?



include/mesos/scheduler/scheduler.proto
Line 252 (original), 252 (patched)


ACKNOWLEDGE_OPERATION_STATUS?



include/mesos/scheduler/scheduler.proto
Line 406 (original), 406 (patched)


Should that be `ACKNOWLEDGE_OPERATION_STATUS` to be consistently with RP 
api?



include/mesos/scheduler/scheduler.proto
Lines 491-492 (original), 492 (patched)


Ditto



src/messages/messages.proto
Line 411 (original), 411 (patched)


Either we use OperationUpdateXXX or OperationStatusXXX, but let's be 
consistent.


- Jie Yu


On Dec. 20, 2017, 2:53 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64726/
> ---
> 
> (Updated Dec. 20, 2017, 2:53 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed operation protos for consistency.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto bf2ec8f4630fb32a36a1a0d49de23e0ddc0c6c84 
>   include/mesos/resource_provider/resource_provider.proto 
> 4534049bce3948f20a00cd704f09173077c8cdf8 
>   include/mesos/scheduler/scheduler.proto 
> a7907e2c0a8e0f23690179b9e334377c4acc9068 
>   include/mesos/v1/mesos.proto 9e240c0756ee9747f1ef226db963dfc6ab5c699a 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> beb9f1734b8d916150ecc6ed1c5b86f0672cd941 
>   include/mesos/v1/scheduler/scheduler.proto 
> e115bca04646133cfa3bbd6e7ba190b69f807f2b 
>   src/messages/messages.proto baefbe97b57d28d70d8952604d12cdc543eaced4 
>   src/resource_provider/state.proto e3362a958851cefc175ee9ba0563da2b8bf8bb08 
> 
> 
> Diff: https://reviews.apache.org/r/64726/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

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

2017-12-20 Thread Mesos Reviewbot Windows


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



PASS: Mesos patch 64069 was successfully built and tested.

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

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

- Mesos Reviewbot Windows


On Dec. 20, 2017, 4:07 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64069/
> ---
> 
> (Updated Dec. 20, 2017, 4:07 p.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
> ---
> 
> Writing a proper test is tricky: our test harness does not allow to drop 
> outgoing messages and hence `RunTaskMessage` from the agent to the executor 
> can't be dropped (since executor is running in a separate process it can't be 
> dropped there as incoming message either). So I've tested this manually via 
> dropping `RunTaskMessage` in `exec.cpp` and using the following test:
> ```
> TEST_P(CommandExecutorTest, ShutdownWithNoLaunch)
> {
>   Try> master = StartMaster();
>   ASSERT_SOME(master);
> 
>   Owned detector = master.get()->createDetector();
> 
>   slave::Flags flags = CreateSlaveFlags();
>   flags.http_command_executor = false;
> 
>   Try> slave = StartSlave(detector.get(), flags);
>   ASSERT_SOME(slave);
> 
>   MockScheduler sched;
>   MesosSchedulerDriver driver(
>   &sched, DEFAULT_FRAMEWORK_INFO, master.get()->pid, DEFAULT_CREDENTIAL);
> 
>   EXPECT_CALL(sched, registered(&driver, _, _));
> 
>   Future> offers;
>   EXPECT_CALL(sched, resourceOffers(&driver, _))
> .WillOnce(FutureArg<1>(&offers))
> .WillRepeatedly(Return()); // Ignore subsequent offers.
> 
>   driver.start();
> 
>   AWAIT_READY(offers);
>   EXPECT_EQ(1u, offers->size());
> 
>   // Launch a task with the command executor.
>   TaskInfo task = createTask(
>   offers->front().slave_id(),
>   offers->front().resources(),
>   SLEEP_COMMAND(1000));
> 
>   driver.launchTasks(offers->front().id(), {task});
> 
>   driver.stop();
>   driver.join();
> }
> ```
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

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

2017-12-20 Thread Alexander Rukletsov


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

(Updated Dec. 20, 2017, 4:27 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 (updated)
---


Thanks,

Alexander Rukletsov

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

2017-12-20 Thread Alexander Rukletsov


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

(Updated Dec. 20, 2017, 4:27 p.m.)


Review request for mesos, Andrei Budnik, Anand Mazumdar, Armand Grillet, and 
Vinod Kone.


Summary (updated)
-

WIP: Ensured executor adapter propagates error and shutdown messages.


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
---


Thanks,

Alexander Rukletsov

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

2017-12-20 Thread Alexander Rukletsov


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

(Updated Dec. 20, 2017, 4:27 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 64750: Fixed an agent assertion.

2017-12-20 Thread Jie Yu


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


Ship it!




Let's add some unit test to capture this regression!

- Jie Yu


On Dec. 20, 2017, 2:22 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64750/
> ---
> 
> (Updated Dec. 20, 2017, 2:22 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initially it was valid to assert that 'checkpointResources' would
> never be called for an agent with resource providers, but we have
> sinced moved to an implementation where we might perform checkpointing
> of agent resources (but never local resource provider resources).
> 
> This patch adjusts an assertion in the agent's 'checkpointResources'
> function to that reality.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 264705f9e8fc91407aae691dc9f6d4dc5048dfaa 
> 
> 
> Diff: https://reviews.apache.org/r/64750/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

2017-12-20 Thread Alexander Rukletsov


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

(Updated Dec. 20, 2017, 4:12 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.

2017-12-20 Thread Alexander Rukletsov


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

(Updated Dec. 20, 2017, 4:07 p.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 (updated)
---

Writing a proper test is tricky: our test harness does not allow to drop 
outgoing messages and hence `RunTaskMessage` from the agent to the executor 
can't be dropped (since executor is running in a separate process it can't be 
dropped there as incoming message either). So I've tested this manually via 
dropping `RunTaskMessage` in `exec.cpp` and using the following test:
```
TEST_P(CommandExecutorTest, ShutdownWithNoLaunch)
{
  Try> master = StartMaster();
  ASSERT_SOME(master);

  Owned detector = master.get()->createDetector();

  slave::Flags flags = CreateSlaveFlags();
  flags.http_command_executor = false;

  Try> slave = StartSlave(detector.get(), flags);
  ASSERT_SOME(slave);

  MockScheduler sched;
  MesosSchedulerDriver driver(
  &sched, DEFAULT_FRAMEWORK_INFO, master.get()->pid, DEFAULT_CREDENTIAL);

  EXPECT_CALL(sched, registered(&driver, _, _));

  Future> offers;
  EXPECT_CALL(sched, resourceOffers(&driver, _))
.WillOnce(FutureArg<1>(&offers))
.WillRepeatedly(Return()); // Ignore subsequent offers.

  driver.start();

  AWAIT_READY(offers);
  EXPECT_EQ(1u, offers->size());

  // Launch a task with the command executor.
  TaskInfo task = createTask(
  offers->front().slave_id(),
  offers->front().resources(),
  SLEEP_COMMAND(1000));

  driver.launchTasks(offers->front().id(), {task});

  driver.stop();
  driver.join();
}
```


Thanks,

Alexander Rukletsov

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

2017-12-20 Thread Alexander Rukletsov

> On Dec. 15, 2017, 4:48 p.m., Vinod Kone wrote:
> > src/launcher/executor.cpp
> > Lines 768 (patched)
> > 
> >
> > Can you add a test for this?

Writing test is tricky: our test harness does not allow to drop outgoing 
messages and hence `RunTaskMessage` from the agent to executor can't be dropped 
(since executor is running in a separate process it can't be dropped there as 
incoming message either). We should consider either improving our harness or 
providing an ability to create executors in test processes. For now, I've done 
manual testing by dropping the message in exec.cpp.

- Alexander

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

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 64750: Fixed an agent assertion.

2017-12-20 Thread Mesos Reviewbot Windows


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



PASS: Mesos patch 64750 was successfully built and tested.

Reviews applied: `['64750']`

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

- Mesos Reviewbot Windows


On Dec. 20, 2017, 2:22 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64750/
> ---
> 
> (Updated Dec. 20, 2017, 2:22 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initially it was valid to assert that 'checkpointResources' would
> never be called for an agent with resource providers, but we have
> sinced moved to an implementation where we might perform checkpointing
> of agent resources (but never local resource provider resources).
> 
> This patch adjusts an assertion in the agent's 'checkpointResources'
> function to that reality.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 264705f9e8fc91407aae691dc9f6d4dc5048dfaa 
> 
> 
> Diff: https://reviews.apache.org/r/64750/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 64713: Fixed a crash when resubscribing resource providers.

2017-12-20 Thread Mesos Reviewbot Windows


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



PASS: Mesos patch 64713 was successfully built and tested.

Reviews applied: `['64713']`

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

- Mesos Reviewbot Windows


On Dec. 20, 2017, 1:24 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64713/
> ---
> 
> (Updated Dec. 20, 2017, 1:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Jie Yu.
> 
> 
> Bugs: MESOS-8346
> https://issues.apache.org/jira/browse/MESOS-8346
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a resource provider resubscribed while its old HTTP connection was
> still open, the agent would crash, as a continuation would be called
> erroneously. This continuation is now only called when a HTTP connection
> is closed by a remote side (i.e. the resource provider) and not when
> the resource provider manager closes the connection.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp e3fcb64b630924e1bb497625708cad3f0fdc064a 
> 
> 
> Diff: https://reviews.apache.org/r/64713/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 64750: Fixed an agent assertion.

2017-12-20 Thread Benjamin Bannier


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

Review request for mesos, Jie Yu and Jan Schlicht.


Repository: mesos


Description
---

Initially it was valid to assert that 'checkpointResources' would
never be called for an agent with resource providers, but we have
sinced moved to an implementation where we might perform checkpointing
of agent resources (but never local resource provider resources).

This patch adjusts an assertion in the agent's 'checkpointResources'
function to that reality.


Diffs
-

  src/slave/slave.cpp 264705f9e8fc91407aae691dc9f6d4dc5048dfaa 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 64450: Update docs to mention new flag.

2017-12-20 Thread Mesos Reviewbot Windows


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



PASS: Mesos patch 64450 was successfully built and tested.

Reviews applied: `['64450']`

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

- Mesos Reviewbot Windows


On Dec. 20, 2017, 2:11 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64450/
> ---
> 
> (Updated Dec. 20, 2017, 2:11 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-1739
> https://issues.apache.org/jira/browse/MESOS-1739
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update docs to mention new  flag.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 8b3d3642c50a60b6ce0f9d27031c2077254473de 
>   docs/agent-recovery.md 35cd5b1a38a099d87ab337df116b7356fcaa7c36 
>   docs/configuration/agent.md d629912e391cebd88c9d0daeabc3bbf436344e95 
>   src/slave/flags.cpp d4b95b7542733b22299bc19235cf99220534c022 
> 
> 
> Diff: https://reviews.apache.org/r/64450/diff/2/
> 
> 
> Testing
> ---
> 
> One pass of proof-reading.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64450: Update docs to mention new flag.

2017-12-20 Thread Benno Evers


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

(Updated Dec. 20, 2017, 2:11 p.m.)


Review request for mesos, Alexander Rukletsov and Vinod Kone.


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


Repository: mesos


Description
---

Update docs to mention new  flag.


Diffs
-

  CHANGELOG 8b3d3642c50a60b6ce0f9d27031c2077254473de 
  docs/agent-recovery.md 35cd5b1a38a099d87ab337df116b7356fcaa7c36 
  docs/configuration/agent.md d629912e391cebd88c9d0daeabc3bbf436344e95 
  src/slave/flags.cpp d4b95b7542733b22299bc19235cf99220534c022 


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


Testing
---

One pass of proof-reading.


Thanks,

Benno Evers

Re: Review Request 64749: Fixed flaky `NestedMesosContainerizerTest` tests.

2017-12-20 Thread Andrei Budnik


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

(Updated Dec. 20, 2017, 1:59 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
---

This patch is an addition to commit 0cc636b2d5.


Diffs
-

  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
92832e7716358da789da4b002ee2262e7740326d 


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


Testing
---

1. make check
2. `./src/mesos-tests --gtest_filter=NestedMesosContainerizerTest.* 
--gtest_break_on_failure --gtest_repeat=100 --verbose`


Thanks,

Andrei Budnik

Re: Review Request 64749: Fixed flaky `NestedMesosContainerizerTest` tests.

2017-12-20 Thread Alexander Rukletsov


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


Ship it!




Ship It!

- Alexander Rukletsov


On Dec. 20, 2017, 1:50 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64749/
> ---
> 
> (Updated Dec. 20, 2017, 1:50 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is an addition to commit 0cc636b2d5.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 92832e7716358da789da4b002ee2262e7740326d 
> 
> 
> Diff: https://reviews.apache.org/r/64749/diff/1/
> 
> 
> Testing
> ---
> 
> 1. make check
> 2. `./src/mesos-tests --gtest_filter=NestedMesosContainerizerTest.* 
> --gtest_break_on_failure --gtest_repeat=100 --verbose`
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Review Request 64749: Fixed flaky `NestedMesosContainerizerTest` tests.

2017-12-20 Thread Andrei Budnik


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

Review request for mesos, Alexander Rukletsov and Gilbert Song.


Repository: mesos


Description
---

This patch is an addition to commit 0cc636b2d5.


Diffs
-

  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
92832e7716358da789da4b002ee2262e7740326d 


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


Testing
---

1. make check
2. `./src/mesos-tests --gtest_filter=NestedMesosContainerizerTest.* 
--gtest_break_on_failure --gtest_repeat=100 --verbose`


Thanks,

Andrei Budnik

Re: Review Request 64450: Update docs to mention new flag.

2017-12-20 Thread Alexander Rukletsov


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


Fix it, then Ship it!




I'll fix the last outstanding issue and commit it for you.


docs/agent-recovery.md
Lines 72-73 (patched)


Indentation


- Alexander Rukletsov


On Dec. 20, 2017, 1:26 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64450/
> ---
> 
> (Updated Dec. 20, 2017, 1:26 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update docs to mention new  flag.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 8b3d3642c50a60b6ce0f9d27031c2077254473de 
>   docs/agent-recovery.md 35cd5b1a38a099d87ab337df116b7356fcaa7c36 
>   docs/configuration/agent.md d629912e391cebd88c9d0daeabc3bbf436344e95 
>   src/slave/flags.cpp d4b95b7542733b22299bc19235cf99220534c022 
> 
> 
> Diff: https://reviews.apache.org/r/64450/diff/2/
> 
> 
> Testing
> ---
> 
> One pass of proof-reading.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64450: Update docs to mention new flag.

2017-12-20 Thread Benno Evers



> On Dec. 20, 2017, 11:27 a.m., Alexander Rukletsov wrote:
> > docs/agent-recovery.md
> > Lines 20-22 (original), 21-23 (patched)
> > 
> >
> > Why have you killed this paragraph? Maybe somehow combine it with the 
> > previous one?

It felt redundant, but I restored it now.


> On Dec. 20, 2017, 11:27 a.m., Alexander Rukletsov wrote:
> > docs/agent-recovery.md
> > Line 26 (original), 27 (patched)
> > 
> >
> > s/refuse/will refuse

Looks like both are correct: 
https://english.stackexchange.com/questions/63081/repeating-to-and-will-in-enumerations-of-verbs

I still went ahead and changed it.


> On Dec. 20, 2017, 11:27 a.m., Alexander Rukletsov wrote:
> > docs/agent-recovery.md
> > Lines 51-57 (original), 55-61 (patched)
> > 
> >
> > Any reason to remove this section in this RR?

It's a bit confusing to say that the `recover` option affects the slave 
recovery behaviour when it has to be set to `reconnect` in order to be able to 
even talk about recovery (since setting it to `cleanup` will just turn the 
`mesos-agent` into some cleanup binary that never registers with the master or 
recovers anything)


- Benno


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


On Dec. 20, 2017, 1:26 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64450/
> ---
> 
> (Updated Dec. 20, 2017, 1:26 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update docs to mention new  flag.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 8b3d3642c50a60b6ce0f9d27031c2077254473de 
>   docs/agent-recovery.md 35cd5b1a38a099d87ab337df116b7356fcaa7c36 
>   docs/configuration/agent.md d629912e391cebd88c9d0daeabc3bbf436344e95 
>   src/slave/flags.cpp d4b95b7542733b22299bc19235cf99220534c022 
> 
> 
> Diff: https://reviews.apache.org/r/64450/diff/2/
> 
> 
> Testing
> ---
> 
> One pass of proof-reading.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64713: Fixed a crash when resubscribing resource providers.

2017-12-20 Thread Jan Schlicht



> On Dec. 19, 2017, 4:28 p.m., Benjamin Bannier wrote:
> > src/tests/resource_provider_manager_tests.cpp
> > Line 1150 (original), 1150 (patched)
> > 
> >
> > Let's add a comment here outlining that we start a second RP with the 
> > same ID which will take `resourceProvider1`'s place with the connection 
> > being closed by the manager.

Test case is no longer changed. Dropping.


> On Dec. 19, 2017, 4:28 p.m., Benjamin Bannier wrote:
> > src/tests/resource_provider_manager_tests.cpp
> > Line 1157 (original), 1157 (patched)
> > 
> >
> > It would be great to check here that `resourceProvider1` actually got 
> > disconnected.

I ran into problems doing that. Seems like the `disconnected` callback is never 
called. And if it would, the changes in this test case wouldn't work, because 
the old RP instance would immediately try to resubscribe. This would result in 
both instances always trying to resubscribe, which is not what we want. I've 
removed the test for now, will investigate, why `disconnected` isn't called and 
come up with a separate test case for that. I've created 
https://issues.apache.org/jira/browse/MESOS-8349 for that.


> On Dec. 19, 2017, 4:28 p.m., Benjamin Bannier wrote:
> > src/tests/resource_provider_manager_tests.cpp
> > Line 1184 (original), 1184 (patched)
> > 
> >
> > Let's add a comment here that this closes the connection on the RP side.

Test case is no longer changed. Dropping.


- Jan


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


On Dec. 20, 2017, 2:24 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64713/
> ---
> 
> (Updated Dec. 20, 2017, 2:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Jie Yu.
> 
> 
> Bugs: MESOS-8346
> https://issues.apache.org/jira/browse/MESOS-8346
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a resource provider resubscribed while its old HTTP connection was
> still open, the agent would crash, as a continuation would be called
> erroneously. This continuation is now only called when a HTTP connection
> is closed by a remote side (i.e. the resource provider) and not when
> the resource provider manager closes the connection.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp e3fcb64b630924e1bb497625708cad3f0fdc064a 
> 
> 
> Diff: https://reviews.apache.org/r/64713/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 64450: Update docs to mention new flag.

2017-12-20 Thread Benno Evers


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

(Updated Dec. 20, 2017, 1:26 p.m.)


Review request for mesos, Alexander Rukletsov and Vinod Kone.


Summary (updated)
-

Update docs to mention new  flag.


Repository: mesos


Description
---

Update docs to mention new  flag.


Diffs (updated)
-

  CHANGELOG 8b3d3642c50a60b6ce0f9d27031c2077254473de 
  docs/agent-recovery.md 35cd5b1a38a099d87ab337df116b7356fcaa7c36 
  docs/configuration/agent.md d629912e391cebd88c9d0daeabc3bbf436344e95 
  src/slave/flags.cpp d4b95b7542733b22299bc19235cf99220534c022 


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

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


Testing
---

One pass of proof-reading.


Thanks,

Benno Evers

Re: Review Request 64713: Fixed a crash when resubscribing resource providers.

2017-12-20 Thread Jan Schlicht


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

(Updated Dec. 20, 2017, 2:24 p.m.)


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


Changes
---

Addressed issues and removed test case (because it has some issues that need 
further investigation).


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


Repository: mesos


Description
---

If a resource provider resubscribed while its old HTTP connection was
still open, the agent would crash, as a continuation would be called
erroneously. This continuation is now only called when a HTTP connection
is closed by a remote side (i.e. the resource provider) and not when
the resource provider manager closes the connection.


Diffs (updated)
-

  src/resource_provider/manager.cpp e3fcb64b630924e1bb497625708cad3f0fdc064a 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

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

2017-12-20 Thread Alexander Rukletsov



> On Dec. 18, 2017, 9:40 p.m., Vinod Kone wrote:
> > 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

Other executors already behave correctly.
Docker: 
https://github.com/apache/mesos/blob/5f40f3d401bbdd7e324113214c286a26faf67540/src/docker/executor.cpp#L333
Default: 
https://github.com/apache/mesos/blob/5f40f3d401bbdd7e324113214c286a26faf67540/src/launcher/default_executor.cpp#L966

Kill task path is addressed here: https://reviews.apache.org/r/64033/

I completely agree with the sentiment about bad UX. I'm fine with doing the 
`CHECK`; let's continue discussion in the aforementioned RR.


- Alexander


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


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 64069: Ensured command executor always honors shutdown request.

2017-12-20 Thread Alexander Rukletsov



> On Dec. 19, 2017, 3:25 p.m., Andrei Budnik wrote:
> > src/launcher/executor.cpp
> > Lines 768 (patched)
> > 
> >
> > Let's print warning message before terminating:
> > ```
> > LOG(WARNING) << "Attempted to shutdown the executor with no running 
> > task";
> > ```

I'm not sure we should. Shutdown is issued if, e.g., the scheduler exits, which 
— in contrast to kill task — is not related to the task lifecycle.


- Alexander


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


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 64450: Update docs to mention new '--reconfiguration_policy' flag.

2017-12-20 Thread Alexander Rukletsov


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


Fix it, then Ship it!





docs/agent-recovery.md
Lines 20-21 (original), 21-22 (patched)


I know it's not you, but since you're touching this text, please fix it. If 
these are protobuf messages names, write `TaskInfo` (mind backticks!), if it is 
English text, write "task specification".



docs/agent-recovery.md
Lines 20-22 (original), 21-23 (patched)


Why have you killed this paragraph? Maybe somehow combine it with the 
previous one?



docs/agent-recovery.md
Line 26 (original), 27 (patched)


s/refuse/will refuse



docs/agent-recovery.md
Line 29 (original), 30 (patched)


Let's explicitly mention here that the agent is allowed to recover its ID 
after a host reboot. And it will fall back to wiping the state if the 
configuration is incompatible.



docs/agent-recovery.md
Line 31 (original), 32 (patched)


s/register/will register



docs/agent-recovery.md
Line 40 (original), 39-43 (patched)


Thank you for cleaning this up!



docs/agent-recovery.md
Lines 51-57 (original), 55-61 (patched)


Any reason to remove this section in this RR?



docs/agent-recovery.md
Line 63 (original), 67 (patched)


s/NOTE:/**NOTE:**



docs/configuration/agent.md
Lines 1055 (patched)


Maybe "The old and the new state must match exactly"?



docs/configuration/agent.md
Lines 1057 (patched)


\n" ?



docs/configuration/agent.md
Lines 1058 (patched)


Maybe "to remove or to modify"?



docs/configuration/agent.md
Lines 1061 (patched)


s/slave/agent



docs/configuration/agent.md
Lines 1062 (patched)


s/which/which, e.g.,/


- Alexander Rukletsov


On Dec. 12, 2017, 11:16 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64450/
> ---
> 
> (Updated Dec. 12, 2017, 11:16 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update docs to mention new  flag.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 8b3d3642c50a60b6ce0f9d27031c2077254473de 
>   docs/agent-recovery.md 35cd5b1a38a099d87ab337df116b7356fcaa7c36 
>   docs/configuration/agent.md d629912e391cebd88c9d0daeabc3bbf436344e95 
> 
> 
> Diff: https://reviews.apache.org/r/64450/diff/1/
> 
> 
> Testing
> ---
> 
> One pass of proof-reading.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64743: Enabled function sections.

2017-12-20 Thread Benjamin Bannier

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

cmake/CompilationConfigure.cmake
Line 262 (original), 262 (patched)

Since we have a configure flag to disable these in autotools land, let's 
also add some cmake option to turn this off.

cmake/CompilationConfigure.cmake
Lines 263 (patched)

Let's not depend on the platform, but instead on the toolchain instead. 
This does work on e.g., macos.

cmake/CompilationConfigure.cmake
Lines 264-265 (patched)

Let's merge these lines.

cmake/CompilationConfigure.cmake
Lines 267 (patched)

What about static archives? Last time I looked this was broken in the cmake 
setup, but it would be great to prepare to also make use of this flag there 
once its ready (if applicable).

configure.ac
Lines 178 (patched)

We already have two forms for handling of configure default args, one 
explicit (e.g., right above), and another one for e.g., 
`parallel-test-execution` or `werror`.

I'd suggest to just follow the flow of `werror` instead of introducing yet 
another approach. That would make the eventual cleanup possibly less upsetting.

configure.ac
Lines 507-508 (patched)

Why are we disabling this on macos? These flags do work with the latest 
LLVM toolchain (clang & lld), so let's default-enable it on macos as well.

If you worry about some Apple clang fork not supporting these flags, let's 
please add some feature detection instead of branching on the platform here.

- Benjamin Bannier

On Dec. 20, 2017, 4:50 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64743/
> ---
> 
> (Updated Dec. 20, 2017, 4:50 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Bugs: MESOS-8348
> https://issues.apache.org/jira/browse/MESOS-8348
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If we tell the compiler to place each function in a separate
> section, this allows the linker to garbage collect unused
> sections. This significantly decreases the size of the final
> build artifacts and provides some modest improvements in build
> times.
> 
> This feature is currently only implemented in Linux toolchains,
> so we automatically enable it only on Linux.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake dc9dc161dce1c714748bcec2fc15c4fbfbccaec2 
>   configure.ac 5ffc33a6b9495db64f1ec164ed96f3faea620348 
> 
> 
> Diff: https://reviews.apache.org/r/64743/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

78 matches

Mail list logo