Skip to content

Commit

Permalink
S3 Repository: Deprecate remaining repositories.s3.* settings (#24144)
Browse files Browse the repository at this point in the history 
Most of these settings should always be pulled from the repository
settings. A couple were leftover that should be moved to client
settings. The path style access setting should be removed altogether.
This commit adds deprecations for all of these existing settings, as
well as adding new client specific settings for max retries and
throttling.

relates #24143
  • Loading branch information
@rjernst
rjernst committed Apr 26, 2017
1 parent 5d313b6 commit 99e5fa0
Show file tree
Hide file tree
Showing 7 changed files with 94 additions and 36 deletions.
20 changes: 8 additions & 12 deletions ...s/repository-s3/src/main/java/org/elasticsearch/repositories/s3/InternalAwsS3Service.java
View file
Expand Up @@ -77,12 +77,6 @@ public synchronized AmazonS3 client(Settings repositorySettings) {
}

String endpoint = findEndpoint(logger, deprecationLogger, clientSettings, repositorySettings);
Integer maxRetries = getValue(repositorySettings, settings,
S3Repository.Repository.MAX_RETRIES_SETTING,
S3Repository.Repositories.MAX_RETRIES_SETTING);
boolean useThrottleRetries = getValue(repositorySettings, settings,
S3Repository.Repository.USE_THROTTLE_RETRIES_SETTING,
S3Repository.Repositories.USE_THROTTLE_RETRIES_SETTING);
// If the user defined a path style access setting, we rely on it,
// otherwise we use the default value set by the SDK
Boolean pathStyleAccess = null;
Expand All @@ -93,12 +87,11 @@ public synchronized AmazonS3 client(Settings repositorySettings) {
S3Repository.Repositories.PATH_STYLE_ACCESS_SETTING);
}

logger.debug("creating S3 client with client_name [{}], endpoint [{}], max_retries [{}], " +
"use_throttle_retries [{}], path_style_access [{}]",
clientName, endpoint, maxRetries, useThrottleRetries, pathStyleAccess);
logger.debug("creating S3 client with client_name [{}], endpoint [{}], path_style_access [{}]",
clientName, endpoint, pathStyleAccess);

AWSCredentialsProvider credentials = buildCredentials(logger, deprecationLogger, clientSettings, repositorySettings);
ClientConfiguration configuration = buildConfiguration(logger, clientSettings, repositorySettings, maxRetries, endpoint, useThrottleRetries);
ClientConfiguration configuration = buildConfiguration(logger, clientSettings, repositorySettings, endpoint);

client = new AmazonS3Client(credentials, configuration);

Expand All @@ -115,8 +108,8 @@ public synchronized AmazonS3 client(Settings repositorySettings) {
}

// pkg private for tests
static ClientConfiguration buildConfiguration(Logger logger, S3ClientSettings clientSettings, Settings repositorySettings,
Integer maxRetries, String endpoint, boolean useThrottleRetries) {
static ClientConfiguration buildConfiguration(Logger logger, S3ClientSettings clientSettings,
Settings repositorySettings, String endpoint) {
ClientConfiguration clientConfiguration = new ClientConfiguration();
// the response metadata cache is only there for diagnostics purposes,
// but can force objects from every response to the old generation.
Expand All @@ -132,10 +125,13 @@ static ClientConfiguration buildConfiguration(Logger logger, S3ClientSettings cl
clientConfiguration.setProxyPassword(clientSettings.proxyPassword);
}

Integer maxRetries = getRepoValue(repositorySettings, S3Repository.Repository.MAX_RETRIES_SETTING, clientSettings.maxRetries);
if (maxRetries != null) {
// If not explicitly set, default to 3 with exponential backoff policy
clientConfiguration.setMaxErrorRetry(maxRetries);
}
boolean useThrottleRetries = getRepoValue(repositorySettings,
S3Repository.Repository.USE_THROTTLE_RETRIES_SETTING, clientSettings.throttleRetries);
clientConfiguration.setUseThrottleRetries(useThrottleRetries);

// #155: we might have 3rd party users using older S3 API version
Expand Down
32 changes: 25 additions & 7 deletions plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java
View file
Expand Up @@ -31,6 +31,7 @@
import org.elasticsearch.common.settings.SecureSetting;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.repositories.s3.AwsS3Service.CLOUD_S3;
Expand Down Expand Up @@ -58,15 +59,15 @@ class S3ClientSettings {

/** The protocol to use to connect to s3. */
static final Setting.AffixSetting<Protocol> PROTOCOL_SETTING = Setting.affixKeySetting(PREFIX, "protocol",
key -> new Setting<>(key, "https", s -> Protocol.valueOf(s.toUpperCase(Locale.ROOT)), Setting.Property.NodeScope));
key -> new Setting<>(key, "https", s -> Protocol.valueOf(s.toUpperCase(Locale.ROOT)), Property.NodeScope));

/** The host name of a proxy to connect to s3 through. */
static final Setting.AffixSetting<String> PROXY_HOST_SETTING = Setting.affixKeySetting(PREFIX, "proxy.host",
key -> Setting.simpleString(key, Setting.Property.NodeScope));
key -> Setting.simpleString(key, Property.NodeScope));

/** The port of a proxy to connect to s3 through. */
static final Setting.AffixSetting<Integer> PROXY_PORT_SETTING = Setting.affixKeySetting(PREFIX, "proxy.port",
key -> Setting.intSetting(key, 80, 0, 1<<16, Setting.Property.NodeScope));
key -> Setting.intSetting(key, 80, 0, 1<<16, Property.NodeScope));

