Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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 @@ -182,6 +182,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 @@ -186,6 +186,12 @@ final class S3ClientSettings {
key -> Setting.boolSetting(key, false, 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 AwsCredentials credentials;

Expand Down Expand Up @@ -233,6 +239,11 @@ final class S3ClientSettings {
/** Region to use for signing requests or empty string to use default. */
final String region;

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

private S3ClientSettings(
AwsCredentials credentials,
HttpScheme protocol,
Expand All @@ -248,7 +259,8 @@ private S3ClientSettings(
boolean pathStyleAccess,
boolean disableChunkedEncoding,
boolean addPurposeCustomQueryParameter,
String region
String region,
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.protocol = protocol;
Expand All @@ -265,6 +277,7 @@ private S3ClientSettings(
this.disableChunkedEncoding = disableChunkedEncoding;
this.addPurposeCustomQueryParameter = addPurposeCustomQueryParameter;
this.region = region;
this.connectionMaxIdleTimeMillis = connectionMaxIdleTimeMillis;
}

/**
Expand Down Expand Up @@ -308,6 +321,11 @@ S3ClientSettings refine(Settings repositorySettings) {
newCredentials = credentials;
}
final String newRegion = getRepoSettingOrDefault(REGION, normalizedSettings, region);
final long newConnectionMaxIdleTimeMillis = getRepoSettingOrDefault(
CONNECTION_MAX_IDLE_TIME_SETTING,
normalizedSettings,
TimeValue.timeValueMillis(connectionMaxIdleTimeMillis)
).millis();
if (Objects.equals(protocol, newProtocol)
&& Objects.equals(endpoint, newEndpoint)
&& Objects.equals(proxyHost, newProxyHost)
Expand All @@ -320,7 +338,8 @@ S3ClientSettings refine(Settings repositorySettings) {
&& newPathStyleAccess == pathStyleAccess
&& newDisableChunkedEncoding == disableChunkedEncoding
&& newAddPurposeCustomQueryParameter == addPurposeCustomQueryParameter
&& Objects.equals(region, newRegion)) {
&& Objects.equals(region, newRegion)
&& Objects.equals(connectionMaxIdleTimeMillis, newConnectionMaxIdleTimeMillis)) {
return this;
}
return new S3ClientSettings(
Expand All @@ -338,7 +357,8 @@ S3ClientSettings refine(Settings repositorySettings) {
newPathStyleAccess,
newDisableChunkedEncoding,
newAddPurposeCustomQueryParameter,
newRegion
newRegion,
newConnectionMaxIdleTimeMillis
);
}

Expand Down Expand Up @@ -446,7 +466,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, ADD_PURPOSE_CUSTOM_QUERY_PARAMETER),
getConfigValue(settings, clientName, REGION)
getConfigValue(settings, clientName, REGION),
getConfigValue(settings, clientName, CONNECTION_MAX_IDLE_TIME_SETTING).millis()
);
}
}
Expand All @@ -473,7 +494,8 @@ public boolean equals(final Object o) {
&& Objects.equals(proxyPassword, that.proxyPassword)
&& Objects.equals(disableChunkedEncoding, that.disableChunkedEncoding)
&& Objects.equals(addPurposeCustomQueryParameter, that.addPurposeCustomQueryParameter)
&& Objects.equals(region, that.region);
&& Objects.equals(region, that.region)
&& Objects.equals(connectionMaxIdleTimeMillis, that.connectionMaxIdleTimeMillis);
}

@Override
Expand All @@ -492,7 +514,8 @@ public int hashCode() {
maxConnections,
disableChunkedEncoding,
addPurposeCustomQueryParameter,
region
region,
connectionMaxIdleTimeMillis
);
}

Expand All @@ -512,5 +535,6 @@ static final class Defaults {
static final TimeValue READ_TIMEOUT = TimeValue.timeValueSeconds(50);
static final int MAX_CONNECTIONS = 50;
static final int RETRY_COUNT = 3;
static final TimeValue CONNECTION_MAX_IDLE_TIME = TimeValue.timeValueSeconds(60);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ public List<Setting<?>> getSettings() {
S3ClientSettings.UNUSED_SIGNER_OVERRIDE,
S3ClientSettings.ADD_PURPOSE_CUSTOM_QUERY_PARAMETER,
S3ClientSettings.REGION,
S3ClientSettings.CONNECTION_MAX_IDLE_TIME_SETTING,
S3Service.REPOSITORY_S3_CAS_TTL_SETTING,
S3Service.REPOSITORY_S3_CAS_ANTI_CONTENTION_DELAY_SETTING,
S3Repository.ACCESS_KEY_SETTING,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ private SdkHttpClient buildHttpClient(

httpClientBuilder.maxConnections(clientSettings.maxConnections);
httpClientBuilder.socketTimeout(Duration.ofMillis(clientSettings.readTimeoutMillis));
httpClientBuilder.connectionMaxIdleTime(Duration.ofMillis(clientSettings.connectionMaxIdleTimeMillis));

Optional<ProxyConfiguration> proxyConfiguration = buildProxyConfiguration(clientSettings);
if (proxyConfiguration.isPresent()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.elasticsearch.common.settings.MockSecureSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.DeterministicTaskQueue;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.env.Environment;
import org.elasticsearch.test.ClusterServiceUtils;
import org.elasticsearch.test.ESTestCase;
Expand Down Expand Up @@ -49,6 +50,7 @@ public void testThereIsADefaultClientByDefault() {
assertThat(defaultSettings.readTimeoutMillis, is(Math.toIntExact(S3ClientSettings.Defaults.READ_TIMEOUT.millis())));
assertThat(defaultSettings.maxConnections, is(S3ClientSettings.Defaults.MAX_CONNECTIONS));
assertThat(defaultSettings.maxRetries, is(S3ClientSettings.Defaults.RETRY_COUNT));
assertThat(defaultSettings.connectionMaxIdleTimeMillis, is(S3ClientSettings.Defaults.CONNECTION_MAX_IDLE_TIME.millis()));
}

public void testDefaultClientSettingsCanBeSet() {
Expand Down Expand Up @@ -206,6 +208,21 @@ public void testRegionCanBeSet() {
}
}

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()));

// the default appears in the docs so let's make sure it doesn't change:
assertEquals(TimeValue.timeValueSeconds(60), S3ClientSettings.Defaults.CONNECTION_MAX_IDLE_TIME);
}

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