grpc: change existing dial/server options that set initial window size to not disable BDP estimation#9078
grpc: change existing dial/server options that set initial window size to not disable BDP estimation#9078easwars wants to merge 3 commits into
Conversation
…e to not disable BDP estimation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9078 +/- ##
==========================================
+ Coverage 80.52% 80.69% +0.16%
==========================================
Files 413 413
Lines 33543 33550 +7
==========================================
+ Hits 27012 27072 +60
+ Misses 4316 4284 -32
+ Partials 2215 2194 -21
🚀 New features to boost your workflow:
|
dfawley
left a comment
dfawley
left a comment
There was a problem hiding this comment.
Not a super thorough review; just high level - @eshitachandwani should still look more closely at the implementation.
| // stream. The lower bound for window size is 64K and any value smaller than | ||
| // that will be ignored. This does not disable dynamic flow control. | ||
| func InitialStreamWindowSize(s int32) ServerOption { | ||
| return InitialWindowSize(s) |
There was a problem hiding this comment.
This is a super nit but I would make the other one call this, since that one is deprecated, and you could even just document the other one as being the same as InitialStreamWindowSize (or "calls" this one or whatever). And similar for the dial option.
| // For the static case, we use a shorter deadline to verify no | ||
| // growth happens. | ||
| if !tc.wantGrowth { | ||
| ctx, cancel = context.WithTimeout(ctx, time.Second) |
There was a problem hiding this comment.
How long does it take to see growth when it is expected? I would assume much less than 1s, but just curious.
| // Set message size to exhaust largest of window sizes. | ||
| messageSize := max(max(wc.serverStream, wc.serverConn), max(wc.clientStream, wc.clientConn)) / int32(numOfIter-1) | ||
| messageSize = max(messageSize, 64*1024) | ||
| t.Logf("easwars: messageSize=%d", messageSize) |
There was a problem hiding this comment.
Looks like some debugging logs got pushed by mistake. Can you please remove these?
| // Find the client socket. We need to wait for the subchannel and | ||
| // socket to be created and registered with channelz. | ||
| var sockets []*channelz.Socket | ||
| for { |
There was a problem hiding this comment.
Nit: can we use verifyResultWithDelay instead of writing the waiting logic ourself?
| @@ -0,0 +1,359 @@ | |||
| /* | |||
| * | |||
| * Copyright 2024 gRPC authors. | |||
There was a problem hiding this comment.
| * Copyright 2024 gRPC authors. | |
| * Copyright 2026 gRPC authors. |
3 participants
Fixes #7923
Specifically, this PR handles part two of the proposal: #7923 (comment)
It also fixes existing tests that relied on BDP estimation being turned OFF to use the
WithStaticXxxandStaticXxxoptions, and adds new tests that verify the behavior of all options in a e2e fashion.RELEASE NOTES:
WithInitialWindowSizeandInitialWindowSize. These are replaced byWithInitialStreamWindowSizeandInitialStreamWindowSizerespectively.