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