grpc: deprecate InitialWindowSize and introduce InitialStreamWindowSize #8789
Conversation
eshitachandwani
added
Type: Bug
Type: Feature
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8789 +/- ##
==========================================
- Coverage 83.42% 83.29% -0.13%
==========================================
Files 418 418
Lines 32897 32897
==========================================
- Hits 27443 27403 -40
- Misses 4069 4086 +17
- Partials 1385 1408 +23
🚀 New features to boost your workflow:
|
Done. |
| if te.clientStaticWindow { | ||
| opts = append(opts, grpc.WithStaticStreamWindowSize(te.clientInitialWindowSize)) | ||
| } else { | ||
| opts = append(opts, grpc.WithInitialWindowSize(te.clientInitialWindowSize)) |
There was a problem hiding this comment.
We should add tests with both WithInitialStreamWindowSize and WithInitialWindowSize options to ensure both code paths are verified if they diverge in future.
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
Please make the release notes imperative sentences, e.g.: Update WithInitialWindowSize and WithInitialConnWindowSize to enable BDP unconditionally..
Please combine similar changes to a single release note. For example, all the changes to the existing options can be a single note and the introduction of the new options can be a separate note.
test/end2end_test.go
Outdated
| unaryServerInt grpc.UnaryServerInterceptor | ||
| streamServerInt grpc.StreamServerInterceptor | ||
| serverInitialWindowSize int32 | ||
| serverStaticWindow bool |
There was a problem hiding this comment.
Can this variable be given a more boolean like name like isServerWindowStatic or useStaticServerWindow?
There was a problem hiding this comment.
isServerWindowStatic reads more like natural English and answers the question "Is the server window static?". isServerStaticWindow feels less natural. Please change.
| clientInitialWindowSize int32 | ||
| clientUseInitialStreamWindowSize bool | ||
| clientInitialConnWindowSize int32 | ||
| clientStaticWindow bool |
There was a problem hiding this comment.
Similar to the server side boolean, please change re-name to isClientWindowStatic
| serverStaticWindow bool | ||
| clientStaticWindow bool |
There was a problem hiding this comment.
Please rename the variables to be similar to isClientWindowStatic and isServerWindowStatic.
test/end2end_test.go
Outdated
| unaryServerInt grpc.UnaryServerInterceptor | ||
| streamServerInt grpc.StreamServerInterceptor | ||
| serverInitialWindowSize int32 | ||
| serverStaticWindow bool |
There was a problem hiding this comment.
isServerWindowStatic reads more like natural English and answers the question "Is the server window static?". isServerStaticWindow feels less natural. Please change.
| clientUseInitialStreamWindowSize: true, | ||
| } | ||
| for _, e := range listTestEnv() { | ||
| testConfigurableWindowSize(t, e, wc) |
There was a problem hiding this comment.
Does this test explicitly assert the static/dynamic behavior of the client/server windows? I would expect the test to break if the window type changes unexpectedly. Is that currently covered?
4 participants
Fixes: #7923
This PR
WithInitialWindowSizeandWithInitialConnWindowSizedialOptions to enable BDP unconditionally.InitialWindowSizeandInitialConnWindowSizeserver options to enable BDP unconditionally.WithInitialWindowSizedialOption as deprecated and introduce a new dialOptionWithInitialStreamWindowSizewhich just returns WithInitialWindowSize`InitialWindowSizeserver option as deprecated and introduce a new server optionInitialStreamWindowSizewhich just returnsInitialWindowSizeRELEASE NOTES:
WithInitialWindowSizeandWithInitialConnWindowSizedialOptions andInitialWindowSizeandInitialConnWindowSizeserver options to enable BDP unconditionally.WithInitialStreamWindowSizeand a new server optionInitialStreamWindowSizeto enable BDP unconditionally and deprecateWithInitialWindowSizeandInitialWindowSize