-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix gRPC query service cmux #1756
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1756 +/- ##
==========================================
- Coverage 98.26% 98.22% -0.05%
==========================================
Files 195 195
Lines 9533 9527 -6
==========================================
- Hits 9368 9358 -10
- Misses 131 134 +3
- Partials 34 35 +1
Continue to review full report at Codecov.
|
Thanks for the research & the fix, @32leaves . I would like to have a unit test that uses grpc client to hit grpc endpoint via cmux, so that we don't have regressions like these in the future. |
I don't think this fix is helping, I added a unit test #1758 for connecting to grpc server via cmux, and it's failing both before and after the change to matchers. Also, the behavioral change in grpc/grpc-go#2406 was only released in grpc-go 1.23, but we're currently pinning grpc to 1.21.1 |
cc @annanay25 |
breaking change: grpc/grpc-go#2406 workaround described in: - soheilhy/cmux#64 - https://github.com/soheilhy/cmux#limitations Signed-off-by: Christian Weichel <[email protected]>
I just pushed the test I used while working on the fix (didn't see you had already added a test as well - I'm happy to remove mine again). With this fix I can successfully call |
My mistake, my PR was using the wrong port. It's fixed now, I prefer to merge my PR because it includes a bunch of other clean-up. I pulled the main commit from here for the actual fix. |
superseded by #1758 |
fixes #1754
breaking change: grpc/grpc-go#2406
workaround described in: