Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

chore: upgrade grpc to v1.26.0 #1167

Closed
wants to merge 1 commit into from

Conversation

scotthart
Copy link
Member

@scotthart scotthart commented Jan 6, 2020

This change is Reviewable

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 6, 2020
@codecov
Copy link

codecov bot commented Jan 6, 2020

Codecov Report

Merging #1167 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1167      +/-   ##
==========================================
+ Coverage    95.7%   95.74%   +0.03%     
==========================================
  Files         174      174              
  Lines       13601    13602       +1     
==========================================
+ Hits        13017    13023       +6     
+ Misses        584      579       -5
Impacted Files Coverage Δ
...anner/integration_tests/client_integration_test.cc 98.47% <0%> (-0.01%) ⬇️
google/cloud/spanner/keys.h 100% <0%> (ø) ⬆️
google/cloud/spanner/samples/samples.cc 86.44% <0%> (+0.28%) ⬆️
google/cloud/spanner/client.cc 97.41% <0%> (+0.86%) ⬆️
...loud/spanner/internal/partial_result_set_resume.cc 95.45% <0%> (+4.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27048c7...e80992c. Read the comment docs.

Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

Do we understand why the vptr change is not needed here? Maybe because we do not define any gRPC servers?

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@scotthart
Copy link
Member Author

Do we understand why the vptr change is not needed here? Maybe because we do not define any gRPC servers?

In the bigtable library we use/pass arguments of type grpc::ServerContext* in multiple places. This class directly inherits from grpc_impl::ServerContextBase, which had one method become virtual in the new version, v1.26.0. The ubsan errors observed from the bigtable repo were:

undefined reference to 'typeinfo for grpc_impl::ServerContextBase'

In spanner, we only use grpc::GenericServerContext, once, as part of our testing. grpc::GenericServerContext inherits from grpc::ServerContext

Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@coryan
Copy link
Contributor

coryan commented Jan 24, 2020

FWIW, the 1.26.x branch allegedly has fixes for the problems you are seeing.

@devbww
Copy link
Contributor

devbww commented Feb 3, 2020

This looks like it is probably obsolete after the recent merge of #1232.

@scotthart scotthart closed this Feb 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants