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.

Sign up for GitHub

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

Jump to bottom

netty:Auto adjust BDP ping frequency #9650

Merged

larry-safran merged 13 commits into grpc:master from larry-safran:bdp-ping-backoff

Dec 22, 2022

Contributor

larry-safran commented Oct 21, 2022 •

edited

Loading

Only initiate a BDP ping after every X bytes of data where X is a multiple of the window size that increases when the window isn't changing.

Added a ticker to the Netty handlers so that bandwidth could be properly controlled from tests.

Fixes #8260

larry-safran added 2 commits

October 21, 2022 14:49


          netty:Auto adjust BDP ping frequency

475435f


          cleanup

a8a2ec2

larry-safran requested a review from ejona86

October 21, 2022 22:03


          Simplify pingFrequencyMultiplier modification when window resizes fro…

24ea208

…m decrement 1 to simply 1.

Contributor Author

larry-safran commented Nov 8, 2022

@ejona86 please review

larry-safran added 4 commits

November 16, 2022 17:35


          Respond to reviewer comments:

0e3ece6

* Split for loop out into its own test
* Change setDataSizeAndSincePing to set time to current time rather than 1 second (or nanosecond) previous.


          Change test method to public.

870e465


          Change method comment about testing from javadoc to simple comment

792ed26


          Fix tests after refactor

50256ad

ejona86 reviewed

View reviewed changes

Member

ejona86 left a comment

Sending what I have, in whatever state it happens to be.

netty/src/main/java/io/grpc/netty/AbstractNettyHandler.java Show resolved Hide resolved

netty/src/main/java/io/grpc/netty/AbstractNettyHandler.java Outdated Show resolved Hide resolved

netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java Show resolved Hide resolved

netty/src/main/java/io/grpc/netty/AbstractNettyHandler.java Outdated Show resolved Hide resolved


          Respond to code review including marking the interface as Experimenta…

2e2b038

…lApi

Contributor Author

larry-safran commented Nov 30, 2022

ejona86 reviewed

View reviewed changes

netty/src/main/java/io/grpc/netty/AbstractNettyHandler.java Show resolved Hide resolved

netty/src/main/java/io/grpc/netty/NettyClientTransport.java Outdated Show resolved Hide resolved

netty/src/main/java/io/grpc/netty/AbstractNettyHandler.java Outdated Show resolved Hide resolved

netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java Outdated Show resolved Hide resolved

netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java Outdated Show resolved Hide resolved

netty/src/main/java/io/grpc/netty/AbstractNettyHandler.java Outdated Show resolved Hide resolved

netty/src/main/java/io/grpc/netty/AbstractNettyHandler.java Outdated Show resolved Hide resolved

netty/src/main/java/io/grpc/netty/NettyClientTransport.java Outdated Show resolved Hide resolved

netty/src/main/java/io/grpc/netty/NettyClientTransport.java Outdated Show resolved Hide resolved

netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java Outdated Show resolved Hide resolved

larry-safran added 2 commits

December 21, 2022 14:37


          Response to code review.

0b9366f


          Remove unused nested class.

eae8d42

ejona86 approved these changes

View reviewed changes

Member

ejona86 left a comment

I'm a bit concerned about whether the NettyClientStream.start() blows up in our face, but assuming it doesn't, consider the comments and LGTM.

netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java Outdated Show resolved Hide resolved

netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java Show resolved Hide resolved

netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java Outdated Show resolved Hide resolved

netty/src/main/java/io/grpc/netty/NettyServerHandler.java Show resolved Hide resolved

larry-safran added 3 commits

December 21, 2022 15:28


          refactor AbstractNettyHandler.onDataRead so update of lastTargetWindo…

1ac7c41

…w happens after check to start a ping.


          Use grpcDataFrame rather than simple dataFrame and so have initXkbBuf…

cdd89eb

…fer return a byte[].


          Use grpcDataFrame rather than simple dataFrame and so have initXkbBuf…

fa7208b

…fer return a byte[].

Remove extra factory NettyServerHandler.newHandler

larry-safran merged commit f5e8459 into grpc:master

larry-safran deleted the bdp-ping-backoff branch

December 22, 2022 02:00

larry-safran added a commit that referenced this pull request


          Revert "netty:Auto adjust BDP ping frequency (#9650)"

a732a18

This reverts commit f5e8459.

larry-safran mentioned this pull request

Revert "netty:Auto adjust BDP ping frequency" #9821

Merged

larry-safran added a commit that referenced this pull request


          Revert "netty:Auto adjust BDP ping frequency (#9650)" (#9821)

a2bbe84

This reverts commit f5e8459.

larry-safran added a commit that referenced this pull request


          Revert "Revert "netty:Auto adjust BDP ping frequency (#9650)" (#9821)"

6fd9983

This reverts commit a2bbe84.

larry-safran mentioned this pull request

Restore "netty:Auto adjust BDP ping frequency" with fix #9847

Merged

larry-safran added a commit that referenced this pull request


          Restore "netty:Auto adjust BDP ping frequency" with fix (#9847)

3cbd948

* Revert "Revert "netty:Auto adjust BDP ping frequency (#9650)" (#9821)"

This reverts commit a2bbe84.

* Eliminate half RTT delay in sending BDP Pings when starting up.
* Eliminate delay for bdp ping when current read would push us over the threshold.

github-actions bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet