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

client: set grpc-accept-encoding to full list of registered compressors #5541

Merged
merged 1 commit into from Oct 11, 2022

Conversation

jronak
Copy link
Contributor

@jronak jronak commented Jul 27, 2022

Part of #2786 (rest fixes in follow-up PRs)

Description:
In the current implementation, we forward grpc-accept-encoding only when the client sets compression on the outgoing RPCs. With this pull request, all the compressors registered with encoding.RegisterCompressor are added under the grpc-accept-encoding header for the outgoing RPCs.

When the client enables compression using deprecated grpc.WithCompressor, stick to the old behaviour of setting the grpc-accept-header header to the outgoing compressor.

RELEASE NOTES:

  • client: set grpc-accept-encoding header with all registered compressors

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 27, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • login: jronak / name: Ronak Jain (8018099)

@jronak jronak changed the title client: set grpc-accept-encoding header with all registered compressors client: set grpc-accept-encoding to full list of registered compressors Aug 2, 2022
@dfawley dfawley added this to the 1.50 Release milestone Aug 12, 2022
Copy link
Contributor

@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.

Thanks for the PR! Overall it looks pretty good. Comments inline, and one higher level comment:

I'm at least a little nervous about enabling this by default. It may be better to have this controlled by a dial option that either lists the codecs the user wants to expose or enables/disables this functionality. I'll get back to you about this part.

encoding/encoding.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Show resolved Hide resolved
internal/transport/http2_client.go Outdated Show resolved Hide resolved
@jronak jronak force-pushed the compress branch 3 times, most recently from 98a34eb to 36d6f4f Compare August 20, 2022 11:31
@dfawley dfawley self-assigned this Aug 23, 2022
Copy link
Contributor

@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.

Sorry for the long delay here. I discussed this with the other language leads and we think a change like this should be good. If it isn't too much trouble, would you mind putting this behavior behind an environment variable (turned on by default), so that, in case users run into trouble in the next couple releases, they can disable it? See: https://github.com/grpc/grpc-go/blob/master/internal/envconfig/envconfig.go

Thank you!

internal/grpcutil/compressor.go Outdated Show resolved Hide resolved
internal/grpcutil/compressor.go Outdated Show resolved Hide resolved
internal/transport/http2_client.go Show resolved Hide resolved
@dfawley dfawley assigned jronak and unassigned dfawley Sep 21, 2022
@jronak jronak force-pushed the compress branch 4 times, most recently from 60f2cb7 to 511c171 Compare October 3, 2022 16:21
@jronak jronak requested a review from dfawley October 3, 2022 16:23
@dfawley dfawley assigned dfawley and unassigned jronak Oct 4, 2022
@dfawley dfawley modified the milestones: 1.50 Release, 1.51 Release Oct 4, 2022
internal/envconfig/envconfig.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned easwars and unassigned dfawley Oct 10, 2022
@dfawley
Copy link
Contributor

dfawley commented Oct 10, 2022

LGTM. Could you please take a pass on this, too, @easwars ?

@easwars easwars merged commit 7b817b4 into grpc:master Oct 11, 2022
11 checks passed
@jronak jronak deleted the compress branch October 13, 2022 17:01
lfengru added a commit to lfengru/grpc-go that referenced this pull request Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants