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