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