Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/125552.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 125552
summary: Expose S3 connection max idle time as a setting
area: Snapshot/Restore
type: enhancement
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,13 @@ protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) {
if (region != null) {
builder.put(S3ClientSettings.REGION.getConcreteSettingForNamespace("test").getKey(), region);
}
if (randomBoolean()) {
// Sometimes explicitly set connection max idle time to ensure it is configurable
builder.put(
S3ClientSettings.CONNECTION_MAX_IDLE_TIME_SETTING.getConcreteSettingForNamespace("test").getKey(),
S3ClientSettings.Defaults.CONNECTION_MAX_IDLE_TIME
);
}
return builder.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,12 @@ final class S3ClientSettings {
key -> Setting.simpleString(key, Property.NodeScope)
);

static final Setting.AffixSetting<TimeValue> CONNECTION_MAX_IDLE_TIME_SETTING = Setting.affixKeySetting(
PREFIX,
"connection_max_idle_time",
key -> Setting.timeSetting(key, Defaults.CONNECTION_MAX_IDLE_TIME, Property.NodeScope)
);
Comment on lines +189 to +193
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to update the docs for this. But was not able to find the source file anymore (asciidoc or md). Not sure whether this is a bug introduced in #123507. I asked in the es-docs channel.


/** Credentials to authenticate with s3. */
final S3BasicCredentials credentials;

Expand Down Expand Up @@ -223,6 +229,11 @@ final class S3ClientSettings {
/** Signer override to use or empty string to use default. */
final String signerOverride;

/**
* The maximum idle time (in millis) of a connection before it is discarded from the connection pool.
*/
final long connectionMaxIdleTimeMillis;

private S3ClientSettings(
S3BasicCredentials credentials,
String endpoint,
Expand All @@ -239,7 +250,8 @@ private S3ClientSettings(
boolean pathStyleAccess,
boolean disableChunkedEncoding,
String region,
String signerOverride
String signerOverride,
long connectionMaxIdleTimeMillis
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd slightly prefer this to be grouped next to readTimeoutMillis (here, and in the field declaration, and in .equals() and .hashcode() etc).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, see 3f09a49

) {
this.credentials = credentials;
this.endpoint = endpoint;
Expand All @@ -257,6 +269,7 @@ private S3ClientSettings(
this.disableChunkedEncoding = disableChunkedEncoding;
this.region = region;
this.signerOverride = signerOverride;
this.connectionMaxIdleTimeMillis = connectionMaxIdleTimeMillis;
}

/**
Expand Down Expand Up @@ -297,6 +310,11 @@ S3ClientSettings refine(Settings repositorySettings) {
}
final String newRegion = getRepoSettingOrDefault(REGION, normalizedSettings, region);
final String newSignerOverride = getRepoSettingOrDefault(SIGNER_OVERRIDE, normalizedSettings, signerOverride);
final long newConnectionMaxIdleTimeMillis = getRepoSettingOrDefault(
CONNECTION_MAX_IDLE_TIME_SETTING,
normalizedSettings,
TimeValue.timeValueMillis(connectionMaxIdleTimeMillis)
).millis();
if (Objects.equals(endpoint, newEndpoint)
&& protocol == newProtocol
&& Objects.equals(proxyHost, newProxyHost)
Expand All @@ -310,7 +328,8 @@ S3ClientSettings refine(Settings repositorySettings) {
&& newPathStyleAccess == pathStyleAccess
&& newDisableChunkedEncoding == disableChunkedEncoding
&& Objects.equals(region, newRegion)
&& Objects.equals(signerOverride, newSignerOverride)) {
&& Objects.equals(signerOverride, newSignerOverride)
&& Objects.equals(connectionMaxIdleTimeMillis, newConnectionMaxIdleTimeMillis)) {
return this;
}
return new S3ClientSettings(
Expand All @@ -329,7 +348,8 @@ S3ClientSettings refine(Settings repositorySettings) {
newPathStyleAccess,
newDisableChunkedEncoding,
newRegion,
newSignerOverride
newSignerOverride,
newConnectionMaxIdleTimeMillis
);
}

Expand Down Expand Up @@ -438,7 +458,8 @@ static S3ClientSettings getClientSettings(final Settings settings, final String
getConfigValue(settings, clientName, USE_PATH_STYLE_ACCESS),
getConfigValue(settings, clientName, DISABLE_CHUNKED_ENCODING),
getConfigValue(settings, clientName, REGION),
getConfigValue(settings, clientName, SIGNER_OVERRIDE)
getConfigValue(settings, clientName, SIGNER_OVERRIDE),
getConfigValue(settings, clientName, CONNECTION_MAX_IDLE_TIME_SETTING).millis()
);
}
}
Expand Down Expand Up @@ -466,7 +487,8 @@ public boolean equals(final Object o) {
&& Objects.equals(proxyPassword, that.proxyPassword)
&& Objects.equals(disableChunkedEncoding, that.disableChunkedEncoding)
&& Objects.equals(region, that.region)
&& Objects.equals(signerOverride, that.signerOverride);
&& Objects.equals(signerOverride, that.signerOverride)
&& Objects.equals(connectionMaxIdleTimeMillis, that.connectionMaxIdleTimeMillis);
}

@Override
Expand All @@ -486,7 +508,8 @@ public int hashCode() {
throttleRetries,
disableChunkedEncoding,
region,
signerOverride
signerOverride,
connectionMaxIdleTimeMillis
);
}

Expand All @@ -507,5 +530,6 @@ static final class Defaults {
static final int MAX_CONNECTIONS = ClientConfiguration.DEFAULT_MAX_CONNECTIONS;
static final int RETRY_COUNT = ClientConfiguration.DEFAULT_RETRY_POLICY.getMaxErrorRetry();
static final boolean THROTTLE_RETRIES = ClientConfiguration.DEFAULT_THROTTLE_RETRIES;
static final TimeValue CONNECTION_MAX_IDLE_TIME = TimeValue.timeValueMillis(ClientConfiguration.DEFAULT_CONNECTION_MAX_IDLE_MILLIS);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ public List<Setting<?>> getSettings() {
S3ClientSettings.USE_THROTTLE_RETRIES_SETTING,
S3ClientSettings.USE_PATH_STYLE_ACCESS,
S3ClientSettings.SIGNER_OVERRIDE,
S3ClientSettings.CONNECTION_MAX_IDLE_TIME_SETTING,
S3ClientSettings.REGION,
S3Service.REPOSITORY_S3_CAS_TTL_SETTING,
S3Service.REPOSITORY_S3_CAS_ANTI_CONTENTION_DELAY_SETTING,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ static ClientConfiguration buildConfiguration(S3ClientSettings clientSettings, b
clientConfiguration.setMaxErrorRetry(clientSettings.maxRetries);
clientConfiguration.setUseThrottleRetries(clientSettings.throttleRetries);
clientConfiguration.setSocketTimeout(clientSettings.readTimeoutMillis);
clientConfiguration.setConnectionMaxIdleMillis(clientSettings.connectionMaxIdleTimeMillis);

if (isStateless) {
clientConfiguration.setRetryPolicy(RETRYABLE_403_RETRY_POLICY);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import org.elasticsearch.common.settings.MockSecureSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.env.Environment;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.watcher.ResourceWatcherService;
Expand Down Expand Up @@ -46,6 +47,7 @@ public void testThereIsADefaultClientByDefault() {
assertThat(defaultSettings.maxConnections, is(ClientConfiguration.DEFAULT_MAX_CONNECTIONS));
assertThat(defaultSettings.maxRetries, is(ClientConfiguration.DEFAULT_RETRY_POLICY.getMaxErrorRetry()));
assertThat(defaultSettings.throttleRetries, is(ClientConfiguration.DEFAULT_THROTTLE_RETRIES));
assertThat(defaultSettings.connectionMaxIdleTimeMillis, is(ClientConfiguration.DEFAULT_CONNECTION_MAX_IDLE_MILLIS));
}

public void testDefaultClientSettingsCanBeSet() {
Expand Down Expand Up @@ -200,6 +202,22 @@ public void testSignerOverrideCanBeSet() {
assertThat(configuration.getSignerOverride(), is(signerOverride));
}

public void testConnectionMaxIdleTimeCanBeSet() {
final TimeValue connectionMaxIdleTimeValue = randomValueOtherThan(
S3ClientSettings.Defaults.CONNECTION_MAX_IDLE_TIME,
ESTestCase::randomTimeValue
);
final Map<String, S3ClientSettings> settings = S3ClientSettings.load(
Settings.builder().put("s3.client.other.connection_max_idle_time", connectionMaxIdleTimeValue).build()
);
assertThat(settings.get("default").connectionMaxIdleTimeMillis, is(S3ClientSettings.Defaults.CONNECTION_MAX_IDLE_TIME.millis()));
assertThat(settings.get("other").connectionMaxIdleTimeMillis, is(connectionMaxIdleTimeValue.millis()));
ClientConfiguration defaultConfiguration = S3Service.buildConfiguration(settings.get("default"), randomBoolean());
assertThat(defaultConfiguration.getConnectionMaxIdleMillis(), is(S3ClientSettings.Defaults.CONNECTION_MAX_IDLE_TIME.millis()));
ClientConfiguration configuration = S3Service.buildConfiguration(settings.get("other"), randomBoolean());
assertThat(configuration.getConnectionMaxIdleMillis(), is(connectionMaxIdleTimeValue.millis()));
}

public void testMaxConnectionsCanBeSet() {
final int maxConnections = between(1, 100);
final Map<String, S3ClientSettings> settings = S3ClientSettings.load(
Expand Down
Loading