Skip to content
This repository was archived by the owner on Oct 11, 2024. It is now read-only.

Comments

Use cmux#1435

Merged
gdbelvin merged 6 commits intogoogle:masterfrom
gdbelvin:cmux2
Jan 20, 2020
Merged

Use cmux#1435
gdbelvin merged 6 commits intogoogle:masterfrom
gdbelvin:cmux2

Conversation

@gdbelvin
Copy link
Contributor

@gdbelvin gdbelvin commented Jan 17, 2020

Use cmux to differentiate between HTTP and GRPC connections.

cmux also (critically) supports serving gRPC over HTTP2 without TLS.

@gdbelvin gdbelvin requested a review from a team as a code owner January 17, 2020 12:22
@gdbelvin gdbelvin requested a review from mhutchinson January 17, 2020 12:22
@dep dep bot added the dependent label Jan 17, 2020
@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1435      +/-   ##
==========================================
+ Coverage   66.36%   66.49%   +0.12%     
==========================================
  Files          54       54              
  Lines        4026     4026              
==========================================
+ Hits         2672     2677       +5     
+ Misses        961      958       -3     
+ Partials      393      391       -2
Impacted Files Coverage Δ
core/integration/client_tests.go 84.77% <0%> (-0.47%) ⬇️
core/sequencer/server.go 73.61% <0%> (+0.65%) ⬆️
impl/sql/directory/storage.go 69.17% <0%> (+1.5%) ⬆️
core/sequencer/trillian_client.go 62.85% <0%> (+4.28%) ⬆️

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 9b523b6...4f80304. Read the comment docs.

Copy link

@NatalieDoduc NatalieDoduc left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying bits of this PR - if you could add this offline context to the PR description, the collective memory would be most grateful :)

Not approving yet, because i'm unsure about the dependency chain implications.

gRPC now requires a handshake to occur grpc/grpc-go#2565
This does not play well with cmux by default, but there is a setting
that cmux offers to complete the handshake correctly: https://github.com/soheilhy/cmux#limitations
@gdbelvin gdbelvin merged commit ef79a0a into google:master Jan 20, 2020
@gdbelvin gdbelvin deleted the cmux2 branch January 20, 2020 14:00
gdbelvin added a commit to gdbelvin/keytransparency that referenced this pull request Jan 20, 2020
@gdbelvin gdbelvin mentioned this pull request Jan 20, 2020
gdbelvin added a commit that referenced this pull request Jan 20, 2020
gdbelvin added a commit to gdbelvin/keytransparency that referenced this pull request Jan 27, 2020
This reverts commit ef79a0a.

cmux cannot serve HTTP2 with Golang's http server.
gdbelvin added a commit to gdbelvin/keytransparency that referenced this pull request Jan 28, 2020
This reverts commit ef79a0a.

cmux cannot serve HTTP2 with Golang's http server.
gdbelvin added a commit that referenced this pull request Jan 28, 2020
* Revert "Use cmux (#1435)"

This reverts commit ef79a0a.

cmux cannot serve HTTP2 with Golang's http server.

* Prioritize h2 over http1.1

* nits
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants