Skip to content

Enable TCP/IP keepalive by default for broker, proxy and client - #14841

Closed
lhotari wants to merge 1 commit into
apache:masterfrom
lhotari:lh-use-tcp-keepalive
Closed

Enable TCP/IP keepalive by default for broker, proxy and client#14841
lhotari wants to merge 1 commit into
apache:masterfrom
lhotari:lh-use-tcp-keepalive

Conversation

@lhotari

@lhotari lhotari commented Mar 24, 2022

Copy link
Copy Markdown
Member

Motivation

The general reasons to use TCP/IP keepalive are explained in https://tldp.org/HOWTO/TCP-Keepalive-HOWTO/overview.html .

@dlg99 pointed out some time ago that Pulsar proxy doesn't use TCP/IP keepalive . It's a valid point and I think it's worth investigating whether there would be a benefit in using TCP/IP keepalive in Pulsar.

In Pulsar, there is an application level keepalive solution that should take care of closing idling connections. That's why TCP/IP keepalive shouldn't be needed.

I have created this PR to open the discussion whether we should also use TCP/IP level keepalive and when that would be useful.

Modifications

  • Enable TCP/IP keepalive on the server side for broker, proxy and proxy extensions.
  • Enable TCP/IP keepalive in the client.

@lhotari lhotari added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker area/proxy labels Mar 24, 2022
@github-actions

Copy link
Copy Markdown

@lhotari:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@addisonj

Copy link
Copy Markdown
Contributor

Thanks for starting this discussion Lari.

In the bookkeeper protocol, we do rely on TCP level keep-alive as opposed to application level keep-alive... and by all accounts, we should just switch to an application level keep-alive instead.

AFAICT, event if we were to enable TCP keep-alive, it likely is not going to have much of an impact because of the OS level defaults being what they are, which, imho, basically make it useless. See this link https://tldp.org/HOWTO/TCP-Keepalive-HOWTO/usingkeepalive.html for details, but essentially the default OS level settings are such that:

  • it only starts after a connection has already been active for 2 hours
  • it only pings every 75 seconds
  • it takes 9 failures to trigger a failed socket

In summary it can take upwards of 11 minutes to time out a socket but only for connections active after 2 hours.

So this, combined with the application level keep alive, I think makes it not that useful by default...

All that said I am not against making it optional. The cost of TCP keep-alive is near zero, but for "power users" it can provide a redundant system against zombie sockets, though I suppose it is possible for some new bugs to surface if we get socket hang ups in other code paths, where right now a dead socket is most likely to be closed by the heartbeat failing.

@merlimat

merlimat commented Mar 24, 2022

Copy link
Copy Markdown
Contributor

In Pulsar, there is an application level keepalive solution that should take care of closing idling connections. That's why TCP/IP keepalive shouldn't be needed.

Yes, we introduced the application level keep-alive because of the limitations of TCP keep alive:

  • Very long minimum time to detect broken connections
  • Configuration of parameter is done in the OS (eg: it's not possible to override from client library) and it becomes an operational burden to set the environment when deploy Pulsar and applications that use Pulsar.

I think the current solution should cover all the aspects already covered by TCP keep-alive.

pointed out some time ago that Pulsar proxy doesn't use TCP/IP keepalive . It's a valid point and I think it's worth investigating whether there would be a benefit in using TCP/IP keepalive in Pulsar.

The Pulsar proxy rely on the fact that application keep-alive are flowing already in the connection, in both directions from client and broker.

@lhotari

lhotari commented Mar 24, 2022

Copy link
Copy Markdown
Member Author

/pulsarbot rerun-failure-checks

@github-actions

Copy link
Copy Markdown

The pr had no activity for 30 days, mark with Stale label.

@lhotari

lhotari commented Apr 29, 2022

Copy link
Copy Markdown
Member Author

@addisonj @merlimat I found a gap in Pulsar's application level keepalive Ping/Pong. It didn't properly handle errors when writing the Ping message. Please review #15382.

@lhotari

lhotari commented Apr 29, 2022

Copy link
Copy Markdown
Member Author

In summary it can take upwards of 11 minutes to time out a socket but only for connections active after 2 hours.

So this, combined with the application level keep alive, I think makes it not that useful by default...

I think that these settings have been tuned in all cloud provider k8s nodes.

In GCP, these are the settings I see on one of the nodes in one of our test environments:

# sysctl -a |grep keepalive
net.ipv4.tcp_keepalive_intvl = 60
net.ipv4.tcp_keepalive_probes = 5
net.ipv4.tcp_keepalive_time = 300

@github-actions

Copy link
Copy Markdown

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions Bot added the Stale label May 30, 2022
@tisonkun

tisonkun commented Dec 6, 2022

Copy link
Copy Markdown
Member

@lhotari do you still work on this patch?

@tisonkun

tisonkun commented Dec 8, 2022

Copy link
Copy Markdown
Member

Closed according to #14841 (comment) and #15382. It seems we use an application-level keep-alive mechanism instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker area/proxy doc-label-missing Stale type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants