added support for keep alive timeouts #513
Conversation
khchau7
changed the title
lib/kubeclient/watch_stream.rb
Outdated
| @@ -11,6 +11,7 @@ def initialize(uri, http_options, formatter:) | |||
| @http_client = nil | |||
| @http_options = http_options | |||
| @http_options[:http_max_redirects] ||= Kubeclient::Client::DEFAULT_HTTP_MAX_REDIRECTS | |||
| @http_options[:keep_alive_timeout] = 60 | |||
There was a problem hiding this comment.
this should be like this, this way you can set the value if its being set otherwise it will be default.
@http_options[:keep_alive_timeout] || = Kubeclient::Client::DEFAULT_KEEP_ALIVE_TIMEOUT
There was a problem hiding this comment.
keep_alive_timeout only set if its passed in otherwise this shouldnt set to keep no changes to existing behavior.
| @@ -53,6 +53,7 @@ class Client | |||
|
|
|||
| DEFAULT_HTTP_PROXY_URI = nil | |||
| DEFAULT_HTTP_MAX_REDIRECTS = 10 | |||
| DEFAULT_KEEP_ALIVE_TIMEOUT = 60 | |||
There was a problem hiding this comment.
The default of http gem seems to be 5 seconds: https://github.com/httprb/http/blob/v4.4.1/lib/http/options.rb#L63
I open to setting it higher by default if that's good for k8s watch connections.
This effectively controls how long we keep the watch open when server has no updates, right?
Do you know what's the default in Go or other k8s clients?
OTOH if we're not sure what's best value, we can set it 5 here to keep existing behavior, this PR will let users experiment...
|
Is this really controlling what #512 describes? Generally, I'm not against exposing more options 👍. We can merge this first and let people experiment with optimal settings. If you believe this is correct approach, please add a mention of the new option in README and add a CHANGELOG entry (in "Unreleased" section, which is currently horribly incomplete). |
| @@ -287,6 +287,7 @@ def define_entity_methods | |||
| # This method used to take resource_version as a param, so | |||
| # this conversion is to keep backwards compatibility | |||
| options = { resource_version: options } unless options.is_a?(Hash) | |||
| options[timeoutSeconds] = 380 # 6 mins 20 seconds | |||
There was a problem hiding this comment.
Did you mean :timeoutSeconds ?
This will override any passed-in timeout. I'm assuming you're just experimenting for now, but this can't be merged.
See #512: Fix in response to watch pods timing out and not recognizing any new notices after about 30 minutes of no new watch events.