-
Notifications
You must be signed in to change notification settings - Fork 60
Make agent grpc-gateway work with grpc 1.18+ #501
Make agent grpc-gateway work with grpc 1.18+ #501
Conversation
cc/ @songy23 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov Report
@@ Coverage Diff @@
## master #501 +/- ##
=======================================
Coverage 58.81% 58.81%
=======================================
Files 69 69
Lines 4507 4507
=======================================
Hits 2651 2651
Misses 1692 1692
Partials 164 164
Continue to review full report at Codecov.
|
@draffensperger we reverted grpc to 1.17.0 since there were other things broken. It looks like that we may skip v1.18.0 will this PR still be needed in this case? |
I'd suggest to put this PR on hold and merge it later once we roll forward the proto (and gRPC) updates. |
…-service into fix-grpc-gateway
The main thing I would like is for the agent grpc-gateway functionality to continue to work as future changes are made, since I'm using that for the opencensus-web project to write spans via HTTP. I'm open to waiting until the grpc dependency is updated but then I'd like this change to be done in the same PR as the upgrade to preserve the functionality. Given that, I actually think it would be simpler to just merge this now so that there will be one less thing to think about when upgrading to grpc 1.18 or 1.19. I updated the PR title and description to talk about this as a preparatory change for the upgrade rather than a fix. |
Makes sense @draffensperger. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that, I actually think it would be simpler to just merge this now so that there will be one less thing to think about when upgrading to grpc 1.18 or 1.19. I updated the PR title and description to talk about this as a preparatory change for the upgrade rather than a fix.
SGTM
The
google.golang.org/grpc
v1.18.0 release includes a change that made handshakes required by default. That when combined with thecmux
multiplexer caused an "all SubConns are in TransientFailure" error message for HTTP calls made via grpc-gateway (see this issue comment for more details). The fix is to use a setting that cmux offers that handles the handshake correctly.Our grpc 1.18 upgrade was rolled back in #503 so this is not immediately needed, but having it in place will make the grpc-gateway functionality continue to work once the grpc dependency is upgraded past 1.18.
See #492 - this will enable the grpc-gateway test to keep passing (and the functionality to keep working) after the future grpc upgrade.