grpc: Deprecate InitialWindowSize and introduce InitialStreamWindowSize #8665
Conversation
Part of grpc#7923. Since it has been two releases after grpc#8283 is merged, we can introduce InitialStreamWindowSize on both server and client, while deprecating InitialWindowSize. RELEASE NOTES: * grpc: Introduce new ServerOption and ClientOption to set stream window size without disabling BDP estimation. * grpc: Behavior change in WithInitialWindowSize in ClientOption and InitialWindowSize in ServerOption. Using these options will no longer disable BDP estimation. To explicitly disable BDP estimation, please use *StaticStreamWindowSize options. * grpc: Mark WithInitialWindowSize for deprecation, to be replaced with WithInitialStreamWindowSize in ClientOption. Similarly, for InitialWindowSize inServerOption to be replaced with InitialStreamWindowSize.
Pramodh-G
force-pushed
the
fix-window-options
branch
from
006cadf to
1a6265f
Compare
|
@Pramodh-G , the tests seem to be failing , could you please make sure that the tests are passing before we review it? |
eshitachandwani
added
the
Type: Behavior Change
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8665 +/- ##
==========================================
+ Coverage 81.97% 81.99% +0.02%
==========================================
Files 417 417
Lines 40788 40806 +18
==========================================
+ Hits 33435 33459 +24
+ Misses 5991 5957 -34
- Partials 1362 1390 +28
🚀 New features to boost your workflow:
|
|
@eshitachandwani / @dfawley: Please take a look. Tests are passing now. |
| // smaller than that will be ignored. | ||
| // | ||
| // Deprecated: use WithInitialStreamWindowSize to set a stream window size without disabling | ||
| // dynamic flow control. |
There was a problem hiding this comment.
Can you please make sure the comments are restricted to 80 cols?
| // WithInitialStreamWindowSize returns a DialOption which sets the value for | ||
| // a initial window size on a stream. The lower bound for window size is 64K | ||
| // and any value smaller than that will be ignored. Importantly, this does | ||
| // not disable dynamic flow control. |
There was a problem hiding this comment.
| // WithInitialStreamWindowSize returns a DialOption which sets the value for | |
| // a initial window size on a stream. The lower bound for window size is 64K | |
| // and any value smaller than that will be ignored. Importantly, this does | |
| // not disable dynamic flow control. | |
| // WithInitialStreamWindowSize returns a DialOption which sets the value for | |
| // a initial window size on a stream without disabling dynamic flow control. | |
| // The lower bound for window size is 64K and any value smaller than that | |
| // will be ignored. |
| // InitialWindowSize returns a ServerOption that sets window size for stream. | ||
| // The lower bound for window size is 64K and any value smaller than that will be ignored. | ||
| // | ||
| // Deprecated: use InitialStreamWindowSize to set a stream window size without disabling |
There was a problem hiding this comment.
Please ensure comments are restricted to 80 cols.
| // InitialStreamWindowSize returns a ServerOption that sets the window size for a stream. | ||
| // THe lower bound for a window size is 64K, and any value smaller than that will be ignored. | ||
| // Importantly, this does not disable dynamic flow control. |
There was a problem hiding this comment.
| // InitialStreamWindowSize returns a ServerOption that sets the window size for a stream. | |
| // THe lower bound for a window size is 64K, and any value smaller than that will be ignored. | |
| // Importantly, this does not disable dynamic flow control. | |
| // InitialStreamWindowSize returns a ServerOption that sets the window size for | |
| // a stream without disabling dynamic flow control. The lower bound for a window | |
| // size is 64K, and any value smaller than that will be ignored. |
| unaryServerInt grpc.UnaryServerInterceptor | ||
| streamServerInt grpc.StreamServerInterceptor | ||
| serverInitialWindowSize int32 | ||
| serverStaticWindowSize bool |
There was a problem hiding this comment.
The variable name seems misleading. serverStaticWindowSize indicates that it should take size for static window as input whereas it takes a boolean. We could either change the variable name to indicate it is boolean, maybe something like isServerWindowStatic or staticServerWindow or something better. OR we could keep the same name and change the logic to actually take the window size and make sure if it is set we use the grpc.StaticStreamWindowSize option. Personally , the second option seems better to me.
| serverStaticWindowSize bool | ||
| clientStaticWindowSize bool |
There was a problem hiding this comment.
Similar concern with the variable names here.
| clientConn int32 | ||
| } | ||
|
|
||
| func (s) TestConfigurableWindowSizeWithLargeWindow(t *testing.T) { |
There was a problem hiding this comment.
Maybe we could also add tests to verify the new behavior i.e. not disable BDP estimation?
3 participants
Part of #7923.
Since it has been two releases after #8283 is merged, we can introduce
InitialStreamWindowSizeon both server and client, while deprecatingInitialWindowSize.RELEASE NOTES:
grpc: Introduce new
ServerOptionandClientOptionto set stream window size without disabling BDP estimation.grpc: Behavior change in
WithInitialWindowSizeinClientOptionandInitialWindowSizeinServerOption. Using these options will no longer disable BDP estimation. To explicitly disable BDP estimation, please use `StaticStreamWindowSize options.grpc: Mark
WithInitialWindowSizefor deprecation, to be replaced withWithInitialStreamWindowSizeinClientOption. Similarly, forInitialWindowSizeinServerOption to be replaced withInitialStreamWindowSize.