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

Reply via email to