/** The username of a proxy to connect to s3 through. */
static final Setting.AffixSetting<SecureString> PROXY_USERNAME_SETTING = Setting.affixKeySetting(PREFIX, "proxy.username",
Expand All @@ -78,8 +79,15 @@ class S3ClientSettings {

/** The socket timeout for connecting to s3. */
static final Setting.AffixSetting<TimeValue> READ_TIMEOUT_SETTING = Setting.affixKeySetting(PREFIX, "read_timeout",
key -> Setting.timeSetting(key, TimeValue.timeValueMillis(ClientConfiguration.DEFAULT_SOCKET_TIMEOUT),
Setting.Property.NodeScope));
key -> Setting.timeSetting(key, TimeValue.timeValueMillis(ClientConfiguration.DEFAULT_SOCKET_TIMEOUT), 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, S3Repository.Repositories.MAX_RETRIES_SETTING, 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, S3Repository.Repositories.USE_THROTTLE_RETRIES_SETTING, Property.NodeScope));

/** Credentials to authenticate with s3. */
final BasicAWSCredentials credentials;
Expand Down Expand Up @@ -113,9 +121,15 @@ class S3ClientSettings {
/** The read timeout for the s3 client. */
final int readTimeoutMillis;

/** The number of retries to use for the s3 client. */
final int maxRetries;

/** Whether the s3 client should use an exponential backoff retry policy. */
final boolean throttleRetries;

private S3ClientSettings(BasicAWSCredentials credentials, String endpoint, String region, Protocol protocol,
String proxyHost, int proxyPort, String proxyUsername,
String proxyPassword, String awsSigner, int readTimeoutMillis) {
String proxyPassword, String awsSigner, int readTimeoutMillis, int maxRetries, boolean throttleRetries) {
this.credentials = credentials;
this.region = region;
this.endpoint = endpoint;
Expand All @@ -126,6 +140,8 @@ private S3ClientSettings(BasicAWSCredentials credentials, String endpoint, Strin
this.proxyPassword = proxyPassword;
this.awsSigner = awsSigner;
this.readTimeoutMillis = readTimeoutMillis;
this.maxRetries = maxRetries;
this.throttleRetries = throttleRetries;
}

/**
Expand Down Expand Up @@ -174,7 +190,9 @@ static S3ClientSettings getClientSettings(Settings settings, String clientName)
proxyUsername.toString(),
proxyPassword.toString(),
CLOUD_S3.SIGNER_SETTING.get(settings),
(int)getConfigValue(settings, clientName, READ_TIMEOUT_SETTING, AwsS3Service.CLOUD_S3.READ_TIMEOUT).millis()
(int)getConfigValue(settings, clientName, READ_TIMEOUT_SETTING, AwsS3Service.CLOUD_S3.READ_TIMEOUT).millis(),
getConfigValue(settings, clientName, MAX_RETRIES_SETTING, S3Repository.Repositories.MAX_RETRIES_SETTING),
getConfigValue(settings, clientName, USE_THROTTLE_RETRIES_SETTING, S3Repository.Repositories.USE_THROTTLE_RETRIES_SETTING)
);
}
}
Expand Down
10 changes: 5 additions & 5 deletions plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java
View file
Expand Up @@ -173,8 +173,8 @@ The default behaviour is to detect which access style to use based on the config
in path-style access) and the bucket being accessed (some buckets are not valid DNS names). Setting this flag
will result in path-style access being used for all requests.
*/
Setting<Boolean> PATH_STYLE_ACCESS_SETTING = Setting.boolSetting("repositories.s3.path_style_access", false, Property.NodeScope,
Property.Deprecated);
Setting<Boolean> PATH_STYLE_ACCESS_SETTING = Setting.boolSetting("repositories.s3.path_style_access", false,
Property.NodeScope, Property.Deprecated);
}

/**
Expand Down Expand Up @@ -223,13 +223,13 @@ public interface Repository {
* max_retries
* @see Repositories#MAX_RETRIES_SETTING
*/
Setting<Integer> MAX_RETRIES_SETTING = Setting.intSetting("max_retries", 3);
Setting<Integer> MAX_RETRIES_SETTING = Setting.intSetting("max_retries", 3, Property.Deprecated);
/**
* use_throttle_retries
* @see Repositories#USE_THROTTLE_RETRIES_SETTING
*/
Setting<Boolean> USE_THROTTLE_RETRIES_SETTING = Setting.boolSetting("use_throttle_retries",
ClientConfiguration.DEFAULT_THROTTLE_RETRIES);
ClientConfiguration.DEFAULT_THROTTLE_RETRIES, Property.Deprecated);
/**
* chunk_size
* @see Repositories#CHUNK_SIZE_SETTING
Expand Down Expand Up @@ -261,7 +261,7 @@ public interface Repository {
* path_style_access
* @see Repositories#PATH_STYLE_ACCESS_SETTING
*/
Setting<Boolean> PATH_STYLE_ACCESS_SETTING = Setting.boolSetting("path_style_access", false);
Setting<Boolean> PATH_STYLE_ACCESS_SETTING = Setting.boolSetting("path_style_access", false, Property.Deprecated);
}

