----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61097/#review181469 -----------------------------------------------------------
Good work! See my detailed comments. I'll do a second pass once the existing comments are addressed. 3rdparty/libprocess/include/process/grpc.hpp Lines 49 (patched) <https://reviews.apache.org/r/61097/#comment257101> Let's comment on this class. One important thing to call out here is that: each instance of this class consists a separate completion queue. So if folks want parallelism or isolation, one can choose to launch multiple instances of this class. Also, I'd rename this class to `Runtime` and put it under `client` namespace: ``` grpc::client::Runtime ...; ``` 3rdparty/libprocess/include/process/grpc.hpp Lines 52 (patched) <https://reviews.apache.org/r/61097/#comment257078> style nits: we usually put parathesis for constructor calls without arguments: ``` new Looper() ``` 3rdparty/libprocess/include/process/grpc.hpp Lines 55 (patched) <https://reviews.apache.org/r/61097/#comment257104> I would s/Looper/Data/ This is more than just a looper thread. 3rdparty/libprocess/include/process/grpc.hpp Lines 62 (patched) <https://reviews.apache.org/r/61097/#comment257105> I would call this `looper`. s/thread/looper/ 3rdparty/libprocess/include/process/grpc.hpp Lines 93 (patched) <https://reviews.apache.org/r/61097/#comment257109> I think you can get rid of this struct if `call` is part of `client::Runtime`? 3rdparty/libprocess/include/process/grpc.hpp Lines 101 (patched) <https://reviews.apache.org/r/61097/#comment257108> s/method/stub/ I'd also restructure this a bit (indentation for function paramters should be 4: ``` std::unique_ptr<::grpc::ClientAsyncResponseReader<Response>>(T::*stub)( ::grpc::ClientContext*, const Request&, ::grpc::CompletionQueue*), ``` 3rdparty/libprocess/include/process/grpc.hpp Lines 106 (patched) <https://reviews.apache.org/r/61097/#comment257113> In fact, I would introduce a `terminated` field in `client::Runtime::Data`, and introduce a `wait` method for `client::Runtime`: ``` class Runtime { public: // The future will become ready when the runtime is terminated; Future<Nothing> wait() { return data->terminating.future() .then(defer(process, [=]() { // NOTE: This is a blocking call. However, the thread is // guaranteed to be exiting, therefore the amount of time in // blocking state is bounded (just like other syscalls we // invoke). looper->join(); return Nothing(); })); } void terminate() { // This will signal the looper thread to exit. data->terminate.test_and_set(); } private: struct Data { ... std::atomic_flag terminate; Promise<Nothing> terminating; }; }; ``` You probably want to use `AsyncNext` rather than `Next` so that the looper thread can be interrupted (by always checking `data->terminate`. 3rdparty/libprocess/include/process/grpc.hpp Lines 112 (patched) <https://reviews.apache.org/r/61097/#comment257080> Can you add a TODO here to support allowing the caller to specify a timeout? 3rdparty/libprocess/include/process/grpc.hpp Lines 123 (patched) <https://reviews.apache.org/r/61097/#comment257115> who is going to delete the lambda function? 3rdparty/libprocess/include/process/grpc.hpp Lines 124 (patched) <https://reviews.apache.org/r/61097/#comment257116> I would comment on why you need to capture those field that are not used in the lambda function. Is it possible that compiler optimize it away? 3rdparty/libprocess/include/process/grpc.hpp Lines 145-154 (patched) <https://reviews.apache.org/r/61097/#comment257103> Can we make this a member function of `Runtime`: ``` grpc::client::Runtime client; client.call(channel, rpc, request); ``` 3rdparty/libprocess/include/process/grpc.hpp Lines 146 (patched) <https://reviews.apache.org/r/61097/#comment257079> no need for `inline` for template. template is always inlined. 3rdparty/libprocess/include/process/grpc.hpp Lines 148 (patched) <https://reviews.apache.org/r/61097/#comment257107> I would use the name `Stub` here. ``` auto call( const Channel& channel, const Stub& stub, const Request& request); ``` Also, is there a way to restrict that `Request` is a subclass of `protobuf::Message`? - Jie Yu On July 25, 2017, 9:13 p.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61097/ > ----------------------------------------------------------- > > (Updated July 25, 2017, 9:13 p.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 use `process::grpc::call(...)` to send an asynchronous > gRPC call and get a future for the response. The client needs to set up > two data structures: a `Channel` which represents a connection to a gRPC > server, and a `ClientRuntime` which maintains a `CompletionQueue` that > keeps track of all pending asynchronous gRPC calls, and spawns a thread > waiting for any response from the `CompletionQueue`. All gRPC calls > using the same `ClientRuntime` would share the same 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/2/ > > > Testing > ------- > > N/A > > > Thanks, > > Chun-Hung Hsiao > >
