Deprecate maxRetryTimeout in RestClient and increase default value (#38425)

This commit deprecates the `maxRetryTimeout` settings in the low-level REST client, and increases its default value from 30 seconds to 90 seconds. The goal of this is to have it set higher than the socket timeout so that users get as few listener timeouts as possible.

Relates to #38085

Author: javanna

client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java

@@ -42,7 +42,7 @@
 public final class RestClientBuilder {
     public static final int DEFAULT_CONNECT_TIMEOUT_MILLIS = 1000;
     public static final int DEFAULT_SOCKET_TIMEOUT_MILLIS = 30000;
-    public static final int DEFAULT_MAX_RETRY_TIMEOUT_MILLIS = DEFAULT_SOCKET_TIMEOUT_MILLIS;
+    public static final int DEFAULT_MAX_RETRY_TIMEOUT_MILLIS = 90000;
     public static final int DEFAULT_MAX_CONN_PER_ROUTE = 10;
     public static final int DEFAULT_MAX_CONN_TOTAL = 30;
 
@@ -105,9 +105,10 @@ public RestClientBuilder setFailureListener(RestClient.FailureListener failureLi
     /**
      * Sets the maximum timeout (in milliseconds) to honour in case of multiple retries of the same request.
      * {@link #DEFAULT_MAX_RETRY_TIMEOUT_MILLIS} if not specified.
-     *
+     * @deprecated this setting is deprecated and will be removed in the future in favour of relying on socket and connect timeout
      * @throws IllegalArgumentException if {@code maxRetryTimeoutMillis} is not greater than 0
      */
+    @Deprecated
     public RestClientBuilder setMaxRetryTimeoutMillis(int maxRetryTimeoutMillis) {
         if (maxRetryTimeoutMillis <= 0) {
             throw new IllegalArgumentException("maxRetryTimeoutMillis must be greater than 0");

client/rest/src/test/java/org/elasticsearch/client/documentation/RestClientDocumentation.java

@@ -305,8 +305,7 @@ public RequestConfig.Builder customizeRequestConfig(
                                 .setConnectTimeout(5000)
                                 .setSocketTimeout(60000);
                         }
-                    })
-                .setMaxRetryTimeoutMillis(60000);
+                    });
             //end::rest-client-config-timeouts
         }
         {

docs/java-rest/low-level/configuration.asciidoc

@@ -18,8 +18,7 @@ https://hc.apache.org/httpcomponents-client-ga/httpclient/apidocs/org/apache/htt
  as an argument and has the same return type. The request config builder can
 be modified and then returned. In the following example we increase the
 connect timeout (defaults to 1 second) and the socket timeout (defaults to 30
-seconds). Also we adjust the max retry timeout accordingly (defaults to 30
-seconds too).
+seconds).
 
 ["source","java",subs="attributes,callouts,macros"]
 --------------------------------------------------

docs/java-rest/low-level/usage.asciidoc

@@ -185,9 +185,9 @@ prevent having to specify them with each single request
 include-tagged::{doc-tests}/RestClientDocumentation.java[rest-client-init-max-retry-timeout]
 --------------------------------------------------
 <1> Set the timeout that should be honoured in case multiple attempts are made
-for the same request. The default value is 30 seconds, same as the default
-socket timeout. In case the socket timeout is customized, the maximum retry
-timeout should be adjusted accordingly
+for the same request. The default value is 90 seconds. In case the socket
+timeout is set to a higher value, the max retry timeout should be adjusted
+accordingly. deprecated[6.7.0, max retry timeout will be removed in 7.0.0]
 
 ["source","java",subs="attributes,callouts,macros"]
 --------------------------------------------------

plugins/repository-azure/qa/microsoft-azure-storage/src/test/java/org/elasticsearch/repositories/azure/AzureStorageRepositoryClientYamlTestSuiteIT.java

