mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@mesosphere.io>
Subject Re: Review Request 61097: Added gRPC support in libprocess.
Date Thu, 27 Jul 2017 22:24:47 GMT


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


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