private final S3BlobStore blobStore;
Expand Down
2 changes: 2 additions & 0 deletions ...ins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3RepositoryPlugin.java
View file
Expand Up @@ -102,6 +102,8 @@ public List<Setting<?>> getSettings() {
S3ClientSettings.PROXY_USERNAME_SETTING,
S3ClientSettings.PROXY_PASSWORD_SETTING,
S3ClientSettings.READ_TIMEOUT_SETTING,
S3ClientSettings.MAX_RETRIES_SETTING,
S3ClientSettings.USE_THROTTLE_RETRIES_SETTING,

// Register global cloud aws settings: cloud.aws (might have been registered in ec2 plugin)
AwsS3Service.KEY_SETTING,
Expand Down
63 changes: 52 additions & 11 deletions .../repository-s3/src/test/java/org/elasticsearch/repositories/s3/AwsS3ServiceImplTests.java
View file
Expand Up @@ -296,23 +296,69 @@ public void testAWSConfigurationWithAwsAndS3SettingsBackcompat() {
AwsS3Service.CLOUD_S3.READ_TIMEOUT});
}

public void testGlobalMaxRetries() {
Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, null);
public void testGlobalMaxRetriesBackcompat() {
Settings settings = Settings.builder()
.put(S3Repository.Repositories.MAX_RETRIES_SETTING.getKey(), 10)
.build();
launchAWSConfigurationTest(settings, repositorySettings, Protocol.HTTPS, null, -1, null,
null, null, 10, false, 50000);
assertSettingDeprecationsAndWarnings(new Setting<?>[]{S3Repository.Repositories.MAX_RETRIES_SETTING});
launchAWSConfigurationTest(settings, Settings.EMPTY, Protocol.HTTPS, null, -1, null, null,
null, 10, false, 50000);
assertSettingDeprecationsAndWarnings(new Setting<?>[]{
S3Repository.Repositories.MAX_RETRIES_SETTING
});
}

public void testRepositoryMaxRetries() {
Settings settings = Settings.builder()
.put("s3.client.default.max_retries", 5)
.build();
launchAWSConfigurationTest(settings, Settings.EMPTY, Protocol.HTTPS, null, -1, null, null,
null, 5, false, 50000);
}

public void testRepositoryMaxRetriesBackcompat() {
Settings repositorySettings = generateRepositorySettings(null, null, "eu-central", null, 20);
Settings settings = Settings.builder()
.put(S3Repository.Repositories.MAX_RETRIES_SETTING.getKey(), 10)
.build();
launchAWSConfigurationTest(settings, repositorySettings, Protocol.HTTPS, null, -1, null,
null, null, 20, false, 50000);
assertSettingDeprecationsAndWarnings(new Setting<?>[]{
S3Repository.Repositories.MAX_RETRIES_SETTING,
S3Repository.Repository.MAX_RETRIES_SETTING
});
}