@@ -41,7 +41,6 @@ public static Iterable<Object[]> parameters() throws Exception {
     protected Settings restClientSettings() {
         // Give more time to repository-azure to complete the snapshot operations
         return Settings.builder().put(super.restClientSettings())
-            .put(ESRestTestCase.CLIENT_RETRY_TIMEOUT, "60s")
             .put(ESRestTestCase.CLIENT_SOCKET_TIMEOUT, "60s")
             .build();
     }

qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/AbstractRollingTestCase.java

@@ -69,7 +69,6 @@ protected final Settings restClientSettings() {
             // increase the timeout here to 90 seconds to handle long waits for a green
             // cluster health. the waits for green need to be longer than a minute to
             // account for delayed shards
-            .put(ESRestTestCase.CLIENT_RETRY_TIMEOUT, "90s")
             .put(ESRestTestCase.CLIENT_SOCKET_TIMEOUT, "90s")
             .build();
     }

qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/UpgradeClusterClientYamlTestSuiteIT.java

@@ -65,7 +65,6 @@ protected Settings restClientSettings() {
             // increase the timeout here to 90 seconds to handle long waits for a green
             // cluster health. the waits for green need to be longer than a minute to
             // account for delayed shards
-            .put(ESRestTestCase.CLIENT_RETRY_TIMEOUT, "90s")
             .put(ESRestTestCase.CLIENT_SOCKET_TIMEOUT, "90s")
             .build();
     }

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

@@ -92,7 +92,6 @@
 public abstract class ESRestTestCase extends ESTestCase {
     public static final String TRUSTSTORE_PATH = "truststore.path";
     public static final String TRUSTSTORE_PASSWORD = "truststore.password";
-    public static final String CLIENT_RETRY_TIMEOUT = "client.retry.timeout";
     public static final String CLIENT_SOCKET_TIMEOUT = "client.socket.timeout";
     public static final String CLIENT_PATH_PREFIX = "client.path.prefix";
 
@@ -729,11 +728,6 @@ protected static void configureClient(RestClientBuilder builder, Settings settin
             }
             builder.setDefaultHeaders(defaultHeaders);
         }
-        final String requestTimeoutString = settings.get(CLIENT_RETRY_TIMEOUT);
-        if (requestTimeoutString != null) {
-            final TimeValue maxRetryTimeout = TimeValue.parseTimeValue(requestTimeoutString, CLIENT_RETRY_TIMEOUT);
-            builder.setMaxRetryTimeoutMillis(Math.toIntExact(maxRetryTimeout.getMillis()));
-        }
         final String socketTimeoutString = settings.get(CLIENT_SOCKET_TIMEOUT);
         if (socketTimeoutString != null) {
             final TimeValue socketTimeout = TimeValue.parseTimeValue(socketTimeoutString, CLIENT_SOCKET_TIMEOUT);

x-pack/qa/full-cluster-restart/src/test/java/org/elasticsearch/xpack/restart/FullClusterRestartIT.java

@@ -63,7 +63,6 @@ protected Settings restClientSettings() {
                 // we increase the timeout here to 90 seconds to handle long waits for a green
                 // cluster health. the waits for green need to be longer than a minute to
                 // account for delayed shards
-                .put(ESRestTestCase.CLIENT_RETRY_TIMEOUT, "90s")
                 .put(ESRestTestCase.CLIENT_SOCKET_TIMEOUT, "90s")
                 .build();
     }

x-pack/qa/kerberos-tests/src/test/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosAuthenticationIT.java

@@ -23,6 +23,7 @@
 import org.elasticsearch.test.rest.ESRestTestCase;
 import org.junit.Before;
 
+import javax.security.auth.login.LoginContext;
 import java.io.IOException;
 import java.net.InetAddress;
 import java.net.UnknownHostException;
@@ -33,8 +34,6 @@
 import java.util.List;
 import java.util.Map;
 
-import javax.security.auth.login.LoginContext;
-
 import static org.elasticsearch.common.xcontent.XContentHelper.convertToMap;
 import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue;
 import static org.hamcrest.Matchers.contains;
@@ -148,13 +147,7 @@ private RestClient buildRestClientForKerberos(final SpnegoHttpClientConfigCallba
         return restClientBuilder.build();
     }
 
-    private static void configureRestClientBuilder(final RestClientBuilder restClientBuilder, final Settings settings)
-            throws IOException {
-        final String requestTimeoutString = settings.get(CLIENT_RETRY_TIMEOUT);
-        if (requestTimeoutString != null) {
-            final TimeValue maxRetryTimeout = TimeValue.parseTimeValue(requestTimeoutString, CLIENT_RETRY_TIMEOUT);
-            restClientBuilder.setMaxRetryTimeoutMillis(Math.toIntExact(maxRetryTimeout.getMillis()));
-        }
+    private static void configureRestClientBuilder(final RestClientBuilder restClientBuilder, final Settings settings) {
         final String socketTimeoutString = settings.get(CLIENT_SOCKET_TIMEOUT);
         if (socketTimeoutString != null) {
             final TimeValue socketTimeout = TimeValue.parseTimeValue(socketTimeoutString, CLIENT_SOCKET_TIMEOUT);

x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/UpgradeClusterClientYamlTestSuiteIT.java

@@ -103,7 +103,6 @@ protected Settings restClientSettings() {
                 // we increase the timeout here to 90 seconds to handle long waits for a green
                 // cluster health. the waits for green need to be longer than a minute to
                 // account for delayed shards
-                .put(ESRestTestCase.CLIENT_RETRY_TIMEOUT, "90s")
                 .put(ESRestTestCase.CLIENT_SOCKET_TIMEOUT, "90s")
                 .build();
     }