Skip to content

Cosmetic fixes to repository-s3 #125397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 21, 2025
Merged

Cosmetic fixes to repository-s3 #125397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c1b8ca0
Cosmetic fixes to `repository-s3`
DaveCTurner Mar 21, 2025
7a42169
HttpStatus -> RestStatus
DaveCTurner Mar 21, 2025
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
17 changes: 12 additions & 5 deletions modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,28 +121,28 @@ final class S3ClientSettings {
static final Setting.AffixSetting<TimeValue> READ_TIMEOUT_SETTING = Setting.affixKeySetting(
PREFIX,
"read_timeout",
key -> Setting.timeSetting(key, TimeValue.timeValueMillis(ClientConfiguration.DEFAULT_SOCKET_TIMEOUT), Property.NodeScope)
key -> Setting.timeSetting(key, Defaults.READ_TIMEOUT, Property.NodeScope)
);

/** The maximum number of concurrent connections to use. */
static final Setting.AffixSetting<Integer> MAX_CONNECTIONS_SETTING = Setting.affixKeySetting(
PREFIX,
"max_connections",
key -> Setting.intSetting(key, ClientConfiguration.DEFAULT_MAX_CONNECTIONS, 1, Property.NodeScope)
key -> Setting.intSetting(key, Defaults.MAX_CONNECTIONS, 1, Property.NodeScope)
);

/** The number of retries to use when an s3 request fails. */
static final Setting.AffixSetting<Integer> MAX_RETRIES_SETTING = Setting.affixKeySetting(
PREFIX,
"max_retries",
key -> Setting.intSetting(key, ClientConfiguration.DEFAULT_RETRY_POLICY.getMaxErrorRetry(), 0, Property.NodeScope)
key -> Setting.intSetting(key, Defaults.RETRY_COUNT, 0, Property.NodeScope)
);

/** Whether retries should be throttled (ie use backoff). */
static final Setting.AffixSetting<Boolean> USE_THROTTLE_RETRIES_SETTING = Setting.affixKeySetting(
PREFIX,
"use_throttle_retries",
key -> Setting.boolSetting(key, ClientConfiguration.DEFAULT_THROTTLE_RETRIES, Property.NodeScope)
key -> Setting.boolSetting(key, Defaults.THROTTLE_RETRIES, Property.NodeScope)
);

/** Whether the s3 client should use path style access. */
Expand Down Expand Up @@ -335,7 +335,7 @@ S3ClientSettings refine(Settings repositorySettings) {

/**
* Load all client settings from the given settings.
*
* <p>
* Note this will always at least return a client named "default".
*/
static Map<String, S3ClientSettings> load(Settings settings) {
Expand Down Expand Up @@ -501,4 +501,11 @@ private static <T> T getRepoSettingOrDefault(Setting.AffixSetting<T> setting, Se
}
return defaultValue;
}

static final class Defaults {
static final TimeValue READ_TIMEOUT = TimeValue.timeValueMillis(ClientConfiguration.DEFAULT_SOCKET_TIMEOUT);
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;
}
}
15 changes: 8 additions & 7 deletions modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import com.amazonaws.services.securitytoken.AWSSecurityTokenServiceClient;
import com.amazonaws.services.securitytoken.AWSSecurityTokenServiceClientBuilder;

import org.apache.http.HttpStatus;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.elasticsearch.ElasticsearchException;
Expand All @@ -44,6 +43,7 @@
import org.elasticsearch.core.IOUtils;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.env.Environment;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.watcher.FileChangesListener;
import org.elasticsearch.watcher.FileWatcher;
import org.elasticsearch.watcher.ResourceWatcherService;
Expand Down Expand Up @@ -115,8 +115,8 @@ class S3Service implements Closeable {

/**
* Refreshes the settings for the AmazonS3 clients and clears the cache of
* existing clients. New clients will be build using these new settings. Old
* clients are usable until released. On release they will be destroyed instead
* existing clients. New clients will be built using these new settings. Old
* clients are usable until released. On release, they will be destroyed instead
* of being returned to the cache.
*/
public synchronized void refreshAndClearCache(Map<String, S3ClientSettings> clientsSettings) {
Expand All @@ -126,7 +126,7 @@ public synchronized void refreshAndClearCache(Map<String, S3ClientSettings> clie
this.staticClientSettings = Maps.ofEntries(clientsSettings.entrySet());
derivedClientSettings = emptyMap();
assert this.staticClientSettings.containsKey("default") : "always at least have 'default'";
// clients are built lazily by {@link client}
/* clients are built lazily by {@link #client} */
}

/**
Expand Down Expand Up @@ -341,7 +341,8 @@ public void refresh() {
* <ul>
* <li>Reads the location of the web identity token not from AWS_WEB_IDENTITY_TOKEN_FILE, but from a symlink
* in the plugin directory, so we don't need to create a hardcoded read file permission for the plugin.</li>
* <li>Supports customization of the STS endpoint via a system property, so we can test it against a test fixture.</li>
* <li>Supports customization of the STS (Security Token Service) endpoint via a system property, so we can
* test it against a test fixture.</li>
* <li>Supports gracefully shutting down the provider and the STS client.</li>
* </ul>
*/
Expand Down Expand Up @@ -384,7 +385,7 @@ static class CustomWebIdentityTokenCredentialsProvider implements AWSCredentials
if (roleArn == null) {
LOGGER.warn(
"Unable to use a web identity token for authentication. The AWS_WEB_IDENTITY_TOKEN_FILE environment "
+ "variable is set, but either AWS_ROLE_ARN is missing"
+ "variable is set, but AWS_ROLE_ARN is missing"
);
return;
}
Expand Down Expand Up @@ -528,7 +529,7 @@ interface JvmEnvironment {
return true;
}
if (exception instanceof AmazonServiceException ase) {
return ase.getStatusCode() == HttpStatus.SC_FORBIDDEN && "InvalidAccessKeyId".equals(ase.getErrorCode());
return ase.getStatusCode() == RestStatus.FORBIDDEN.getStatus() && "InvalidAccessKeyId".equals(ase.getErrorCode());
}
return false;
})
Expand Down