Skip to content

Add ManagedChannelBuilder/ServerBuilder.maxInboundMetadataSize#4897

Merged
ejona86 merged 4 commits into
grpc:masterfrom
ejona86:maxinboundmessagesize
Oct 18, 2018
Merged

Add ManagedChannelBuilder/ServerBuilder.maxInboundMetadataSize#4897
ejona86 merged 4 commits into
grpc:masterfrom
ejona86:maxinboundmessagesize

Conversation

@ejona86

@ejona86 ejona86 commented Oct 2, 2018

Copy link
Copy Markdown
Member

Support was added to InProcess and OkHttp transports. As mentioned
in #4567 OkHttp will not actually limit the memory used.

This API was a minor rename to the pre-existing Netty API,
so has already undergone API review and thus is not ExperimentalApi.

In adding transport tests I discovered Netty's behavior for trailers is
broken causing the stream to be orphaned and the client to hang. I have
verified this is fixed by netty/netty#8332 .

The commits will remain separate when merging. They may help during
review, but probably aren't too necessary.

Fixes #4050 and #4567.

CC @garrettjonesgoogle

@ejona86 ejona86 changed the title Add ManagedChannelBuilder/ServerBuilder.maxInboundMessageSize Add ManagedChannelBuilder/ServerBuilder.maxInboundMetadataSize Oct 2, 2018
@ejona86 ejona86 requested a review from zpencer October 2, 2018 17:09
Comment thread core/src/main/java/io/grpc/inprocess/InProcessTransport.java
* HTTP/2's SETTINGS_MAX_HEADER_LIST_SIZE</a>. It sums the bytes from each entry's key and value,
* plus 32 bytes of overhead per entry.
*/
public NettyChannelBuilder maxInboundMetadataSize(int maxInboundMetadataSize) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: was the parameter supposed to be named bytes for this set of functions? For InProcess it is currently named bytes.

@ejona86 ejona86 force-pushed the maxinboundmessagesize branch from bf6191b to 11fb10e Compare October 17, 2018 22:27
@ejona86

ejona86 commented Oct 17, 2018

Copy link
Copy Markdown
Member Author

@zpencer, done. I noticed many more problems. I'll need to rebase before merging.

@ejona86

ejona86 commented Oct 18, 2018

Copy link
Copy Markdown
Member Author

@zpencer PTAL

@ejona86 ejona86 force-pushed the maxinboundmessagesize branch from 11fb10e to 3645abc Compare October 18, 2018 15:17
@ejona86

ejona86 commented Oct 18, 2018

Copy link
Copy Markdown
Member Author

Rebased to get past conflicts. Waiting on CI and then will merge.

This is a rename of the pre-existing Netty builder method, so aliases
were added to the Netty builders.

Fixes grpc#4050. This API was a minor rename to the pre-existing Netty API,
so has already undergone API review and thus is not ExperimentalApi.
@ejona86 ejona86 force-pushed the maxinboundmessagesize branch from 3645abc to 53b0f3e Compare October 18, 2018 21:01
@ejona86 ejona86 merged commit b7c3d27 into grpc:master Oct 18, 2018
@ejona86 ejona86 deleted the maxinboundmessagesize branch October 18, 2018 21:34
@lock lock Bot locked as resolved and limited conversation to collaborators Jan 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generalize NettyChannelBuilder.maxHeaderListSize to ManagedChannelBuilder

2 participants