-
Notifications
You must be signed in to change notification settings - Fork 4.6k
grpc: Deprecate InitialWindowSize and introduce InitialStreamWindowSize #8665
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -210,10 +210,13 @@ func WithReadBufferSize(s int) DialOption { | |||||||||||||||||
| // WithInitialWindowSize returns a DialOption which sets the value for initial | ||||||||||||||||||
| // window size on a stream. The lower bound for window size is 64K and any value | ||||||||||||||||||
| // smaller than that will be ignored. | ||||||||||||||||||
| // | ||||||||||||||||||
| // Deprecated: use WithInitialStreamWindowSize to set a stream window size without disabling | ||||||||||||||||||
| // dynamic flow control. | ||||||||||||||||||
| // Will be supported throughout 1.x. | ||||||||||||||||||
| func WithInitialWindowSize(s int32) DialOption { | ||||||||||||||||||
| return newFuncDialOption(func(o *dialOptions) { | ||||||||||||||||||
| o.copts.InitialWindowSize = s | ||||||||||||||||||
| o.copts.StaticWindowSize = true | ||||||||||||||||||
| }) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -223,10 +226,17 @@ func WithInitialWindowSize(s int32) DialOption { | |||||||||||||||||
| func WithInitialConnWindowSize(s int32) DialOption { | ||||||||||||||||||
| return newFuncDialOption(func(o *dialOptions) { | ||||||||||||||||||
| o.copts.InitialConnWindowSize = s | ||||||||||||||||||
| o.copts.StaticWindowSize = true | ||||||||||||||||||
| }) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // 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. | ||||||||||||||||||
|
Comment on lines
+232
to
+235
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
| func WithInitialStreamWindowSize(s int32) DialOption { | ||||||||||||||||||
| return WithInitialWindowSize(s) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // WithStaticStreamWindowSize returns a DialOption which sets the initial | ||||||||||||||||||
| // stream window size to the value provided and disables dynamic flow control. | ||||||||||||||||||
| func WithStaticStreamWindowSize(s int32) DialOption { | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -278,10 +278,13 @@ func ReadBufferSize(s int) ServerOption { | |||||||||||||
|
|
||||||||||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please ensure comments are restricted to 80 cols. |
||||||||||||||
| // dynamic flow control. | ||||||||||||||
| // Will be supported throughout 1.x. | ||||||||||||||
| func InitialWindowSize(s int32) ServerOption { | ||||||||||||||
| return newFuncServerOption(func(o *serverOptions) { | ||||||||||||||
| o.initialWindowSize = s | ||||||||||||||
| o.staticWindowSize = true | ||||||||||||||
| }) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -290,10 +293,16 @@ func InitialWindowSize(s int32) ServerOption { | |||||||||||||
| func InitialConnWindowSize(s int32) ServerOption { | ||||||||||||||
| return newFuncServerOption(func(o *serverOptions) { | ||||||||||||||
| o.initialConnWindowSize = s | ||||||||||||||
| o.staticWindowSize = true | ||||||||||||||
| }) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // 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. | ||||||||||||||
|
Comment on lines
+299
to
+301
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
| func InitialStreamWindowSize(s int32) ServerOption { | ||||||||||||||
| return InitialWindowSize(s) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // StaticStreamWindowSize returns a ServerOption to set the initial stream | ||||||||||||||
| // window size to the value provided and disables dynamic flow control. | ||||||||||||||
| // The lower bound for window size is 64K and any value smaller than that | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -490,6 +490,7 @@ type test struct { | |
| unaryServerInt grpc.UnaryServerInterceptor | ||
| streamServerInt grpc.StreamServerInterceptor | ||
| serverInitialWindowSize int32 | ||
| serverStaticWindowSize bool | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The variable name seems misleading. |
||
| serverInitialConnWindowSize int32 | ||
| customServerOptions []grpc.ServerOption | ||
|
|
||
|
|
@@ -509,6 +510,7 @@ type test struct { | |
| unaryClientInt grpc.UnaryClientInterceptor | ||
| streamClientInt grpc.StreamClientInterceptor | ||
| clientInitialWindowSize int32 | ||
| clientStaticWindowSize bool | ||
| clientInitialConnWindowSize int32 | ||
| perRPCCreds credentials.PerRPCCredentials | ||
| customDialOptions []grpc.DialOption | ||
|
|
@@ -605,10 +607,15 @@ func (te *test) listenAndServe(ts testgrpc.TestServiceServer, listen func(networ | |
| if te.unknownHandler != nil { | ||
| sopts = append(sopts, grpc.UnknownServiceHandler(te.unknownHandler)) | ||
| } | ||
| if te.serverInitialWindowSize > 0 { | ||
| sopts = append(sopts, grpc.InitialWindowSize(te.serverInitialWindowSize)) | ||
| if te.serverStaticWindowSize && te.serverInitialWindowSize > 0 { | ||
| sopts = append(sopts, grpc.StaticStreamWindowSize(te.serverInitialWindowSize)) | ||
| } else if te.serverInitialWindowSize > 0 { | ||
| sopts = append(sopts, grpc.InitialStreamWindowSize(te.serverInitialWindowSize)) | ||
| } | ||
| if te.serverInitialConnWindowSize > 0 { | ||
|
|
||
| if te.serverStaticWindowSize && te.serverInitialConnWindowSize > 0 { | ||
| sopts = append(sopts, grpc.StaticConnWindowSize(te.serverInitialConnWindowSize)) | ||
| } else if te.serverInitialConnWindowSize > 0 { | ||
| sopts = append(sopts, grpc.InitialConnWindowSize(te.serverInitialConnWindowSize)) | ||
| } | ||
| la := ":0" | ||
|
|
@@ -816,10 +823,14 @@ func (te *test) configDial(opts ...grpc.DialOption) ([]grpc.DialOption, string) | |
| if te.e.balancer != "" { | ||
| opts = append(opts, grpc.WithDefaultServiceConfig(fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, te.e.balancer))) | ||
| } | ||
| if te.clientInitialWindowSize > 0 { | ||
| opts = append(opts, grpc.WithInitialWindowSize(te.clientInitialWindowSize)) | ||
| if te.clientStaticWindowSize && te.clientInitialWindowSize > 0 { | ||
| opts = append(opts, grpc.WithStaticStreamWindowSize(te.clientInitialWindowSize)) | ||
| } else if te.clientInitialWindowSize > 0 { | ||
| opts = append(opts, grpc.WithInitialStreamWindowSize(te.clientInitialWindowSize)) | ||
| } | ||
| if te.clientInitialConnWindowSize > 0 { | ||
| if te.clientStaticWindowSize && te.clientInitialConnWindowSize > 0 { | ||
| opts = append(opts, grpc.WithStaticConnWindowSize(te.clientInitialConnWindowSize)) | ||
| } else if te.clientInitialConnWindowSize > 0 { | ||
| opts = append(opts, grpc.WithInitialConnWindowSize(te.clientInitialConnWindowSize)) | ||
| } | ||
| if te.perRPCCreds != nil { | ||
|
|
@@ -5374,18 +5385,25 @@ func (s) TestClientWriteFailsAfterServerClosesStream(t *testing.T) { | |
| } | ||
|
|
||
| type windowSizeConfig struct { | ||
| serverStream int32 | ||
| serverConn int32 | ||
| clientStream int32 | ||
| clientConn int32 | ||
| serverStaticWindowSize bool | ||
| clientStaticWindowSize bool | ||
|
Comment on lines
+5388
to
+5389
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar concern with the variable names here. |
||
| serverStream int32 | ||
| serverConn int32 | ||
| clientStream int32 | ||
| clientConn int32 | ||
| } | ||
|
|
||
| func (s) TestConfigurableWindowSizeWithLargeWindow(t *testing.T) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we could also add tests to verify the new behavior i.e. not disable BDP estimation? |
||
| // Before behavior change in PR #8665, large window sizes set | ||
| // using WithInitialWindowSize disabled BDP by default. Post the | ||
| // behavior change, we have to explicitly disable BDP. | ||
| wc := windowSizeConfig{ | ||
| serverStream: 8 * 1024 * 1024, | ||
| serverConn: 12 * 1024 * 1024, | ||
| clientStream: 6 * 1024 * 1024, | ||
| clientConn: 8 * 1024 * 1024, | ||
| serverStaticWindowSize: true, | ||
| clientStaticWindowSize: true, | ||
| serverStream: 8 * 1024 * 1024, | ||
| serverConn: 12 * 1024 * 1024, | ||
| clientStream: 6 * 1024 * 1024, | ||
| clientConn: 8 * 1024 * 1024, | ||
| } | ||
| for _, e := range listTestEnv() { | ||
| testConfigurableWindowSize(t, e, wc) | ||
|
|
@@ -5406,6 +5424,8 @@ func (s) TestConfigurableWindowSizeWithSmallWindow(t *testing.T) { | |
|
|
||
| func testConfigurableWindowSize(t *testing.T, e env, wc windowSizeConfig) { | ||
| te := newTest(t, e) | ||
| te.serverStaticWindowSize = wc.serverStaticWindowSize | ||
| te.clientStaticWindowSize = wc.clientStaticWindowSize | ||
| te.serverInitialWindowSize = wc.serverStream | ||
| te.serverInitialConnWindowSize = wc.serverConn | ||
| te.clientInitialWindowSize = wc.clientStream | ||
|
|
||
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.
Can you please make sure the comments are restricted to 80 cols?