Skip to content

*: update to grpc 1.26.0 #425

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

Merged
merged 5 commits into from
Jan 10, 2020
Merged

*: update to grpc 1.26.0 #425

merged 5 commits into from
Jan 10, 2020

Conversation

BusyJay
Copy link
Member

@BusyJay BusyJay commented Jan 8, 2020

Close #375.

@BusyJay BusyJay added the gRPC Core Related to grpc's c/cpp core label Jan 8, 2020
@BusyJay BusyJay requested review from nrc, overvenus and hicqu January 8, 2020 14:25
slice.0.refcount = Box::into_raw(mem::transmute(ref_count));
slice
unsafe {
GrpcSlice(grpcio_sys::grpc_slice_from_copied_buffer(
Copy link
Contributor

Choose a reason for hiding this comment

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

grpc_slice grpc_slice_new_with_len(void* p, size_t len,
                                   void (*destroy)(void*, size_t)) {
  grpc_slice slice;
  slice.refcount = (new grpc_core::NewWithLenSliceRefcount(destroy, p, len))
                       ->base_refcount();
  slice.data.refcounted.bytes = static_cast<uint8_t*>(p);
  slice.data.refcounted.length = len;
  return slice;
}

Seems we can pass capacity into grpc_slice_new_with_len, and then

slice.data.refcounted.length = v.len();

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 prefer not to rely on the implement details of grpc_slice_new_with_len.

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 just not to rely grpc_core::NewWithLenSliceRefcount is ok. slice.data.refcounted.length is public for us, so we can adjust it if only we can ensure it's less than capacity.
Otherwise memory copy is necessary, I think we need to avoid it.

Copy link
Member Author

Choose a reason for hiding this comment

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

If not rely on grpc_core::NewWithLenSliceRefCount, then you can't for sure whether slice.data.refcounted.length or its internal field is forwarded to destroy hook.

Signed-off-by: Jay Lee <[email protected]>
Signed-off-by: Jay Lee <[email protected]>
nrc
nrc previously approved these changes Jan 9, 2020
Copy link
Contributor

@nrc nrc left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: Jay Lee <[email protected]>
@BusyJay
Copy link
Member Author

BusyJay commented Jan 9, 2020

@nrc @hicqu PTAL.

One breakage is that grpcio probably can't link statically on MacOS that is <= 10.13. A similar issue can be found alexcrichton/curl-rust#279.

@nrc
Copy link
Contributor

nrc commented Jan 10, 2020

One breakage is that grpcio probably can't link statically on MacOS that is <= 10.13

Seems OK, I think most Mac devs stay up to date.

@BusyJay BusyJay merged commit 35d24f1 into tikv:master Jan 10, 2020
@BusyJay BusyJay deleted the update-to-1.26.0 branch January 11, 2020 11:55
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.

3 participants