Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade gRPC to v1.14.2 #203

Merged
merged 26 commits into from
Sep 14, 2018
Merged

Upgrade gRPC to v1.14.2 #203

merged 26 commits into from
Sep 14, 2018

Conversation

overvenus
Copy link
Member

@overvenus overvenus commented Jul 6, 2018

This PR upgrades the gRPC to v1.10.0, replaces core alarm with grpc++'s. Its ABI is compatible with the offical grpc.

Please do not merge this until tikv/grpc#12 gets merged.

TODO:

  • Fix macOS build.
  • Fix Windows build.
  • Fix timeout_on_sleeping_server

Cc #197
Close #171

cc.define("GRPC_SYS_SECURE", None);
"grpc"
("grpc", "grpc++")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest gRPC use grpcpp and may deprecate grpc++, we need to add comments here and don't forget to change it later.

Btw, maybe we can upgrade to the gRPC 1.13 directly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont worry, it's the name grpc++ library, it is not changed in the gRFC L22 .
It only changes the include path grpcpp/....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but I think we should use grpcpp, it is recommended officially.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpcpp is a include path. grpc++ is the name of cpp library, it's still grpc++, and there is no library called grpcpp.
See it's Cmakefile https://github.com/grpc/grpc/blob/v1.13.0/CMakeLists.txt#L2689
and Makefile https://github.com/grpc/grpc/blob/v1.13.0/Makefile#L1380

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you use C++ gRPC, you can find that the generated files and examples now all use cpp header.

https://github.com/grpc/grpc/blob/master/examples/cpp/route_guide/route_guide_client.cc#L28

