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

server: expose API to set send compressor #5744

Merged
merged 13 commits into from Jan 31, 2023

Conversation

jronak
Copy link
Contributor

@jronak jronak commented Oct 26, 2022

Fixes #5792

Follow-up of #5541 and #2786

Description:
In the current implementation, the server compresses the send payload when the client sends a compressed request payload or explicitly uses the grpc.RPCCompressor option. It cannot be controlled by the user to either use a different compression algorithm or disable compression (identity).

This PR exposes an API grpc.SetSendCompressor(ctx, name) similar to the grpc.SetHeader(ctx, md), which allows the user to set the compressor to be used for compressing response payload (sent back to the client). The provided compressor is used when the below conditions are met:

  • the compressor name provided must be registered via encoding.RegisterCompressor in the server.
  • compressor name exists in the client advertised compressor names sent in grpc-accept-encoding metadata.

Other gRPC platforms expose similar APIs, for example grpc-java.

RELEASE NOTES:

  • server: expose SetSendCompressor API to set send compressor name

@easwars easwars added this to the 1.51 Release milestone Nov 1, 2022
@easwars easwars self-assigned this Nov 1, 2022
@zasweq zasweq modified the milestones: 1.51 Release, 1.52 Release Nov 8, 2022
Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

Sincere apologies for the delay in getting to this PR.

server.go Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
internal/transport/transport.go Outdated Show resolved Hide resolved
internal/transport/transport.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
stream.go Show resolved Hide resolved
@jronak
Copy link
Contributor Author

jronak commented Nov 21, 2022

@easwars thanks much for the review, I have addressed the comments. Please review when you get a chance

internal/grpcutil/compressor.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
internal/transport/http2_server.go Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Dec 7, 2022

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

internal/transport/http2_server.go Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
test/end2end_test.go Show resolved Hide resolved
test/end2end_test.go Show resolved Hide resolved
internal/transport/http2_server.go Outdated Show resolved Hide resolved
@jronak jronak requested a review from dfawley January 23, 2023 15:30
@arvindbr8 arvindbr8 assigned dfawley and unassigned jronak Jan 24, 2023
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM modulo the one testing nit. Thanks!

test/end2end_test.go Outdated Show resolved Hide resolved
@dfawley
Copy link
Member

dfawley commented Jan 25, 2023

@easwars please take another pass on this after the latest set(s) of changes.

@easwars easwars removed their assignment Jan 25, 2023
@arvindbr8 arvindbr8 modified the milestones: 1.53 Release, 1.54 Release Jan 26, 2023
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Please see previous comment re: adding a comment. (Updating state.)

@dfawley dfawley merged commit 0954097 into grpc:master Jan 31, 2023
1 check passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server Compression Asymmetry
5 participants