[fix][io] Update Elasticsearch sink idle cnx timeout to 30s

[michaeljmarshall]

Motivation

The current Elasticsearch sink has a setting named connectionIdleTimeoutInMs. This setting closes expired and idle http connections opened by the Elasticsearch Rest Client. The current default is 5 milliseconds. With this value, I observed connection issues both for timeouts trying to connect to elasticsearch and errors like "connection reset by peer". When overriding the default to 5 seconds, all errors disappeared. I propose we change it to 30 seconds since even 5 seconds is a little low.

Modifications

• Change default value for connectionIdleTimeoutInMs from 5 millis to 30000 millis
• Remove unnecessary call to closeExpiredConnections. This call is unnecessary because the closeIdleConnections also closes expired connections. Based on looking at the source code, we iterate over connections for each method call, so relying on the closeIdleConnections lets us iterate over connections once instead of twice. Source: https://hc.apache.org/httpcomponents-asyncclient-4.1.x/current/httpasyncclient/apidocs/org/apache/http/nio/conn/NHttpClientConnectionManager.html#closeIdleConnections(long,%20java.util.concurrent.TimeUnit)

    /**
     * Closes idle connections in the pool.
     * <p>
     * Open connections in the pool that have not been used for the
     * timespan given by the argument will be closed.
     * Currently allocated connections are not subject to this method.
     * Times will be checked with milliseconds precision
     *
     * All expired connections will also be closed.
     *
     * @param idletime  the idle time of connections to be closed
     * @param tunit     the unit for the {@code idletime}
     *
     * @see #closeExpiredConnections()
     */
    void closeIdleConnections(long idletime, TimeUnit tunit);

Verifying this change

This is a trivial change.

Does this pull request potentially affect one of the following parts:

• The default values of configurations

This PR changes a default value for a sink. The current default is problematic.

Documentation

• doc-not-needed

We generate the docs for this sink, so we don't need to update any site docs. We should include this update on the release notes.

Matching PR in forked repository

PR in forked repository: michaeljmarshall#22

[lhotari]

/pulsarbot rerun-failure-checks

[aymkhalil]

I wonder if this config should be validated against bulkFlushIntervalInMs when bulk API is enabled - something like connectionIdleTimeoutInMs > 2 * bulkFlushIntervalInMs because it seems the connection will set idle by design in-between flushes

[michaeljmarshall]

I think it'd be fine to let the connection get closed in that scenario. My main goal here is to make sure we have working defaults.

[aymkhalil]

Yeah I understand both comments are probably outside scope of this PR: First step is to have working defaults, and later maybe make them foolproof...

[aymkhalil]

Q: Is it required at all to evict idle connections? I wonder what's wrong with long lived connection that has a life cycle coupled with that of the sink instance. If it is not required, we could drop the connectionIdleTimeoutInMs for good but I maybe missing something.

[michaeljmarshall]

Good question. The motivation of this PR assumes that closing these connections is necessary, however, I am not sure that it is. The fundamental risk is that something between the client and the server closes the connection. In my mind, the canonical example is a network load balancer with a 4 or 5 minute timeout.

Closing expired and idle connections is one solution to prevent such errors due to inactivity.

While troubleshooting the underlying behavior this PR aims to fix, I came across elastic/elasticsearch#65213, which indicates that an alternative solution is to enable socket keepalives and to decrease the net.ipv4.tcp_keepalive_time in order to make sure those keepalives are sent before any intermediate server closes the connection due to inactivity. Since that solution requires modifying OS settings, I think this solution might be easier to maintain, even though it'll be less efficient.

[michaeljmarshall]

After re-reading that elasticsearch issue, it could be reasonable to move in the direction of enabling tcp keep-alives. At the very least, I think we should merge this and fix the existing default values.

[lhotari]

Since that solution requires modifying OS settings

@michaeljmarshall On managed cloud k8s environments, the OS settings are already properly tuned. related comment: #14841 (comment)

[michaeljmarshall]

@lhotari - that was not the case in the AKS cluster that I was testing with as of yesterday. When I tried to override the settings using https://kubernetes.io/docs/tasks/administer-cluster/sysctl-cluster/#setting-sysctls-for-a-pod, I got an error because overriding net.ipv4.tcp_keepalive_time = 300 is considered "unsafe" by default.