/** Create a completion queue alarm instance */
GPR_EXPORT grpcwrap_alarm *GPR_CALLTYPE
grpcwrap_alarm_create(void) {
void *ptr = gpr_malloc(sizeof(Alarm));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

em, seem you can malloc with sizeof(Alarm) + sizeof(grpcwrap_alarm) for both Alarm and wapper, so we don't need to alloc and dealloc twice.

@siddontang
Copy link
Contributor

This PR is awesome, coooooooooool!

Copy link
Member

@BusyJay BusyJay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.travis.yml Outdated
- cargo build --no-default-features
- cargo build --no-default-features --features protobuf-codec
- cargo build
- cargo test --all
- GRPCIO_SYS_USE_PKG_CONFIG=1 cargo test --all
- GRPCIO_SYS_USE_PKG_CONFIG=1 cargo build --all
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have trouble in linking grpcio's test targets, need help. 🆘

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give some more information?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From: https://travis-ci.org/overvenus/grpc-rs/jobs/400903660

   Compiling grpcio-sys v0.2.3 (file:///home/travis/build/overvenus/grpc-rs/grpc-sys)
   Compiling grpcio v0.3.0 (file:///home/travis/build/overvenus/grpc-rs)
error: linking with `cc` failed: exit code: 1
  |
  = note: "cc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-m64" "-L" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.1v9koybaboeosxiy.rcgu.o" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.1y16o1qfye96o7m0.rcgu.o" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.2rnarwe9869ff3em.rcgu.o" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.2xs38k3nu4oe9uo6.rcgu.o" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.3rngp6bm2u2q5z0y.rcgu.o" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.4oc10dk278mpk1vy.rcgu.o" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.4xq48u46a1pwiqn7.rcgu.o" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.51s1w397y42gpez1.rcgu.o" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.5et8vp13oj1m4v7a.rcgu.o" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.8xzrsc1ux72v29j.rcgu.o" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.9elsx31vb4it187.rcgu.o" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.oa3rad818d8sgn4.rcgu.o" "-o" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/grpcio_sys-6001af5e1c5efae1.crate.allocator.rcgu.o" "-Wl,--gc-sections" "-pie" "-Wl,-z,relro,-z,now" "-nodefaultlibs" "-L" "/home/travis/build/overvenus/grpc-rs/target/debug/deps" "-L" "/home/travis/.cache/lib" "-L" "/home/travis/.cache/lib" "-L" "/home/travis/build/overvenus/grpc-rs/target/debug/build/grpcio-sys-afd36be57b22e64e/out" "-L" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-l" "grpc" "-l" "grpc++" "-Wl,-Bstatic" "-Wl,--whole-archive" "-l" "grpc_wrap" "-Wl,--no-whole-archive" "-Wl,-Bdynamic" "-l" "stdc++" "-Wl,-Bstatic" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libtest-315d48fd46f254fe.rlib" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libterm-4dd46751eaaa3cb4.rlib" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libgetopts-b0cb061f9f47a202.rlib" "/home/travis/build/overvenus/grpc-rs/target/debug/deps/liblibc-76755a6888618a72.rlib" "-Wl,--start-group" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-2e0e6fd5a64772ec.rlib" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libpanic_unwind-731e169e757c346a.rlib" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc_jemalloc-095fc2295ae9ef1b.rlib" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libunwind-937988d834c61738.rlib" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc_system-b7ac4cdefbcbe74e.rlib" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liblibc-2acd14cf338f9037.rlib" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc-abc00040df319771.rlib" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcore-c03be6943b237af4.rlib" "-Wl,--end-group" "/home/travis/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcompiler_builtins-7d0cd09e2c099c63.rlib" "-Wl,-Bdynamic" "-l" "util" "-l" "util" "-l" "dl" "-l" "rt" "-l" "pthread" "-l" "pthread" "-l" "gcc_s" "-l" "c" "-l" "m" "-l" "rt" "-l" "pthread" "-l" "util" "-l" "util"
  = note: /home/travis/build/overvenus/grpc-rs/target/debug/build/grpcio-sys-afd36be57b22e64e/out/libgrpc_wrap.a(grpc_wrap.o):(.data.rel._ZN4grpc8internalL14g_core_codegenE+0x0): undefined reference to `vtable for grpc::CoreCodegen'
          /home/travis/build/overvenus/grpc-rs/target/debug/build/grpcio-sys-afd36be57b22e64e/out/libgrpc_wrap.a(grpc_wrap.o): In function `grpc::CoreCodegen::~CoreCodegen()':
          /home/travis/.cache/include/grpcpp/impl/codegen/core_codegen.h:32: undefined reference to `vtable for grpc::CoreCodegen'
          collect2: error: ld returned 1 exit status

It may be related to grpc/grpc#13500

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I see the same error:

   Compiling clap v2.31.2
   Compiling grpcio-sys v0.2.3 (file:///home/hoverbear/git/grpc-rs/grpc-sys)
   Compiling grpcio-compiler v0.3.0 (file:///home/hoverbear/git/grpc-rs/compiler)
   Compiling grpcio v0.3.0 (file:///home/hoverbear/git/grpc-rs)
   Compiling grpcio-proto v0.3.0 (file:///home/hoverbear/git/grpc-rs/proto)
error: linking with `cc` failed: exit code: 1
  |
  = note: "cc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-m64" "-L" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.1v9koybaboeosxiy.rcgu.o" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.1y16o1qfye96o7m0.rcgu.o" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.2rnarwe9869ff3em.rcgu.o" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.2xs38k3nu4oe9uo6.rcgu.o" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.3rngp6bm2u2q5z0y.rcgu.o" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.4oc10dk278mpk1vy.rcgu.o" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.4xq48u46a1pwiqn7.rcgu.o" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.51s1w397y42gpez1.rcgu.o" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.5et8vp13oj1m4v7a.rcgu.o" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.8xzrsc1ux72v29j.rcgu.o" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.9elsx31vb4it187.rcgu.o" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.oa3rad818d8sgn4.rcgu.o" "-o" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03" "/home/hoverbear/git/grpc-rs/target/debug/deps/grpcio_sys-04563ee405ba0b03.crate.allocator.rcgu.o" "-Wl,--gc-sections" "-pie" "-Wl,-z,relro,-z,now" "-nodefaultlibs" "-L" "/home/hoverbear/git/grpc-rs/target/debug/deps" "-L" "/usr/lib" "-L" "/usr/lib" "-L" "/home/hoverbear/git/grpc-rs/target/debug/build/grpcio-sys-ac23585aa46eac93/out" "-L" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-l" "grpc" "-l" "grpc++" "-Wl,-Bstatic" "-Wl,--whole-archive" "-l" "grpc_wrap" "-Wl,--no-whole-archive" "-Wl,-Bdynamic" "-l" "stdc++" "-Wl,-Bstatic" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libtest-019326da6686230a.rlib" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libterm-7f8a78dfff7bac57.rlib" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libgetopts-dbfbcb47517df890.rlib" "/home/hoverbear/git/grpc-rs/target/debug/deps/liblibc-77d5831ef1267f52.rlib" "-Wl,--start-group" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-0888da25bc5aa659.rlib" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libpanic_unwind-c21a3f7e9b2ce71d.rlib" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc_jemalloc-a89ec74248f6f925.rlib" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libunwind-9232e45013d81e94.rlib" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc_system-afbe0a28ca4ea99f.rlib" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liblibc-bcf95ff3aa4e3276.rlib" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc-26716181a02171e3.rlib" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcore-978afee7e21437ec.rlib" "-Wl,--end-group" "/home/hoverbear/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcompiler_builtins-3f4ad009a7cf735f.rlib" "-Wl,-Bdynamic" "-l" "util" "-l" "util" "-l" "dl" "-l" "rt" "-l" "pthread" "-l" "pthread" "-l" "gcc_s" "-l" "c" "-l" "m" "-l" "rt" "-l" "pthread" "-l" "util" "-l" "util"
  = note: /home/hoverbear/git/grpc-rs/target/debug/build/grpcio-sys-ac23585aa46eac93/out/libgrpc_wrap.a(grpc_wrap.o):(.data.rel._ZN4grpc8internalL14g_core_codegenE+0x0): undefined reference to `vtable for grpc::CoreCodegen'
          /home/hoverbear/git/grpc-rs/target/debug/build/grpcio-sys-ac23585aa46eac93/out/libgrpc_wrap.a(grpc_wrap.o): In function `grpc::CoreCodegen::~CoreCodegen()':
          /usr/include/grpcpp/impl/codegen/core_codegen.h:32: undefined reference to `vtable for grpc::CoreCodegen'
          collect2: error: ld returned 1 exit status

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to wait for the official team fixing it or just work around it? @overvenus

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It have been fixed in e2639c3.

@overvenus overvenus changed the title Upgrade gRPC to v1.10.0, take 2 Upgrade gRPC to v1.13.0, take 2 Jul 9, 2018
@siddontang
Copy link
Contributor

do you use https://github.com/grpc/grpc/blob/master/.clang-format to format the CPP codes?

@overvenus
Copy link
Member Author

PTAL @siddontang #206 .

@huachaohuang
Copy link

I may need some time to scan the gRPC code before I can review this PR.

/// Consumes the `MockTag`, returning the wrapped raw pointer.
pub fn into_raw(self) -> *mut GrpcwrapTag {
if let MockTag::Spawn(_) = self {
panic!("MockTag::Spawn can not into raw pointer")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can not turn into a raw pointer

CompletionQueue* cq;
} grpcwrap_completion_queue;

/** Create a ComletionQueue, a shadow of grpc_completion_queue.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CompletionQueue

/** Set a completion queue alarm instance associated to a cq.
*
* Once the alarm notifies (see a grpcwrap_alarm_notify) or it's destroy (see a
* grpcwrap_alarm_destroy), an event with tag a tag will be added to a cq. If the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You repeat yourself here an event with tag a tag


/** Set a completion queue alarm instance associated to a cq.
*
* Once the alarm notifies (see a grpcwrap_alarm_notify) or it's destroy (see a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mention grpcwrap_alarm_notify but I do not see anything called this. Can you help me find it?

hoverbear@nomad:git/grpc-rs ‹upgrade-1.10-1*›$ rg grpcwrap_alarm_notify
grpc-sys/grpc_wrap.cc
888: * Once the alarm notifies (see a grpcwrap_alarm_notify) or it's destroy (see a

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, it should be grpcwrap_alarm_cancel. 🤦‍♂️

/** A helper class that implements `CompletionQueueTag`.
*
* TODO: Remove it. The `grpc::Alarm` submit itself as a tag to a
* `CompletionQueue`, so we needs it to unify extraction of tags`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need it

@overvenus
Copy link
Member Author

overvenus commented Jul 14, 2018

src/cq.rs Outdated
@@ -110,6 +117,7 @@ impl CompletionQueueHandle {
impl Drop for CompletionQueueHandle {
fn drop(&mut self) {
unsafe { grpc_sys::grpc_completion_queue_destroy(self.cq) }
// TOOD: free cq_shadow.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to address this before merging so we don't have a memory leak.

@overvenus overvenus changed the title Upgrade gRPC to v1.13.0, take 2 Upgrade gRPC to v1.14.2 Sep 10, 2018
@@ -133,6 +135,14 @@ impl TestService for InteropTestService {
if let Some(param) = req.get_response_parameters().get(0) {
resp.set_payload(util::new_payload(param.get_size() as usize));
}
// A Workaround for timeout_on_sleeping_server test.
// The request only has 27182 bytes of zeros in payload.
if req.get_payload().get_body().len() == 27182
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, 27182 is so magic 😭

@siddontang
Copy link
Contributor

seem we need to upgrade the Cargo version now.

siddontang
siddontang previously approved these changes Sep 11, 2018
Copy link
Contributor

@siddontang siddontang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

.travis.yml Outdated
@@ -8,6 +8,7 @@ cache:
- $TRAVIS_BUILD_DIR/target
before_cache:
- find $TRAVIS_BUILD_DIR/target/debug -maxdepth 1 -type f -delete
- cargo clean -p grpcio-sys
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

.travis.yml Outdated
fi
- >-
if [[ ! -f "$GRPC_HEADER" ]] ; then
(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

.travis.yml Outdated
git clone -b v$GRPC_VERSION https://github.com/grpc/grpc &&
cd grpc &&
git submodule update --init &&
env prefix=$HOME/.cache make install_c install_cxx &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is cxx still necessary?

.travis.yml Outdated
- export PATH="$PATH:$HOME/.cache/bin:$HOME/.cargo/bin"
- GRPC_HEADER="$HOME/.cache/include/grpc/grpc.h"
- if [[ $TRAVIS_OS_NAME == "osx" ]] && [[ ! -f $GRPC_HEADER ]]; then export CC=clang; brew update && brew install autoconf libtool shtool; fi
- test -f "$GRPC_HEADER" || (git clone -b v1.7.2 https://github.com/grpc/grpc && cd grpc && git submodule update --init && env prefix=$HOME/.cache make install_c)
- >-
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think >- is necessary if if is moved up.

src/cq.rs Outdated
@@ -109,7 +109,9 @@ impl CompletionQueueHandle {

impl Drop for CompletionQueueHandle {
fn drop(&mut self) {
unsafe { grpc_sys::grpc_completion_queue_destroy(self.cq) }
unsafe {
grpc_sys::grpc_completion_queue_destroy(self.cq);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change?


if use_pkg_config {
// Link libgrpc.so.
probe_library(library, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it still necessary to probe the library twice?

@@ -133,6 +135,14 @@ impl TestService for InteropTestService {
if let Some(param) = req.get_response_parameters().get(0) {
resp.set_payload(util::new_payload(param.get_size() as usize));
}
// A Workaround for timeout_on_sleeping_server test.
// The request only has 27182 bytes of zeros in payload.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it still necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can set it to 1ms, and you will find the client may receive a response.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too hacky. And comment should talk about the problem this hack tries to work around.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A friendly ping.

@overvenus
Copy link
Member Author

PTAL, thanks!

@overvenus
Copy link
Member Author

A friendly ping.

BusyJay
BusyJay previously approved these changes Sep 14, 2018
Copy link
Contributor

@siddontang siddontang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@overvenus overvenus merged commit b47f32f into master Sep 14, 2018
@overvenus overvenus deleted the upgrade-1.10-1 branch September 14, 2018 13:46
@siddontang
Copy link
Contributor

@overvenus

please update the Cargo version

bacek pushed a commit to bacek/grpc-rs that referenced this pull request Oct 9, 2018
@ice1000 ice1000 mentioned this pull request Oct 10, 2018
@ice1000 ice1000 added the gRPC Core Related to grpc's c/cpp core label Dec 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gRPC Core Related to grpc's c/cpp core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants