mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 67157: Refactored the gRPC client runtime wrapper in libprocess.
Date Thu, 17 May 2018 14:22:26 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message