grpc: update dial/server buffer options to support a "disable" setting #2147
Conversation
clientconn.go
Outdated
| @@ -150,6 +150,8 @@ func WithWriteBufferSize(s int) DialOption { | |||
|
|
|||
| // WithReadBufferSize lets you set the size of read buffer, this determines how much data can be read at most | |||
| // for each read syscall. | |||
| // A negative value will disable read buffer for a connection so data framer can access the underlying | |||
There was a problem hiding this comment.
Zero seems like a pretty good "disable" value to me.. It seems the problem is that we don't have a "default{Dial,Server}Options", but we do for "defaultCallOptions", right? Could you look into adding some non-zero defaults that way if it's not too hard?
dfawley
changed the title
| @@ -150,6 +150,8 @@ func WithWriteBufferSize(s int) DialOption { | |||
|
|
|||
There was a problem hiding this comment.
Should we also do the same thing with the write buffers while we're at it?
MakMukhi
force-pushed
the
disable_read_buffer
branch
2 times, most recently
from
17be597
to
504bbd9
Compare
MakMukhi
force-pushed
the
disable_read_buffer
branch
from
504bbd9
to
e34e502
Compare
clientconn.go
Outdated
| func defaultDialOptions() dialOptions { | ||
| return dialOptions{ | ||
| copts: transport.ConnectOptions{ | ||
| WriteBufferSize: -1, |
There was a problem hiding this comment.
Is it possible to use defaultWriteBufSize instead (and move the const into the grpc package instead of transport)?
clientconn.go
Outdated
| @@ -142,6 +154,8 @@ func WithWaitForHandshake() DialOption { | |||
|
|
|||
| // WithWriteBufferSize lets you set the size of write buffer, this determines how much data can be batched | |||
| // before doing a write on the wire. | |||
| // Zero will disable the write buffer such that each write will be on underlying connection. | |||
| // Note: A Send call may not directly translate to a write. | |||
There was a problem hiding this comment.
Please document the default value here (and also for read).
There was a problem hiding this comment.
Small note here, in fact gRPC allocates buffer of WriteBufferSize * 2 size, is it expected and maybe we should update documentation?
MakMukhi
force-pushed
the
disable_read_buffer
branch
from
26b3aed
to
74f3341
Compare
menghanl
changed the title
fixes #2013