-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add the ability to turn off huffman for given http2 protocol option. #42263
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
Conversation
Signed-off-by: Kevin Baichoo <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
Assigning @RyanTheOptimist as a reviewer (legacy from #38270). Feel free to reassign if needed. |
|
Dependency change is the addition of a patch. Requires API review too /lgtm deps |
|
/wait |
Signed-off-by: Kevin Baichoo <[email protected]>
RyanTheOptimist
left a comment
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.
Looks great!
|
PTAL @markdroth |
| // Disables encoding the headers using huffman encoding. | ||
| // This can be useful in cases where the cpu spent encoding the headers isn't | ||
| // worth the network bandwidth saved e.g. for localhost. | ||
| bool disable_huffman_encoding = 18; |
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.
How about encoding this as google.protobuf.BoolValue enable_huffman_encoding, so that the data plane's default isn't necessarily baked into the API? It also avoids a double negative -- i.e., instead of if (!disable_huffman_encoding), we can say if (enable_huffman_encoding).
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.
good idea, done.
|
ping @KBaichoo it looks like this PR is waiting for you to address one comment |
|
yep will take a swing at this today -- have been busy with oncall 📟 |
Signed-off-by: Kevin Baichoo <[email protected]>
|
/lgtm api |
Signed-off-by: Kevin Baichoo <[email protected]>
|
/lgtm deps |
|
@markdroth could you stamp again? |
|
/lgtm api |
Commit Message: Add the ability to selectively remove huffman encoding headers as the sender for a given hop.
Additional Description: This is just #38270 without the wonked git history from being an old PR.
Risk Level: low (off by default)
Testing: Unit test, Running in Prod
Docs Changes: n/a
Release Notes: included
Platform Specific Features: n/a
Fixes: #38025