Expose S3 connection max idle time as a setting #125552
Conversation
Resolves: ES-10815
ywangd
added
>enhancement
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Hi @ywangd, I've created a changelog YAML for you. |
| 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) | ||
| ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The biggest obstacle we're facing in the upgrade to AWS SDK v2 is finding equivalent behaviour (and tests) for all the existing settings. It's far from trivial, and this PR adds to that burden. Can we hold off on this change until the SDK upgrade is complete instead please?
|
Sure. There is no urgency for this to be completed. |
|
The AWS SDK upgrade to v2 is now complete so I think we can carry on with this now. I believe the correct way to pass this setting into the new SDK is as follows: diff --git a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java
index fc18f4bc97a8..f76fe17702a3 100644
--- a/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java
+++ b/modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java
@@ -310,6 +310,7 @@ class S3Service extends AbstractLifecycleComponent {
httpClientBuilder.maxConnections(clientSettings.maxConnections);
httpClientBuilder.socketTimeout(Duration.ofMillis(clientSettings.readTimeoutMillis));
+ httpClientBuilder.connectionMaxIdleTime(...);
Optional<ProxyConfiguration> proxyConfiguration = buildProxyConfiguration(clientSettings);
if (proxyConfiguration.isPresent()) { |
This is a new setting added in elastic/elasticsearch#125552
|
Thanks for the prompt. I updated the PR to reflect latest changes. Also raised a doc PR elastic/docs-content#2982 |
| boolean addPurposeCustomQueryParameter, | ||
| String region | ||
| String region, | ||
| long connectionMaxIdleTimeMillis |
There was a problem hiding this comment.
Nit: I'd slightly prefer this to be grouped next to readTimeoutMillis (here, and in the field declaration, and in .equals() and .hashcode() etc).
ywangd
added
the
auto-merge-without-approval
This is a new setting added in elastic/elasticsearch#125552
Resolves: ES-10815
3 participants
Resolves: ES-10815