public void testGlobalThrottleRetriesBackcompat() {
Settings settings = Settings.builder()
.put(S3Repository.Repositories.USE_THROTTLE_RETRIES_SETTING.getKey(), true)
.build();
launchAWSConfigurationTest(settings, Settings.EMPTY, Protocol.HTTPS, null, -1, null, null,
null, 3, true, 50000);
assertSettingDeprecationsAndWarnings(new Setting<?>[]{
S3Repository.Repositories.USE_THROTTLE_RETRIES_SETTING
});
}

public void testRepositoryThrottleRetries() {
Settings settings = Settings.builder()
.put("s3.client.default.use_throttle_retries", true)
.build();
launchAWSConfigurationTest(settings, Settings.EMPTY, Protocol.HTTPS, null, -1, null, null,
null, 3, true, 50000);
}

public void testRepositoryThrottleRetriesBackcompat() {
Settings repositorySettings = Settings.builder()
.put(S3Repository.Repository.USE_THROTTLE_RETRIES_SETTING.getKey(), true).build();
Settings settings = Settings.builder()
.put(S3Repository.Repositories.USE_THROTTLE_RETRIES_SETTING.getKey(), false)
.build();
launchAWSConfigurationTest(settings, repositorySettings, Protocol.HTTPS, null, -1, null, null,
null, 3, true, 50000);
assertSettingDeprecationsAndWarnings(new Setting<?>[]{
S3Repository.Repositories.USE_THROTTLE_RETRIES_SETTING,
S3Repository.Repository.USE_THROTTLE_RETRIES_SETTING
});
}

protected void launchAWSConfigurationTest(Settings settings,
Expand All @@ -326,14 +372,9 @@ protected void launchAWSConfigurationTest(Settings settings,
Integer expectedMaxRetries,
boolean expectedUseThrottleRetries,
int expectedReadTimeout) {
Integer maxRetries = S3Repository.getValue(singleRepositorySettings, settings,
S3Repository.Repository.MAX_RETRIES_SETTING, S3Repository.Repositories.MAX_RETRIES_SETTING);
Boolean useThrottleRetries = S3Repository.getValue(singleRepositorySettings, settings,
S3Repository.Repository.USE_THROTTLE_RETRIES_SETTING, S3Repository.Repositories.USE_THROTTLE_RETRIES_SETTING);

S3ClientSettings clientSettings = S3ClientSettings.getClientSettings(settings, "default");
ClientConfiguration configuration = InternalAwsS3Service.buildConfiguration(logger, clientSettings,
singleRepositorySettings, maxRetries, null, useThrottleRetries);
ClientConfiguration configuration = InternalAwsS3Service.buildConfiguration(logger, clientSettings, singleRepositorySettings, null);

assertThat(configuration.getResponseMetadataCacheSize(), is(0));
assertThat(configuration.getProtocol(), is(expectedProtocol));
Expand Down
2 changes: 1 addition & 1 deletion plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3RepositoryTests.java
View file
Expand Up @@ -123,7 +123,7 @@ public void testBasePathSetting() throws IOException {
Settings settings = Settings.builder().put(Repositories.BASE_PATH_SETTING.getKey(), "/foo/bar").build();
s3repo = new S3Repository(metadata, settings, NamedXContentRegistry.EMPTY, new DummyS3Service());
assertEquals("foo/bar/", s3repo.basePath().buildAsString()); // make sure leading `/` is removed and trailing is added
assertSettingDeprecationsAndWarnings(new Setting<?>[]{Repositories.BASE_PATH_SETTING},
assertSettingDeprecationsAndWarnings(new Setting<?>[]{ Repositories.BASE_PATH_SETTING },
"S3 repository base_path trimming the leading `/`, and leading `/` will not be supported for the S3 repository in " +
"future releases");
}
Expand Down
1 change: 1 addition & 0 deletions test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java
View file
Expand Up @@ -310,6 +310,7 @@ protected final void assertWarnings(String... expectedWarnings) {
}
try {
final List<String> actualWarnings = threadContext.getResponseHeaders().get("Warning");
assertNotNull(actualWarnings);
final Set<String> actualWarningValues =
actualWarnings.stream().map(DeprecationLogger::extractWarningValueFromWarningHeader).collect(Collectors.toSet());
for (String msg : expectedWarnings) {
Expand Down

0 comments on commit 99e5fa0

Please sign in to comment.