Re: Review Request 67156: Renamed `grpc::Channel` to `grpc::client::Connection`.

[Chun-Hung Hsiao]

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

Re: Review Request 67156: Renamed `grpc::Channel` to `grpc::client::Connection`.

[Benjamin Bannier]

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


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)


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)


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