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

netty: allow to use bandwidth delay product #6979

Merged
merged 10 commits into from May 1, 2020
Merged

Conversation

creamsoup
Copy link
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 Apr 27, 2020
Copy link
Member

@ejona86 ejona86 left a comment

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.

netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java Outdated Show resolved Hide resolved
netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java Outdated Show resolved Hide resolved
private static synchronized int getLatestWindow(AbstractNettyHandler handler) {
Preconditions.checkNotNull(handler);
return handler.decoder().flowController()
.initialWindowSize(handler.connection().connectionStream());
}

static ChannelFutureListener cleanUpTask() {
Copy link
Member

@ejona86 ejona86 Apr 28, 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
Contributor Author

@creamsoup creamsoup Apr 28, 2020

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.

ejona86
ejona86 approved these changes May 1, 2020

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

@ejona86 ejona86 May 1, 2020

Choose a reason for hiding this comment

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

s/;;/;/

Copy link
Contributor Author

@creamsoup creamsoup May 1, 2020

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
13 checks passed
@creamsoup creamsoup deleted the bdp2 branch May 1, 2020
dfawley pushed a commit to dfawley/grpc-java that referenced this issue 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.

None yet

2 participants