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

Glide update and comment out usage of grpc-opentracing until fixed #996

Merged
merged 2 commits into from
May 4, 2017

Conversation

bufdev
Copy link
Contributor

@bufdev bufdev commented May 4, 2017

Here's the situation:

  • grpc/grpc-go pushed a breaking change to their metadata API when they released 1.3.0
  • If you call metadata.NewContext twice, the second call overrides the first, and metadata is lost Metadata context changes are not backwards compatible/break existing code grpc/grpc-go#1219
  • grpc-ecosystem/grpc-opentracing did not have a corresponding release to handle this, causing yarpc/transport/x/grpc to break when we glide update (headers are no longer sent from client to server)

I commented out usages of grpc-opentracing for now until grpc-ecosystem/grpc-opentracing#29 is merged, and changed grpc-go in glide to be pinned to minor versions.

grpc-opentracing doesn't actually have releases either, so this makes it more problematic. We may want to review using it altogether.

@mention-bot
Copy link

@peter-edge, thanks for your PR! By analyzing the history of the files in this pull request, we identified @abhinav to be a potential reviewer.

@codecov
Copy link

codecov bot commented May 4, 2017

Codecov Report

Merging #996 into dev will decrease coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #996      +/-   ##
==========================================
- Coverage   80.66%   80.48%   -0.18%     
==========================================
  Files         194      194              
  Lines        9160     9158       -2     
==========================================
- Hits         7389     7371      -18     
- Misses       1411     1427      +16     
  Partials      360      360
Flag Coverage Δ
#experimental 70.68% <ø> (-0.91%) ⬇️
#main 82.94% <ø> (ø) ⬆️
Impacted Files Coverage Δ
transport/x/grpc/outbound.go 76.82% <ø> (-0.28%) ⬇️
transport/x/grpc/inbound.go 83.09% <ø> (-0.24%) ⬇️
transport/x/grpc/options.go 25% <0%> (-25%) ⬇️
transport/x/grpc/handler.go 53.03% <0%> (-15.16%) ⬇️

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 e4dc746...e502586. Read the comment docs.

Copy link
Contributor

@HelloGrayson HelloGrayson left a comment

Choose a reason for hiding this comment

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

Sure - let's not block on opentracing integration while we're figuring out the core support story.

@bufdev bufdev merged commit 0492cbd into dev May 4, 2017
@bufdev bufdev deleted the grpc-fix branch May 4, 2017 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants