mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 61097: Added gRPC support in libprocess.
Date Fri, 28 Jul 2017 19:09:25 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61097/#review181682
-----------------------------------------------------------


Fix it, then Ship it!





3rdparty/libprocess/include/process/grpc.hpp
Lines 53 (patched)
<https://reviews.apache.org/r/61097/#comment257344>

    Let's add some add some comments about this. Also, let's use Doxygen style comments for
public symbols:
    ```
    /**
     *
     */
    ```
    
    Ditto below for `Runtime`



3rdparty/libprocess/include/process/grpc.hpp
Lines 75 (patched)
<https://reviews.apache.org/r/61097/#comment257304>

    single instance of `Runtime` to handle



3rdparty/libprocess/include/process/grpc.hpp
Lines 90 (patched)
<https://reviews.apache.org/r/61097/#comment257303>

    indentation should be 4 here (function arguments)



3rdparty/libprocess/include/process/grpc.hpp
Lines 102 (patched)
<https://reviews.apache.org/r/61097/#comment257306>

    I would do the following to avoid jagdness:
    ```
    synchronized (data->lock) {
      if (data->terminating) {
        return Failure("Client runtime is terminating");
      }
      
      ...
    }
    ```



3rdparty/libprocess/include/process/grpc.hpp
Lines 105 (patched)
<https://reviews.apache.org/r/61097/#comment257311>

    insert a blank line above.



3rdparty/libprocess/include/process/grpc.hpp
Lines 119-121 (patched)
<https://reviews.apache.org/r/61097/#comment257310>

    I would prefer the following (not sure it compiles or not):
    ```
    std::shared_ptr<::grpc::ClientAsyncResponseReader<Response>> reader(
        (Stub(channel.channel).*rpc)(context.get(), request, &data->queue));
    ```



3rdparty/libprocess/include/process/grpc.hpp
Lines 122-123 (patched)
<https://reviews.apache.org/r/61097/#comment257317>

    Add a blank line above this line
    
    Also, I would adjust the style a bit here:
    ```
    reader->Finish(
        response.get(),
        status.get(),
        new lambda::function<void()>(
            // ...
            [...]() {
              ...
            }));
    ```



3rdparty/libprocess/include/process/grpc.hpp
Lines 147 (patched)
<https://reviews.apache.org/r/61097/#comment257345>

    I won't mention `data` in the public comments. Imagine how a caller will understand `data`
in the comment?



3rdparty/libprocess/include/process/grpc.hpp
Lines 151 (patched)
<https://reviews.apache.org/r/61097/#comment257346>

    Ditto on `data`
    
    ALso, i would mention that all pending RPC calls will be drained.



3rdparty/libprocess/include/process/grpc.hpp
Lines 168 (patched)
<https://reviews.apache.org/r/61097/#comment257305>

    s/shutdown/terminating/



3rdparty/libprocess/src/grpc.cpp
Lines 40 (patched)
<https://reviews.apache.org/r/61097/#comment257347>

    new line above


- Jie Yu


On July 28, 2017, 1:09 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61097/
> -----------------------------------------------------------
> 
> (Updated July 28, 2017, 1:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7810
>     https://issues.apache.org/jira/browse/MESOS-7810
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> A gRPC client can create a `process::grpc::client::Runtime` instance and
> use its `call()` method to send an asynchronous gRPC call to `Channel`
> (representing a connection to a gRPC server), and get a future waiting
> for the response. The `Runtime` class maintains a `CompletionQueue` to
> manage all pending asynchronous gRPC calls, and spawns a thread waiting
> for any response from the `CompletionQueue`. All gRPC calls using the
> same `Runtime` copy would share the same `CompletionQueue` and thread.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/grpc.hpp PRE-CREATION 
>   3rdparty/libprocess/src/grpc.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61097/diff/4/
> 
> 
> Testing
> -------
> 
> N/A
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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