-
Notifications
You must be signed in to change notification settings - Fork 9
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
Expose the ability to set HTTP and TCP keepalives on dynamic backends. #140
Conversation
Co-authored-by: Ulyssa <git@ulyssa.dev>
Co-authored-by: Ulyssa <git@ulyssa.dev>
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.
Naming tweaks.
fsthttp/backend.go
Outdated
@@ -311,6 +311,52 @@ func (b *BackendOptions) PoolConnections(poolingOn bool) *BackendOptions { | |||
return b | |||
} | |||
|
|||
// HttpKeepaliveTime configures how long to allow HTTP connections to remain | |||
// idle in a connection pool before it should be considered closed. | |||
func (b *BackendOptions) HttpKeepaliveTime(time time.Duration) *BackendOptions { |
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.
Do we want HTTP
instead of Http
?
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.
https://pkg.go.dev/net/http#ParseHTTPVersion says yes...?
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.
Fixed in 107ec31!
fsthttp/backend.go
Outdated
|
||
// TcpKeepaliveEnable sets whether or not to use TCP keepalives to try to | ||
// maintain the connetion to the backend. | ||
func (b *BackendOptions) TcpKeepaliveEnable(enable bool) *BackendOptions { |
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.
Naming: TCP
vs Tcp
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.
random hit says yes: https://pkg.go.dev/github.com/m-lab/tcp-info/tcp uses TCP
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.
Fixed in 107ec31!
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.
One question about keepalives probe, but otherwise LGTM.
} | ||
|
||
func (b *BackendConfigOptions) TCPKeepaliveProbes(count uint32) { | ||
if count > 0 { |
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.
Count is a uint32
and so always >= 0
. Do we want to handle the 0
case too like with KeepaliveEnable
or use 0
to disable this option after previously setting it?
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.
I have a little bit of hesitation about using 0 to unset (I just don't like special values like that), but yes, that's a possibility. Basically, using 0 here would return this value to being the default for the system. If you wanted to turn probes off, you'd still need to unset KeepaliveEnable.
No description provided.