Skip to content

netty: allow to use bandwidth delay product#6979

Merged
creamsoup merged 10 commits intogrpc:masterfrom
creamsoup:bdp2
May 1, 2020
Merged

netty: allow to use bandwidth delay product#6979
creamsoup merged 10 commits intogrpc:masterfrom
creamsoup:bdp2

Conversation

@creamsoup
Copy link
Copy Markdown
Contributor

@creamsoup creamsoup commented Apr 24, 2020

  • avoid ping spam after WINDOW_UPDATE, DATA frame
  • allow to set initialWindowUpdate for Netty{Channel,Server}Builder which enables bdp
  • remove InternalHandlerSettings, NettyHandlerSettings

@creamsoup creamsoup requested a review from ejona86 April 27, 2020 22:55
Copy link
Copy Markdown
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

I still need to look at this more, but one of my comments seems good to send now.

The PR description says, "force send WINDOW__UPDATE with bdp ping" but it appears the code is taking the "avoid sending a PING when it will cause a problem" approach.

Comment thread netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java
Comment thread netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java Outdated
.initialWindowSize(handler.connection().connectionStream());
}

static ChannelFutureListener cleanUpTask() {
Copy link
Copy Markdown
Member

@ejona86 ejona86 Apr 27, 2020

Choose a reason for hiding this comment

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

I'm fiercely annoyed by the storage of these handlers and the dance to clean them up. (Not your code, their existence.)

What do you think about having NettyFlowControlTest create a custom ProtocolNegotiator that grabs the GrpcHttp2ConnectionHandler and we pass that to getLatestWindow() (so we would cast to AbstractNettyHandler in this class)?

(I'm also fine at that point trashing this class and just having InternalHandlerSettings)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done. using ProtocolNegotiator made the conversion very easy.


@Override
public void close() {
delegate.close();;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/;;/;/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oops =p

@creamsoup creamsoup merged commit 83a3b25 into grpc:master May 1, 2020
@creamsoup creamsoup deleted the bdp2 branch May 1, 2020 22:39
dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jun 14, 2021
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.

2 participants