Skip to content

Commit cfa8ec9

Browse files
DiannaHohenseeDiannaHohensee
committed
throttleRetries is deprecated, remove its use from the code; working on AwsS3ServiceImplTests, configurable settings
1 parent a171e34 commit cfa8ec9

File tree

5 files changed

+57
-58
lines changed

5 files changed

+57
-58
lines changed

‎modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,7 @@ public enum AwsSignerOverrideType {
185185
// Note: AWS4UnsignedPayloadSignerType is no longer supported, there is no equivalent in V2 short of a custom signer.
186186
// Note: QueryStringSigner is deprecated in V2, thus not given support.
187187
// TODO NOMERGE let's find better names for these things and make this less of a breaking change
188+
// TODO NOMERGE: why are these Signer types deprecated? What's the alternative?
188189
SDKV1_AWS_V4_SIGNER_TYPE("Aws4SignerType", Aws4Signer::create),
189190
SDKV1_AWS_S3_V4_SIGNER_TYPE("AwsS3V4Signer", AwsS3V4Signer::create),
190191
SDKV1_NOOP_SIGNER_TYPE("NoopSigner", NoOpSigner::new),
@@ -242,9 +243,6 @@ public enum AwsSignerOverrideType {
242243
/** The number of retries to use for the s3 client. */
243244
final int maxRetries;
244245

245-
/** Whether the s3 client should use an exponential backoff retry policy. */
246-
final boolean throttleRetries; // TODO: remove, no longer supported in v2
247-
248246
/** Whether the s3 client should use path style access. */
249247
final boolean pathStyleAccess;
250248

@@ -268,7 +266,6 @@ private S3ClientSettings(
268266
int readTimeoutMillis,
269267
int maxConnections,
270268
int maxRetries,
271-
boolean throttleRetries,
272269
boolean pathStyleAccess,
273270
boolean disableChunkedEncoding,
274271
String region,
@@ -284,7 +281,6 @@ private S3ClientSettings(
284281
this.readTimeoutMillis = readTimeoutMillis;
285282
this.maxConnections = maxConnections;
286283
this.maxRetries = maxRetries;
287-
this.throttleRetries = throttleRetries;
288284
this.pathStyleAccess = pathStyleAccess;
289285
this.disableChunkedEncoding = disableChunkedEncoding;
290286
this.region = region;
@@ -313,7 +309,6 @@ S3ClientSettings refine(Settings repositorySettings) {
313309
);
314310
final int newMaxConnections = getRepoSettingOrDefault(MAX_CONNECTIONS_SETTING, normalizedSettings, maxConnections);
315311
final int newMaxRetries = getRepoSettingOrDefault(MAX_RETRIES_SETTING, normalizedSettings, maxRetries);
316-
final boolean newThrottleRetries = getRepoSettingOrDefault(USE_THROTTLE_RETRIES_SETTING, normalizedSettings, throttleRetries);
317312
final boolean newPathStyleAccess = getRepoSettingOrDefault(USE_PATH_STYLE_ACCESS, normalizedSettings, pathStyleAccess);
318313
final boolean newDisableChunkedEncoding = getRepoSettingOrDefault(
319314
DISABLE_CHUNKED_ENCODING,
@@ -335,7 +330,6 @@ S3ClientSettings refine(Settings repositorySettings) {
335330
&& newReadTimeoutMillis == readTimeoutMillis
336331
&& maxConnections == newMaxConnections
337332
&& maxRetries == newMaxRetries
338-
&& newThrottleRetries == throttleRetries
339333
&& Objects.equals(credentials, newCredentials)
340334
&& newPathStyleAccess == pathStyleAccess
341335
&& newDisableChunkedEncoding == disableChunkedEncoding
@@ -354,7 +348,6 @@ S3ClientSettings refine(Settings repositorySettings) {
354348
newReadTimeoutMillis,
355349
newMaxConnections,
356350
newMaxRetries,
357-
newThrottleRetries,
358351
newPathStyleAccess,
359352
newDisableChunkedEncoding,
360353
newRegion,
@@ -462,7 +455,6 @@ static S3ClientSettings getClientSettings(final Settings settings, final String
462455
Math.toIntExact(getConfigValue(settings, clientName, READ_TIMEOUT_SETTING).millis()),
463456
getConfigValue(settings, clientName, MAX_CONNECTIONS_SETTING),
464457
getConfigValue(settings, clientName, MAX_RETRIES_SETTING),
465-
getConfigValue(settings, clientName, USE_THROTTLE_RETRIES_SETTING),
466458
getConfigValue(settings, clientName, USE_PATH_STYLE_ACCESS),
467459
getConfigValue(settings, clientName, DISABLE_CHUNKED_ENCODING),
468460
getConfigValue(settings, clientName, REGION),
@@ -484,7 +476,6 @@ public boolean equals(final Object o) {
484476
&& readTimeoutMillis == that.readTimeoutMillis
485477
&& maxConnections == that.maxConnections
486478
&& maxRetries == that.maxRetries
487-
&& throttleRetries == that.throttleRetries
488479
&& Objects.equals(credentials, that.credentials)
489480
&& Objects.equals(endpoint, that.endpoint)
490481
&& Objects.equals(proxyHost, that.proxyHost)
@@ -509,7 +500,6 @@ public int hashCode() {
509500
readTimeoutMillis,
510501
maxRetries,
511502
maxConnections,
512-
throttleRetries,
513503
disableChunkedEncoding,
514504
region,
515505
signerOverride

‎modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java‎

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import software.amazon.awssdk.auth.credentials.InstanceProfileCredentialsProvider;
1818
import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
1919
import software.amazon.awssdk.awscore.exception.AwsServiceException;
20+
import software.amazon.awssdk.awscore.retry.AwsRetryStrategy;
2021
import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration;
2122
import software.amazon.awssdk.core.client.config.SdkAdvancedClientOption;
2223
import software.amazon.awssdk.http.SdkHttpClient;
@@ -65,6 +66,7 @@
6566
import java.time.Duration;
6667
import java.util.Map;
6768
import java.util.Objects;
69+
import java.util.Optional;
6870
import java.util.concurrent.CompletableFuture;
6971
import java.util.function.Consumer;
7072

@@ -253,7 +255,10 @@ static SdkHttpClient buildHttpClient(S3ClientSettings clientSettings, @Nullable
253255
httpClientBuilder.maxConnections(clientSettings.maxConnections);
254256
httpClientBuilder.socketTimeout(Duration.ofMillis(clientSettings.readTimeoutMillis));
255257

256-
applyProxyConfiguration(clientSettings, httpClientBuilder);
258+
Optional<ProxyConfiguration> proxyConfiguration = buildProxyConfiguration(clientSettings);
259+
if (proxyConfiguration.isPresent()) {
260+
httpClientBuilder.proxyConfiguration(proxyConfiguration.get());
261+
}
257262

258263
if (dnsResolver != null) {
259264
httpClientBuilder.dnsResolver(dnsResolver);
@@ -271,25 +276,29 @@ static boolean RETRYABLE_403_RETRY_PREDICATE(Throwable e) {
271276

272277
static ClientOverrideConfiguration buildConfiguration(S3ClientSettings clientSettings, boolean isStateless) {
273278
ClientOverrideConfiguration.Builder clientOverrideConfiguration = ClientOverrideConfiguration.builder();
274-
275-
clientOverrideConfiguration.retryStrategy(builder -> {
276-
builder.maxAttempts(clientSettings.maxRetries + 1 /* first attempt is not a retry */);
277-
// TODO NOMERGE: revisit this, does it still make sense to specially retry?
278-
// -- dct: yes, in serverless we sometimes get 403s during because of delays in propagating updated credentials
279-
// (IAM is not strongly consistent); TODO NOMERGE this should be covered by some end-to-end test, and documented more accurately
280-
if (isStateless) {
281-
// Create a 403 error retryable policy.
282-
builder.retryOnException(S3Service::RETRYABLE_403_RETRY_PREDICATE);
283-
}
284-
});
285279
clientOverrideConfiguration.putAdvancedOption(SdkAdvancedClientOption.SIGNER, clientSettings.signerOverride.signerFactory.get());
280+
var retryStrategyBuilder = AwsRetryStrategy.standardRetryStrategy()
281+
.toBuilder()
282+
.maxAttempts(clientSettings.maxRetries + 1 /* first attempt is not a retry */);
283+
if (isStateless) {
284+
// Create a 403 error retryable policy. In serverless we sometimes get 403s during because of delays in propagating updated
285+
// credentials because IAM is not strongly consistent.
286+
// TODO NOMERGE this should be covered by some end-to-end test, and documented more accurately
287+
retryStrategyBuilder.retryOnException(S3Service::RETRYABLE_403_RETRY_PREDICATE);
288+
}
289+
clientOverrideConfiguration.retryStrategy(retryStrategyBuilder.build());
286290
return clientOverrideConfiguration.build();
287291
}
288292

289-
private static void applyProxyConfiguration(S3ClientSettings clientSettings, ApacheHttpClient.Builder httpClientBuilder) {
293+
/**
294+
* Populates a {@link ProxyConfiguration} with any user specified settings via {@link S3ClientSettings}, if any are set.
295+
* Otherwise, returns empty Optional.
296+
*/
297+
// pkg private for tests
298+
static Optional<ProxyConfiguration> buildProxyConfiguration(S3ClientSettings clientSettings) {
290299
// If proxy settings are provided
291300
if (Strings.hasText(clientSettings.proxyHost)) {
292-
final var uriBuilder = new URIBuilder();
301+
final URIBuilder uriBuilder = new URIBuilder();
293302
uriBuilder.setScheme(clientSettings.proxyScheme.getSchemeString())
294303
.setHost(clientSettings.proxyHost)
295304
.setPort(clientSettings.proxyPort);
@@ -299,15 +308,17 @@ private static void applyProxyConfiguration(S3ClientSettings clientSettings, Apa
299308
} catch (URISyntaxException e) {
300309
throw new IllegalArgumentException(e);
301310
}
302-
httpClientBuilder.proxyConfiguration(
311+
312+
return Optional.of(
303313
ProxyConfiguration.builder()
304314
.endpoint(proxyUri)
305-
.scheme(clientSettings.proxyScheme.getSchemeString())
315+
.scheme(clientSettings.proxyScheme.getSchemeString()) // TODO NOMERGE do we need this again?
306316
.username(clientSettings.proxyUsername)
307317
.password(clientSettings.proxyPassword)
308318
.build()
309319
);
310320
}
321+
return Optional.empty();
311322
}
312323

313324
// pkg private for tests

‎modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/AwsS3ServiceImplTests.java‎

Lines changed: 22 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@
99

1010
package org.elasticsearch.repositories.s3;
1111

12-
import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
13-
1412
import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
1513
import software.amazon.awssdk.auth.credentials.AwsCredentials;
1614
import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
@@ -37,7 +35,6 @@
3735
import static org.hamcrest.Matchers.is;
3836
import static org.hamcrest.Matchers.startsWith;
3937

40-
@ThreadLeakScope(ThreadLeakScope.Scope.NONE) // TODO NOMERGE
4138
public class AwsS3ServiceImplTests extends ESTestCase {
4239

4340
private final S3Service.CustomWebIdentityTokenCredentialsProvider webIdentityTokenCredentialsProvider = Mockito.mock(
@@ -161,8 +158,8 @@ public void testAWSDefaultConfiguration() {
161158
-1,
162159
null,
163160
null,
161+
null,
164162
3,
165-
S3ClientSettings.Defaults.THROTTLE_RETRIES,
166163
Math.toIntExact(S3ClientSettings.Defaults.READ_TIMEOUT.seconds())
167164
);
168165
}
@@ -173,53 +170,49 @@ public void testAWSConfigurationWithAwsSettings() {
173170
secureSettings.setString("s3.client.default.proxy.password", "aws_proxy_password");
174171
final Settings settings = Settings.builder()
175172
.setSecureSettings(secureSettings)
176-
.put("s3.client.default.protocol", "http")
177173
.put("s3.client.default.proxy.host", "aws_proxy_host")
178174
.put("s3.client.default.proxy.port", 8080)
175+
.put("s3.client.default.proxy.scheme", "http")
179176
.put("s3.client.default.read_timeout", "10s")
180177
.build();
181-
launchAWSConfigurationTest(
182-
settings,
183-
"aws_proxy_host",
184-
8080,
185-
"aws_proxy_username",
186-
"aws_proxy_password",
187-
3,
188-
S3ClientSettings.Defaults.THROTTLE_RETRIES,
189-
10000
190-
);
178+
launchAWSConfigurationTest(settings, "aws_proxy_host", 8080, "http", "aws_proxy_username", "aws_proxy_password", 3, 10000);
191179
}
192180

193181
public void testRepositoryMaxRetries() {
194182
final Settings settings = Settings.builder().put("s3.client.default.max_retries", 5).build();
195-
launchAWSConfigurationTest(settings, null, -1, null, null, 5, S3ClientSettings.Defaults.THROTTLE_RETRIES, 50000);
183+
launchAWSConfigurationTest(settings, null, -1, null, null, null, 5, 50000);
196184
}
197185

198186
private void launchAWSConfigurationTest(
199187
Settings settings,
200188
String expectedProxyHost,
201189
int expectedProxyPort,
190+
String expectedHttpScheme,
202191
String expectedProxyUsername,
203192
String expectedProxyPassword,
204193
Integer expectedMaxRetries,
205-
boolean expectedUseThrottleRetries,
206194
int expectedReadTimeout
207195
) {
208-
209196
final S3ClientSettings clientSettings = S3ClientSettings.getClientSettings(settings, "default");
210-
final var httpClient = S3Service.buildHttpClient(clientSettings, null);
197+
198+
final var proxyClientConfiguration = S3Service.buildProxyConfiguration(clientSettings);
199+
if (proxyClientConfiguration.isPresent()) {
200+
final var proxyConfig = proxyClientConfiguration.get();
201+
assertThat(proxyConfig.username(), is(expectedProxyUsername));
202+
assertThat(proxyConfig.password(), is(expectedProxyPassword));
203+
assertThat(proxyConfig.scheme(), is(expectedHttpScheme));
204+
// TODO NOMERGE: something about URI is broken here.
205+
// In S3Service, URI proxyUri returns the right endpoint, but not host and port pieces.
206+
// Can't get endpoint here (though toString() surfaces it).
207+
// assertThat(proxyConfig.host(), is(expectedProxyHost));
208+
// assertThat(proxyConfig.port(), is(expectedProxyPort));
209+
}
210+
211211
final ClientOverrideConfiguration configuration = S3Service.buildConfiguration(clientSettings, false);
212+
assertThat(configuration.retryStrategy().get().maxAttempts(), is(expectedMaxRetries + 1));
212213

213-
// TODO NOMERGE
214-
// assertThat(configuration.(), getResponseMetadataCacheSize(), is(0));
215-
// assertThat(httpClientBuilder.proxyConfiguration(), is(expectedProxyHost));
216-
// assertThat(configuration.getProxyPort(), is(expectedProxyPort));
217-
// assertThat(configuration.getProxyUsername(), is(expectedProxyUsername));
218-
// assertThat(configuration.getProxyPassword(), is(expectedProxyPassword));
219-
// assertThat(configuration.getMaxErrorRetry(), is(expectedMaxRetries));
220-
// assertThat(configuration.useThrottledRetries(), is(expectedUseThrottleRetries));
221-
// assertThat(configuration.getSocketTimeout(), is(expectedReadTimeout));
222-
// assertThat(configuration.retryPolicy(), is(PredefinedRetryPolicies.DEFAULT));
214+
// TODO NOMERGE: consider whether this needs to be tested elsewhere.
215+
// assertThat(configuration.getSocketTimeout(), is(expectedReadTimeout)); // set on the httpClient
223216
}
224217

225218
public void testEndpointSetting() {

‎modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ClientSettingsTests.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ public void testThereIsADefaultClientByDefault() {
4747
assertThat(defaultSettings.readTimeoutMillis, is(Math.toIntExact(S3ClientSettings.Defaults.READ_TIMEOUT.millis())));
4848
assertThat(defaultSettings.maxConnections, is(S3ClientSettings.Defaults.MAX_CONNECTIONS));
4949
assertThat(defaultSettings.maxRetries, is(S3ClientSettings.Defaults.RETRY_COUNT));
50-
assertThat(defaultSettings.throttleRetries, is(S3ClientSettings.Defaults.THROTTLE_RETRIES));
5150
}
5251

5352
public void testDefaultClientSettingsCanBeSet() {

‎modules/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ServiceTests.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,19 @@ public class S3ServiceTests extends ESTestCase {
3030

3131
public void testCachedClientsAreReleased() throws IOException {
3232
final S3Service s3Service = new S3Service(mock(Environment.class), Settings.EMPTY, mock(ResourceWatcherService.class));
33-
final Settings settings = Settings.builder().put("endpoint", "http://first").build();
33+
final String endpointOverride = "http://first";
34+
final Settings settings = Settings.builder().put("endpoint", endpointOverride).build();
3435
final RepositoryMetadata metadata1 = new RepositoryMetadata("first", "s3", settings);
3536
final RepositoryMetadata metadata2 = new RepositoryMetadata("second", "s3", settings);
3637
final S3ClientSettings clientSettings = s3Service.settings(metadata2);
3738
final S3ClientSettings otherClientSettings = s3Service.settings(metadata2);
3839
assertSame(clientSettings, otherClientSettings);
3940
final AmazonS3Reference reference = s3Service.client(metadata1);
41+
42+
// TODO NOMERGE: move to its own test.
43+
assertEquals(endpointOverride, reference.client().serviceClientConfiguration().endpointOverride().get().toString());
44+
assertEquals("us-east-1", reference.client().serviceClientConfiguration().region().toString());
45+
4046
reference.close();
4147
s3Service.close();
4248
final AmazonS3Reference referenceReloaded = s3Service.client(metadata1);

0 commit comments

Comments
 (0)