> On July 26, 2017, 10:49 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/grpc.hpp > > Lines 101 (patched) > > <https://reviews.apache.org/r/61097/diff/2/?file=1782280#file1782280line101> > > > > 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*), > > ```
I changed `T` to `Stub` to reflect that it's the `Stub` class in gRPC's generated code. `method` is renamed to `rpc`. > On July 26, 2017, 10:49 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/grpc.hpp > > Lines 106 (patched) > > <https://reviews.apache.org/r/61097/diff/2/?file=1782280#file1782280line106> > > > > 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`. gRPC requires us to drain the `CompletionQueue` before it gets destructed. Therefore even if we make `Data::loop()` interruptible, we still need to run a loop somewhere to drain the queue. I'd prefer that we call `CompletionQueue::Shutdown()` in `Runtime::terminate()`, then let `Data::loop()` to drain all pending callbacks in the queue. Also, if we send out another call after `CompletionQueue::Shutdown()` is called, the behavior is undocumented. I've reach out to the community to ask if there's a way to do error handling on this scenario, but for now I'll make `Runtime::call()` and `Runtime::terminate()` mutual exclusive and introduce a `Data::terminate` boolean variable (similar to `Future::Data::discard`) to avoid it. Also, I'd like that `Runtime::terminate()` also trigggers the terminatation of the process managed by `data` after it processes all pending callbacks. Currently I make `Runtime::terminate()` and `Runtime::wait()` similar to `process::terminate()` and `process::wait()`, so `Runtime::wait()` returns a bool instead of a `Future`. Please refer to my updated patch and share your thoughts about my changes. > On July 26, 2017, 10:49 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/grpc.hpp > > Lines 123 (patched) > > <https://reviews.apache.org/r/61097/diff/2/?file=1782280#file1782280line123> > > > > who is going to delete the lambda function? > > Chun-Hung Hsiao wrote: > This lambda will be retrieved and managed by a `shared_ptr` in > `client::Runtime::Data::loop()`. When it will be deleted depends on if the > call is successful. If so, the `shared_ptr` will be captured by another > lambda that is dispatched to `client::Runtime::process`, and thus this lambda > will be deleted after execution; otherwise, this lambda will be deleted at > the end of the lifecycle of the `shared_ptr`. It is required that a > `CompletionQueue` must be drained before getting destructed, so a regular > termination process would call `CompletionQueue::Shutdown()` which makes > `CompletionQueue::Next()` to return pending calls as failures in > `client::Runtime::Data::loop()`, thus all allocated lambdas will eventually > be deleted. Added comments to explain that it will be managed in `Data::loop()`. > On July 26, 2017, 10:49 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/grpc.hpp > > Lines 124 (patched) > > <https://reviews.apache.org/r/61097/diff/2/?file=1782280#file1782280line124> > > > > 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? According to http://en.cppreference.com/w/cpp/language/lambda, my interpretation is that they won't be optimized out. @mpark also said so. > On July 26, 2017, 10:49 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/grpc.hpp > > Lines 148 (patched) > > <https://reviews.apache.org/r/61097/diff/2/?file=1782280#file1782280line148> > > > > 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`? > > Chun-Hung Hsiao wrote: > We don't need such a restriction, since the code won't compile if the > inferred signature cannot match any methods in the stub class of grpc. Added a static assertion. - Chun-Hung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61097/#review181469 ----------------------------------------------------------- On July 27, 2017, 10:24 p.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61097/ > ----------------------------------------------------------- > > (Updated July 27, 2017, 10:24 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/3/ > > > Testing > ------- > > N/A > > > Thanks, > > Chun-Hung Hsiao > >
