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...@apache.org> on 2018/05/16 19:25:24 UTC
Review Request 67156: Renamed `grpc::Channel` to
`grpc::client::Connection`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67156/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
Bugs: MESOS-8924
https://issues.apache.org/jira/browse/MESOS-8924
Repository: mesos
Description
-------
This renaming is made to avoid name conflicts between `::grpc::Channel`
and libprocess' own wrapper. Also, since this wrapper is only used
at the client side, it is moved into the `client` namespace.
NOTE: The name `client::Connection` matches the `ClientConn` function
in gRPC Go API, so it should be a good name for `::grpc::Channel`.
Diffs
-----
3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
3rdparty/libprocess/src/tests/grpc_tests.cpp 38cd6c61b54518a1019bb11a3551be13026c3f0d
Diff: https://reviews.apache.org/r/67156/diff/1/
Testing
-------
make check in libprocess
NOTE: Mesos cannot be built with this patch standalone. The tests are done later in the chain.
Thanks,
Chun-Hung Hsiao
Re: Review Request 67156: Renamed `grpc::Channel` to
`grpc::client::Connection`.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67156/#review203320
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/libprocess/include/process/grpc.hpp
Lines 37-40 (original), 37-40 (patched)
<https://reviews.apache.org/r/67156/#comment285428>
Nit: You could drop the parens for symmetry with `client::Runtime` and adjust punctuation,
// defines two wrapper classes: `client::Connection` which represents a
// connection to a gRPC server, and `client::Runtime` which integrates an event
// loop waiting for gRPC responses and provides the `call` interface to create
// an asynchronous gRPC call and returns a `Future`.
3rdparty/libprocess/include/process/grpc.hpp
Lines 92 (patched)
<https://reviews.apache.org/r/67156/#comment285429>
Maybe expand while we are at it?
* All `Connection` copies share the same gRPC channel.
It might make sense to mention that `::grpc::Channel` is thread safe so there is no need to manage concurrent access.
3rdparty/libprocess/include/process/grpc.hpp
Lines 101 (patched)
<https://reviews.apache.org/r/67156/#comment285433>
Taking the `shared_ptr` by `const` ref here while taking ownership seems to be inspired by `::grpc::CreateChannel` itself (where it probably makes a copy) so we might just do the same, but it is weird smart pointer usage.
No need to do anything.
- Benjamin Bannier
On May 16, 2018, 9:25 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67156/
> -----------------------------------------------------------
>
> (Updated May 16, 2018, 9:25 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8924
> https://issues.apache.org/jira/browse/MESOS-8924
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This renaming is made to avoid name conflicts between `::grpc::Channel`
> and libprocess' own wrapper. Also, since this wrapper is only used
> at the client side, it is moved into the `client` namespace.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
> 3rdparty/libprocess/src/tests/grpc_tests.cpp 38cd6c61b54518a1019bb11a3551be13026c3f0d
>
>
> Diff: https://reviews.apache.org/r/67156/diff/3/
>
>
> Testing
> -------
>
> make check in libprocess
>
> NOTE: Mesos cannot be built with this patch standalone. The tests are done later in the chain.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 67156: Renamed `grpc::Channel` to
`grpc::client::Connection`.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67156/
-----------------------------------------------------------
(Updated May 17, 2018, 7:37 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Zhitao Li.
Changes
-------
Addressed Benjamin's comments.
Bugs: MESOS-8924
https://issues.apache.org/jira/browse/MESOS-8924
Repository: mesos
Description
-------
This renaming is made to avoid name conflicts between `::grpc::Channel`
and libprocess' own wrapper. Also, since this wrapper is only used
at the client side, it is moved into the `client` namespace.
Diffs (updated)
-----
3rdparty/libprocess/include/process/grpc.hpp 321a46e19c69eafb24012bcef68bb8b0cc6aa436
3rdparty/libprocess/src/tests/grpc_tests.cpp 38cd6c61b54518a1019bb11a3551be13026c3f0d
Diff: https://reviews.apache.org/r/67156/diff/4/
Changes: https://reviews.apache.org/r/67156/diff/3-4/
Testing
-------
make check in libprocess
NOTE: Mesos cannot be built with this patch standalone. The tests are done later in the chain.
Thanks,
Chun-Hung Hsiao