You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2018/05/16 19:27:23 UTC
Review Request 67157: Refactored the gRPC client runtime wrapper in
libprocess.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
Bugs: MESOS-8924
https://issues.apache.org/jira/browse/MESOS-8924
Repository: mesos
Description
-------
The refactoring does the following things:
1. Manage the gRPC completion queue and the looper thread in the runtime
process to get rid of a lock in `Runtime::Data`.
2. Move the computation of sending a request into the runtime process.
3. Let libprocess manage the runtime process automatically instead of
managing its lifecycle in `Runtime::Data`.
Diffs
-----
3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
Diff: https://reviews.apache.org/r/67157/diff/1/
Testing
-------
make check in libprocess
NOTE: Mesos cannot be built with this patch standalone. The tests are done later in the chain.
Thanks,
Chun-Hung Hsiao
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/#review203394
-----------------------------------------------------------
FAIL: Some of the unit tests failed. Please check the relevant logs.
Reviews applied: `['67164', '67190', '67191', '67154', '67155', '67156', '67158', '67157']`
Failed command: `Start-MesosCITesting`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67157
Relevant logs:
- [stout-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67157/logs/stout-tests-cmake-stdout.log):
```
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\logging.cc(640): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\logging.cc(773): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\logging.cc(798): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\logging.cc(808): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\logging.cc(1092): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\logging.cc(1092): warning C4267: 'initializing': conversion from 'size_t' to 'const int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\logging.cc(1353): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\logging.cc(1353): warning C4267: 'initializing': conversion from 'size_t' to 'const int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\logging.cc(1426): warning C4267: 'argument': conversion from 'size_t' to 'const int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\logging.cc(1448): warning C4267: 'argument': conversion from 'size_t' to 'unsigned int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\logging.cc(1535): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\logging.cc(1548): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\raw_logging.cc(153): warning C4267: 'argument': conversion from 'size_t' to 'unsigned int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\utilities.cc(82): warning C4267: 'argument': conversion from 'size_t' to 'unsigned int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\vlog_is_on.cc(163): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
d:\dcos\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70\src\vlog_is_on.cc(163): warning C4267: 'initializing': conversion from 'size_t' to 'const int', possible loss of data [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.14.26428\include\xutility(2483): warning C4996: 'std::copy::_Unchecked_iterators::_Deprecate': Call to 'std::copy' with parameters that may be unsafe - this call relies on the caller to check that the passed values are correct. To disable this warning, use -D_SCL_SECURE_NO_WARNINGS. See documentation on how to use Visual C++ 'Checked Iterators' [D:\DCOS\mesos\3rdparty\glog-da816ea70\src\glog-da816ea70-build\glog.vcxproj] [D:\DCOS\mesos\3rdparty\glog-da816ea70.vcxproj]
"D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj" (default target) (1) ->
(ClCompile target) ->
d:\dcos\mesos\mesos\3rdparty\stout\include\stout\windows\error.hpp(101): error C3861: 'stringify': identifier not found (compiling source file D:\DCOS\mesos\mesos\3rdparty\stout\tests\base64_tests.cpp) [D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
d:\dcos\mesos\mesos\3rdparty\stout\include\stout\windows\error.hpp(101): error C3861: 'stringify': identifier not found (compiling source file D:\DCOS\mesos\mesos\3rdparty\stout\tests\interval_tests.cpp) [D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
d:\dcos\mesos\mesos\3rdparty\stout\include\stout\windows\error.hpp(101): error C3861: 'stringify': identifier not found (compiling source file D:\DCOS\mesos\mesos\3rdparty\stout\tests\path_tests.cpp) [D:\DCOS\mesos\3rdparty\stout\tests\stout-tests.vcxproj]
39 Warning(s)
3 Error(s)
Time Elapsed 00:04:12.69
```
- [libprocess-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67157/logs/libprocess-tests-cmake-stdout.log):
```
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(874): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_core.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(1275): warning C4244: 'function': conversion from 'intptr_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_core.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(1456): warning C4244: 'function': conversion from 'intptr_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_core.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\buffer.c(3099): warning C4244: 'function': conversion from 'int64_t' to 'unsigned int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\evutil_time.c(499): warning C4244: '=': conversion from 'int64_t' to 'long', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\evutil_time.c(504): warning C4244: '=': conversion from 'time_t' to 'long', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(185): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(230): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(587): warning C4244: '=': conversion from 'SSIZE_T' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(598): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(654): warning C4244: '=': conversion from 'SSIZE_T' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(656): warning C4244: '=': conversion from 'SSIZE_T' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(675): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(755): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(763): warning C4244: '=': conversion from 'SSIZE_T' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(874): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(1275): warning C4244: 'function': conversion from 'intptr_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\bufferevent_openssl.c(1456): warning C4244: 'function': conversion from 'intptr_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
d:\dcos\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta\evdns.c(3792): warning C4996: 'GetVersion': was declared deprecated [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta\src\libevent-2.1.5-beta-build\event_extra.vcxproj] [D:\DCOS\mesos\3rdparty\libevent-2.1.5-beta.vcxproj]
"D:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj" (default target) (1) ->
(ClCompile target) ->
d:\dcos\mesos\mesos\3rdparty\stout\include\stout\windows\error.hpp(101): error C3861: 'stringify': identifier not found (compiling source file D:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\metrics_tests.cpp) [D:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj]
112 Warning(s)
1 Error(s)
Time Elapsed 00:05:20.26
```
- [mesos-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67157/logs/mesos-tests-cmake-stdout.log):
```
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205): warning C4716: 'pthread_cond_wait': must return a value [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124): warning C4996: 'fopen': This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(543): warning C4996: 'strcpy': This function or variable may be unsafe. Consider using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(548): warning C4996: 'fopen': This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(569): warning C4996: 'strcpy': This function or variable may be unsafe. Consider using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
"D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
"D:\DCOS\mesos\src\slave\mesos-agent.vcxproj" (default target) (12) ->
"D:\DCOS\mesos\src\launcher\mesos-executor.vcxproj" (default target) (35) ->
(ClCompile target) ->
d:\dcos\mesos\mesos\3rdparty\stout\include\stout\windows\error.hpp(101): error C3861: 'stringify': identifier not found [D:\DCOS\mesos\src\launcher\mesos-executor.vcxproj]
"D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
"D:\DCOS\mesos\src\tests\test-helper.vcxproj" (default target) (24) ->
d:\dcos\mesos\mesos\3rdparty\stout\include\stout\windows\error.hpp(101): error C3861: 'stringify': identifier not found (compiling source file D:\DCOS\mesos\mesos\src\tests\flags.cpp) [D:\DCOS\mesos\src\tests\test-helper.vcxproj]
d:\dcos\mesos\mesos\3rdparty\stout\include\stout\windows\error.hpp(101): error C3861: 'stringify': identifier not found (compiling source file D:\DCOS\mesos\mesos\src\tests\resources_utils.cpp) [D:\DCOS\mesos\src\tests\test-helper.vcxproj]
172 Warning(s)
3 Error(s)
Time Elapsed 00:17:11.60
```
- Mesos Reviewbot Windows
On May 17, 2018, 10:23 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> -----------------------------------------------------------
>
> (Updated May 17, 2018, 10:23 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
> process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
> managing its lifecycle in `Runtime::Data`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
> 3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
>
>
> Diff: https://reviews.apache.org/r/67157/diff/4/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/#review203446
-----------------------------------------------------------
Patch looks great!
Reviews applied: [67164, 67190, 67191, 67154, 67155, 67156, 67158, 67157]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 17, 2018, 10:23 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> -----------------------------------------------------------
>
> (Updated May 17, 2018, 10:23 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
> process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
> managing its lifecycle in `Runtime::Data`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
> 3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
>
>
> Diff: https://reviews.apache.org/r/67157/diff/4/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On May 25, 2018, 5:12 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/grpc.cpp
> > Lines 70 (patched)
> > <https://reviews.apache.org/r/67157/diff/4/?file=2024986#file2024986line80>
> >
> > I am slightly worried that we only `join` this thread on `terminate`. Could we make sure we `join` the thread whenever `looper` gets `reset` or goes out of scope -- we might be able to e.g., use a custom deleter for that if the lifetimes are guaranteed to be correct.
Maybe as a simpler solution, let's call `looper.reset()` here and add a destructor
Runtime::RuntimeProcess::~RuntimeProcess()
{
CHECK(!looper);
}
to make sure on the language level (in addition to libprocess `Process` semantics) that we never leak this thread.
> On May 25, 2018, 5:12 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/grpc.cpp
> > Lines 76 (patched)
> > <https://reviews.apache.org/r/67157/diff/4/?file=2024986#file2024986line86>
> >
> > This seems almost like the libprocess equivalent of a throwing destructor. Could we trigger this e.g., if we fail a test `ASSERT`?
> >
> > Also see my comment regarding joining whenever `looper` goes out of scope.
>
> Chun-Hung Hsiao wrote:
> Given that we never directly call `process::terminate` on a `RuntimeProcess`, but only indirectly terminate the process through `RuntimeProcess::terminate` which also sets `terminating`, this cannot fail at any case, including failed test assertions. I could remove this if this line looks weird to you.
Makes sense, let's leave the assertion in here.
Dropped
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/#review203871
-----------------------------------------------------------
On May 18, 2018, 12:23 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> -----------------------------------------------------------
>
> (Updated May 18, 2018, 12:23 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
> process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
> managing its lifecycle in `Runtime::Data`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
> 3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
>
>
> Diff: https://reviews.apache.org/r/67157/diff/4/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On May 25, 2018, 3:12 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/grpc.cpp
> > Lines 76 (patched)
> > <https://reviews.apache.org/r/67157/diff/4/?file=2024986#file2024986line86>
> >
> > This seems almost like the libprocess equivalent of a throwing destructor. Could we trigger this e.g., if we fail a test `ASSERT`?
> >
> > Also see my comment regarding joining whenever `looper` goes out of scope.
Given that we never directly call `process::terminate` on a `RuntimeProcess`, but only indirectly terminate the process through `RuntimeProcess::terminate` which also sets `terminating`, this cannot fail at any case, including failed test assertions. I could remove this if this line looks weird to you.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/#review203871
-----------------------------------------------------------
On May 17, 2018, 10:23 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> -----------------------------------------------------------
>
> (Updated May 17, 2018, 10:23 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
> process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
> managing its lifecycle in `Runtime::Data`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
> 3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
>
>
> Diff: https://reviews.apache.org/r/67157/diff/4/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On May 25, 2018, 3:12 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/grpc.cpp
> > Lines 70 (patched)
> > <https://reviews.apache.org/r/67157/diff/4/?file=2024986#file2024986line80>
> >
> > I am slightly worried that we only `join` this thread on `terminate`. Could we make sure we `join` the thread whenever `looper` gets `reset` or goes out of scope -- we might be able to e.g., use a custom deleter for that if the lifetimes are guaranteed to be correct.
>
> Benjamin Bannier wrote:
> Maybe as a simpler solution, let's call `looper.reset()` here and add a destructor
>
> Runtime::RuntimeProcess::~RuntimeProcess()
> {
> CHECK(!looper);
> }
>
> to make sure on the language level (in addition to libprocess `Process` semantics) that we never leak this thread.
Will do. I'll also add `CHECK(!looper)` before `looper.reset(new ...)`.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/#review203871
-----------------------------------------------------------
On May 17, 2018, 10:23 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> -----------------------------------------------------------
>
> (Updated May 17, 2018, 10:23 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
> process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
> managing its lifecycle in `Runtime::Data`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
> 3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
>
>
> Diff: https://reviews.apache.org/r/67157/diff/4/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/#review203871
-----------------------------------------------------------
3rdparty/libprocess/include/process/grpc.hpp
Lines 27 (patched)
<https://reviews.apache.org/r/67157/#comment286205>
Since you are touching includes, let's make sure we include what is required.
We should add
#include <process/process.hpp> // Process
#include <stout/errorbase.hpp> // Error
#include <stout/nothing.hpp> // Nothing
#include <string> // string
We should remove
#include <atomic>
#include <stout/duration.hpp>
#include <stout/synchronized.hpp>
3rdparty/libprocess/include/process/grpc.hpp
Lines 152-154 (patched)
<https://reviews.apache.org/r/67157/#comment286232>
Let's rewrap this for better readability,
typename std::enable_of<
std::is_convertible<
typename std::decay<Request>::type*,
google::protobuf::Message*>::value,
int>::type = 0>
3rdparty/libprocess/src/grpc.cpp
Lines 70 (patched)
<https://reviews.apache.org/r/67157/#comment286234>
I am slightly worried that we only `join` this thread on `terminate`. Could we make sure we `join` the thread whenever `looper` gets `reset` or goes out of scope -- we might be able to e.g., use a custom deleter for that if the lifetimes are guaranteed to be correct.
3rdparty/libprocess/src/grpc.cpp
Lines 76 (patched)
<https://reviews.apache.org/r/67157/#comment286233>
This seems almost like the libprocess equivalent of a throwing destructor. Could we trigger this e.g., if we fail a test `ASSERT`?
Also see my comment regarding joining whenever `looper` goes out of scope.
- Benjamin Bannier
On May 18, 2018, 12:23 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> -----------------------------------------------------------
>
> (Updated May 18, 2018, 12:23 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
> process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
> managing its lifecycle in `Runtime::Data`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
> 3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
>
>
> Diff: https://reviews.apache.org/r/67157/diff/4/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/#review204481
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Bannier
On June 8, 2018, 3:21 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> -----------------------------------------------------------
>
> (Updated June 8, 2018, 3:21 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
> process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
> managing its lifecycle in `Runtime::Data`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp cf165f0ca1fe3f4abea9117024ac97c25f6f31dc
> 3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
>
>
> Diff: https://reviews.apache.org/r/67157/diff/5/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/
-----------------------------------------------------------
(Updated June 8, 2018, 1:21 a.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
Changes
-------
Addressed Benjamin's comments.
Bugs: MESOS-8924
https://issues.apache.org/jira/browse/MESOS-8924
Repository: mesos
Description
-------
The refactoring does the following things:
1. Manage the gRPC completion queue and the looper thread in the runtime
process to get rid of a lock in `Runtime::Data`.
2. Move the computation of sending a request into the runtime process.
3. Let libprocess manage the runtime process automatically instead of
managing its lifecycle in `Runtime::Data`.
Diffs (updated)
-----
3rdparty/libprocess/include/process/grpc.hpp cf165f0ca1fe3f4abea9117024ac97c25f6f31dc
3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
Diff: https://reviews.apache.org/r/67157/diff/5/
Changes: https://reviews.apache.org/r/67157/diff/4-5/
Testing
-------
sudo make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/
-----------------------------------------------------------
(Updated May 17, 2018, 10:23 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
Changes
-------
Addressed Benjamin's comments.
Bugs: MESOS-8924
https://issues.apache.org/jira/browse/MESOS-8924
Repository: mesos
Description
-------
The refactoring does the following things:
1. Manage the gRPC completion queue and the looper thread in the runtime
process to get rid of a lock in `Runtime::Data`.
2. Move the computation of sending a request into the runtime process.
3. Let libprocess manage the runtime process automatically instead of
managing its lifecycle in `Runtime::Data`.
Diffs (updated)
-----
3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
Diff: https://reviews.apache.org/r/67157/diff/4/
Changes: https://reviews.apache.org/r/67157/diff/3-4/
Testing
-------
sudo make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/#review203302
-----------------------------------------------------------
Patch looks great!
Reviews applied: [67164, 67154, 67155, 67156, 67158, 67157]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 16, 2018, 8:49 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> -----------------------------------------------------------
>
> (Updated May 16, 2018, 8:49 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
> process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
> managing its lifecycle in `Runtime::Data`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
> 3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
>
>
> Diff: https://reviews.apache.org/r/67157/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/#review203297
-----------------------------------------------------------
PASS: Mesos patch 67157 was successfully built and tested.
Reviews applied: `['67154', '67155', '67156', '67158', '67157']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67157
- Mesos Reviewbot Windows
On May 16, 2018, 10:49 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> -----------------------------------------------------------
>
> (Updated May 16, 2018, 10:49 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
> process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
> managing its lifecycle in `Runtime::Data`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
> 3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
>
>
> Diff: https://reviews.apache.org/r/67157/diff/3/
>
>
> Testing
> -------
>
> make check in libprocess
>
> NOTE: Mesos cannot be built with this patch standalone. The tests are done later in the chain.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On May 17, 2018, 11:12 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/grpc.hpp
> > Line 154 (original), 154 (patched)
> > <https://reviews.apache.org/r/67157/diff/3/?file=2024192#file2024192line156>
> >
> > Do we want to actively disallow passing in lvalue `request`s or is this just the implementation we currently need?
> >
> > In the latter case let's just remove the `&&` so the type of `request` can be deduced as either lvalue or rvalue, and then `std::forward` like we currently do.
> >
> > Same argument applies to existing the `Method` parameter (noted in https://reviews.apache.org/r/67155/).
This is a universal reference so it does not forbid lvalues.
> On May 17, 2018, 11:12 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/grpc.hpp
> > Line 157 (original), 157-159 (patched)
> > <https://reviews.apache.org/r/67157/diff/3/?file=2024192#file2024192line159>
> >
> > Not added here, but this `static_assert` triggers to late (after we have already resolved the function to use).
> >
> > We should e.g., add an `enable_if` to the template parameters of this function so SFINAE can take effect.
We already has the same assertion in the codebase: https://github.com/apache/mesos/blob/40b40d9b73221388e583fc140280f1eb2b48b832/3rdparty/stout/include/stout/protobuf.hpp#L695
It seems to me that whether this should be a SFINAE depends on if we want a hard error (for better error messaging) or a soft error (so we could introduce other template specialization in the future). If you think SFINAE makes more sense, I could do it in `MethodTraits` and keep the declaration here simple.
> On May 17, 2018, 11:12 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/grpc.hpp
> > Line 208 (original), 217 (patched)
> > <https://reviews.apache.org/r/67157/diff/3/?file=2024192#file2024192line226>
> >
> > Can we avoid introducing this pattern, even if it works here? The state managed by the `shared_ptr` is shared and we could should copy here.
These shared pointers are workarounds for unique pointers, and are actually not shared. Using move here is to avoid an potentially expensive protobuf copy.
> On May 17, 2018, 11:12 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/grpc.hpp
> > Line 238 (original), 257-258 (patched)
> > <https://reviews.apache.org/r/67157/diff/3/?file=2024192#file2024192line267>
> >
> > Just take values here and force callers to `std::move`. `CallableOnce` is already non-copiable, so we don't need to protect against users passing in shared data.
I'm following the same pattern in our codebase: https://github.com/apache/mesos/blob/40b40d9b73221388e583fc140280f1eb2b48b832/3rdparty/libprocess/include/process/future.hpp#L172
Do you think it makes more sense to enforce the caller to move?
> On May 17, 2018, 11:12 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/grpc.hpp
> > Lines 260 (patched)
> > <https://reviews.apache.org/r/67157/diff/3/?file=2024192#file2024192line270>
> >
> > Do you think it would make sense to change `terminate` to return this result instead and remove `wait`?
> >
> > Future<Nothing> terminate();
Keeping them separated make more sense, since it enables other copies of `Runtime` to wait as well.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/#review203323
-----------------------------------------------------------
On May 16, 2018, 8:49 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> -----------------------------------------------------------
>
> (Updated May 16, 2018, 8:49 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
> process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
> managing its lifecycle in `Runtime::Data`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
> 3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
>
>
> Diff: https://reviews.apache.org/r/67157/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/#review203323
-----------------------------------------------------------
3rdparty/libprocess/include/process/grpc.hpp
Line 154 (original), 154 (patched)
<https://reviews.apache.org/r/67157/#comment285441>
Do we want to actively disallow passing in lvalue `request`s or is this just the implementation we currently need?
In the latter case let's just remove the `&&` so the type of `request` can be deduced as either lvalue or rvalue, and then `std::forward` like we currently do.
Same argument applies to existing the `Method` parameter (noted in https://reviews.apache.org/r/67155/).
3rdparty/libprocess/include/process/grpc.hpp
Line 157 (original), 157-159 (patched)
<https://reviews.apache.org/r/67157/#comment285444>
Not added here, but this `static_assert` triggers to late (after we have already resolved the function to use).
We should e.g., add an `enable_if` to the template parameters of this function so SFINAE can take effect.
3rdparty/libprocess/include/process/grpc.hpp
Line 161 (original), 163 (patched)
<https://reviews.apache.org/r/67157/#comment285443>
We could add `TODO` to make this a `std::unique_ptr` once we use at least C++14 which allows generalized lambda captures (which can `move` capture).
3rdparty/libprocess/include/process/grpc.hpp
Line 165 (original), 180 (patched)
<https://reviews.apache.org/r/67157/#comment285445>
Could also be a `unique_ptr` in C++14 and beyond.
3rdparty/libprocess/include/process/grpc.hpp
Line 208 (original), 217 (patched)
<https://reviews.apache.org/r/67157/#comment285446>
Can we avoid introducing this pattern, even if it works here? The state managed by the `shared_ptr` is shared and we could should copy here.
3rdparty/libprocess/include/process/grpc.hpp
Line 209 (original), 218 (patched)
<https://reviews.apache.org/r/67157/#comment285447>
Ditto.
3rdparty/libprocess/include/process/grpc.hpp
Lines 224 (patched)
<https://reviews.apache.org/r/67157/#comment285442>
We need to `#include <utility>` for `std::forward`.
3rdparty/libprocess/include/process/grpc.hpp
Line 238 (original), 257-258 (patched)
<https://reviews.apache.org/r/67157/#comment285448>
Just take values here and force callers to `std::move`. `CallableOnce` is already non-copiable, so we don't need to protect against users passing in shared data.
3rdparty/libprocess/include/process/grpc.hpp
Lines 260 (patched)
<https://reviews.apache.org/r/67157/#comment285450>
Do you think it would make sense to change `terminate` to return this result instead and remove `wait`?
Future<Nothing> terminate();
3rdparty/libprocess/include/process/grpc.hpp
Lines 263-264 (patched)
<https://reviews.apache.org/r/67157/#comment285449>
No need to make these `protected` instead of `private` as any derived class would always want to override the `protected` methods from `ProcessBase`, not these functions here.
That would require some rewiring of the termination status.
- Benjamin Bannier
On May 16, 2018, 10:49 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> -----------------------------------------------------------
>
> (Updated May 16, 2018, 10:49 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
> process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
> managing its lifecycle in `Runtime::Data`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
> 3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
>
>
> Diff: https://reviews.apache.org/r/67157/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/#review203290
-----------------------------------------------------------
FAIL: Failed to apply the dependent review: 67155.
Failed command: `python.exe .\support\apply-reviews.py -n -r 67155`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67157
Relevant logs:
- [apply-review-67155-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67157/logs/apply-review-67155-stdout.log):
```
error: patch failed: 3rdparty/libprocess/src/tests/grpc_tests.cpp:124
error: 3rdparty/libprocess/src/tests/grpc_tests.cpp: patch does not apply
```
- Mesos Reviewbot Windows
On May 16, 2018, 8:49 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67157/
> -----------------------------------------------------------
>
> (Updated May 16, 2018, 8:49 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The refactoring does the following things:
> 1. Manage the gRPC completion queue and the looper thread in the runtime
> process to get rid of a lock in `Runtime::Data`.
> 2. Move the computation of sending a request into the runtime process.
> 3. Let libprocess manage the runtime process automatically instead of
> managing its lifecycle in `Runtime::Data`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
> 3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
>
>
> Diff: https://reviews.apache.org/r/67157/diff/2/
>
>
> Testing
> -------
>
> make check in libprocess
>
> NOTE: Mesos cannot be built with this patch standalone. The tests are done later in the chain.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67157: Refactored the gRPC client runtime wrapper
in libprocess.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67157/
-----------------------------------------------------------
(Updated May 16, 2018, 8:49 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
Bugs: MESOS-8924
https://issues.apache.org/jira/browse/MESOS-8924
Repository: mesos
Description
-------
The refactoring does the following things:
1. Manage the gRPC completion queue and the looper thread in the runtime
process to get rid of a lock in `Runtime::Data`.
2. Move the computation of sending a request into the runtime process.
3. Let libprocess manage the runtime process automatically instead of
managing its lifecycle in `Runtime::Data`.
Diffs (updated)
-----
3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
3rdparty/libprocess/src/grpc.cpp a80bcb614ec96d92d21bc88a281d3208e86141a0
Diff: https://reviews.apache.org/r/67157/diff/2/
Changes: https://reviews.apache.org/r/67157/diff/1-2/
Testing
-------
make check in libprocess
NOTE: Mesos cannot be built with this patch standalone. The tests are done later in the chain.
Thanks,
Chun-Hung Hsiao