Add envoy SO_KEEPALIVE settings #71
Conversation
There was a problem hiding this comment.
Thanks for creating this PR @jackkleeman. I had one question for clarification. +1 for merging after resolving it.
| @@ -285,6 +285,8 @@ data: | |||
| lb_policy: CLUSTER_PROVIDED | |||
| # assume http2 in upstream | |||
| http2_protocol_options: {} | |||
| upstream_connection_options: | |||
| tcp_keepalive: {} | |||
There was a problem hiding this comment.
This will default to the OS settings for the TCP keepalive configuration, right?
There was a problem hiding this comment.
How long would the keep alive timeout be? I found that by default it only kicks in after 2 hours. Won't that mean that we'll run into the TCP retransmission timeout before (after ~ 15 minutes) and therefore, the keepalive timeout won't have any effect?
There was a problem hiding this comment.
yes, that is the default - but bear in mind that a tcp connection is fairly likely to be open for many hours before a failure occurs, so we would expect keepalives every 75s for most conns
There was a problem hiding this comment.
If I understand things correctly, then the keep alive will start after 2h of idleness. Hence, whenever there is a package arriving it will be reset. Therefore, I believe that the default settings won't help here and if we recommend enabling upstream tcp keepalive with envoy, then we should set concrete values here.
There was a problem hiding this comment.
Slightly related question: Francesco concluded on restatedev/restate#626 to use HTTP/2 keep alives instead TCP keep alives. He also mentioned that Envoy should be able to do the same. Should we rather recommend using this mechanism instead of TCP keep alive?
There was a problem hiding this comment.
yep, sounds good to me, ill revert this and implement http2
No description provided.