You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@mesosphere.io> on 2017/07/25 21:13:34 UTC
Re: Review Request 61097: Added gRPC support in libprocess.
-----------------------------------------------------------
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.
Changes
-------
Refactored and moved CMake-related updates into a separated patch: https://reviews.apache.org/r/61118/
Summary (updated)
-----------------
Added gRPC support in libprocess.
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 (updated)
-----
3rdparty/libprocess/include/process/grpc.hpp PRE-CREATION
3rdparty/libprocess/src/grpc.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/61097/diff/2/
Changes: https://reviews.apache.org/r/61097/diff/1-2/
Testing (updated)
-------
N/A
Thanks,
Chun-Hung Hsiao
Re: Review Request 61097: Added gRPC support in libprocess.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61097/#review181713
-----------------------------------------------------------
3rdparty/libprocess/include/process/grpc.hpp
Lines 43 (patched)
<https://reviews.apache.org/r/61097/#comment257354>
Can we call this `GRPC_RPC`?
- 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
>
>
Re: Review Request 61097: Added gRPC support in libprocess.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 61097: Added gRPC support in libprocess.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61097/
-----------------------------------------------------------
(Updated Aug. 2, 2017, 7:19 p.m.)
Review request for mesos, Benjamin Mahler, Jie Yu, and Joseph Wu.
Changes
-------
Addressed Jie's comments.
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 (updated)
-----
3rdparty/libprocess/include/process/grpc.hpp PRE-CREATION
3rdparty/libprocess/src/grpc.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/61097/diff/5/
Changes: https://reviews.apache.org/r/61097/diff/4-5/
Testing
-------
N/A
Thanks,
Chun-Hung Hsiao
Re: Review Request 61097: Added gRPC support in libprocess.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61097/#review181723
-----------------------------------------------------------
3rdparty/libprocess/include/process/grpc.hpp
Lines 130-133 (patched)
<https://reviews.apache.org/r/61097/#comment257374>
I am wondering if we should set the promise if status is OK, even if `promise->future.hasDiscard()` is true?
- 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
>
>
Re: Review Request 61097: Added gRPC support in libprocess.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
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 (updated)
-------
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
Re: Review Request 61097: Added gRPC support in libprocess.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61097/
-----------------------------------------------------------
(Updated July 28, 2017, 12:49 a.m.)
Review request for mesos, Benjamin Mahler, Jie Yu, and Joseph Wu.
Changes
-------
`Runtime::wait()` now returns a `Future` that waits for joining the looper thread. `Runtime::Data` will now waits for its process to terminate during destruction.
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 (updated)
-----
3rdparty/libprocess/include/process/grpc.hpp PRE-CREATION
3rdparty/libprocess/src/grpc.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/61097/diff/4/
Changes: https://reviews.apache.org/r/61097/diff/3-4/
Testing
-------
N/A
Thanks,
Chun-Hung Hsiao
Re: Review Request 61097: Added gRPC support in libprocess.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Addressed Jie's comments.
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 (updated)
-----
3rdparty/libprocess/include/process/grpc.hpp PRE-CREATION
3rdparty/libprocess/src/grpc.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/61097/diff/3/
Changes: https://reviews.apache.org/r/61097/diff/2-3/
Testing
-------
N/A
Thanks,
Chun-Hung Hsiao
Re: Review Request 61097: Added gRPC support in libprocess.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> 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`?
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.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61097/#review181469
-----------------------------------------------------------
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
>
>
Re: Review Request 61097: Added gRPC support in libprocess.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> 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
>
>
Re: Review Request 61097: Added gRPC support in libprocess.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> 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?
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.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61097/#review181469
-----------------------------------------------------------
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
>
>
Re: Review Request 61097: Added gRPC support in libprocess.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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
>
>