-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Conversation
creamsoup
commented
Apr 24, 2020
•
edited
Loading
edited
- avoid ping spam after WINDOW_UPDATE, DATA frame
- allow to set initialWindowUpdate for Netty{Channel,Server}Builder which enables bdp
- remove InternalHandlerSettings, NettyHandlerSettings
There was a problem hiding this 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.
private static synchronized int getLatestWindow(AbstractNettyHandler handler) { | ||
Preconditions.checkNotNull(handler); | ||
return handler.decoder().flowController() | ||
.initialWindowSize(handler.connection().connectionStream()); | ||
} | ||
|
||
static ChannelFutureListener cleanUpTask() { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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();; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/;;/;/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops =p