Skip to content

Remove httpRequestTimeout and executionTimeout features #494

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.

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 1 commit into from
May 23, 2018
Merged
Show file tree
Hide file tree
Changes from all 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 .changes/next-release/removal-AWSSDKforJavav2-1b4f69d.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"category": "AWS SDK for Java v2",
"type": "removal",
"description": "Remove httpRequestTimeout and totalExecutionTimeout features"
}
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,6 @@ private AwsSyncClientConfiguration initializedSyncConfiguration() {

private ClientOverrideConfiguration initializedOverrideConfiguration() {
return ClientOverrideConfiguration.builder()
.httpRequestTimeout(Duration.ofSeconds(2))
.totalExecutionTimeout(Duration.ofSeconds(4))
.gzipEnabled(true)
.addAdditionalHttpHeader("header", "value")
.advancedOption(AwsAdvancedClientOption.USER_AGENT_PREFIX, "userAgentPrefix")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ public AmazonSyncHttpClient build() {
SdkHttpClient sdkHttpClient = this.httpClient != null ? this.httpClient : testSdkHttpClient();
ClientOverrideConfiguration overrideConfiguration =
ClientOverrideConfiguration.builder()
.totalExecutionTimeout(clientExecutionTimeout)
.apply(this::configureRetryPolicy)
.apply(this::configureAdditionalHeaders)
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,12 @@
import java.io.InputStream;
import java.util.Arrays;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.runners.MockitoJUnitRunner;
import software.amazon.awssdk.annotations.ReviewBeforeRelease;
import software.amazon.awssdk.core.exception.AbortedException;
import software.amazon.awssdk.core.exception.SdkClientException;
import software.amazon.awssdk.core.http.AmazonSyncHttpClient;
Expand All @@ -55,6 +57,8 @@
import utils.HttpTestUtils;

@RunWith(MockitoJUnitRunner.class)
@Ignore
@ReviewBeforeRelease("add it back once execution time out is added back")
public class AbortedExceptionClientExecutionTimerIntegrationTest extends MockServerTestBase {

private AmazonSyncHttpClient httpClient;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@

import java.util.Collections;
import org.junit.BeforeClass;
import org.junit.Ignore;
import org.junit.Test;
import software.amazon.awssdk.annotations.ReviewBeforeRelease;
import software.amazon.awssdk.core.TestPreConditions;
import software.amazon.awssdk.core.http.AmazonSyncHttpClient;
import software.amazon.awssdk.core.http.ExecutionContext;
Expand All @@ -36,6 +38,8 @@
/**
* Tests that use a server that returns a predetermined error response within the timeout limit
*/
@Ignore
@ReviewBeforeRelease("add it back once execution time out is added back")
public class DummyErrorResponseServerIntegrationTests extends MockServerTestBase {

private static final int STATUS_CODE = 500;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
import static software.amazon.awssdk.core.internal.http.timers.TimeoutTestConstants.TEST_TIMEOUT;

import java.util.Arrays;
import org.junit.Ignore;
import org.junit.Test;
import software.amazon.awssdk.annotations.ReviewBeforeRelease;
import software.amazon.awssdk.core.http.AmazonSyncHttpClient;
import software.amazon.awssdk.core.http.ExecutionContext;
import software.amazon.awssdk.core.http.MockServerTestBase;
Expand All @@ -31,7 +33,8 @@
import software.amazon.awssdk.core.internal.http.response.DummyResponseHandler;
import software.amazon.awssdk.core.internal.http.response.UnresponsiveResponseHandler;
import utils.HttpTestUtils;

@Ignore
@ReviewBeforeRelease("add it back once execution time out is added back")
public class DummySuccessfulResponseServerIntegrationTests extends MockServerTestBase {

private static final int STATUS_CODE = 200;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,24 @@
import java.net.SocketTimeoutException;
import java.time.Duration;
import org.junit.BeforeClass;
import org.junit.Ignore;
import org.junit.Test;
import software.amazon.awssdk.annotations.ReviewBeforeRelease;
import software.amazon.awssdk.core.TestPreConditions;
import software.amazon.awssdk.core.config.ClientOverrideConfiguration;
import software.amazon.awssdk.core.config.SdkMutableClientConfiguration;
import software.amazon.awssdk.core.exception.SdkClientException;
import software.amazon.awssdk.core.http.AmazonSyncHttpClient;
import software.amazon.awssdk.core.http.UnresponsiveMockServerTestBase;
import software.amazon.awssdk.core.http.exception.ClientExecutionTimeoutException;
import software.amazon.awssdk.core.internal.http.timers.TimeoutTestConstants;
import software.amazon.awssdk.core.retry.FixedTimeBackoffStrategy;
import software.amazon.awssdk.core.retry.RetryPolicy;
import software.amazon.awssdk.http.SdkHttpClient;
import software.amazon.awssdk.http.apache.ApacheSdkHttpClientFactory;
import utils.HttpTestUtils;

@Ignore
@ReviewBeforeRelease("Add the tests back once execution, request timeout are added back")
public class UnresponsiveServerIntegrationTests extends UnresponsiveMockServerTestBase {

private static final Duration LONGER_SOCKET_TIMEOUT =
Expand Down Expand Up @@ -90,7 +93,6 @@ public void interruptCausedBySomethingOtherThanTimer_PropagatesInterruptToCaller

ClientOverrideConfiguration overrideConfiguration =
ClientOverrideConfiguration.builder()
.totalExecutionTimeout(TimeoutTestConstants.CLIENT_EXECUTION_TIMEOUT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are these tests working if we don't have a timeout anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, they failed along with other tests. Fixed them(adding ignore).

Running integ tests now and will wait and see if there are more failing.

.retryPolicy(retryPolicy)
.build();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,11 @@ public abstract class RequestOverrideConfig {

private final Map<String, List<String>> rawQueryParameters;

private final Duration requestExecutionTimeout;

private final List<ApiName> apiNames;

protected RequestOverrideConfig(Builder<?> builder) {
this.headers = builder.headers();
this.rawQueryParameters = builder.rawQueryParameters();
this.requestExecutionTimeout = builder.requestExecutionTimeout();
this.apiNames = builder.apiNames();
}

Expand All @@ -66,15 +63,6 @@ public Optional<Map<String, List<String>>> rawQueryParameters() {
return Optional.ofNullable(rawQueryParameters);
}

/**
* Optional execution timeout for the request.
*
* @return The optional execution timeout.
*/
public Optional<Duration> requestExecutionTimeout() {
return Optional.ofNullable(requestExecutionTimeout);
}

/**
* The optional names of the higher level libraries that constructed the request.
*
Expand Down Expand Up @@ -170,22 +158,6 @@ default B rawQueryParameter(String name, String value) {
*/
B rawQueryParameters(Map<String, List<String>> rawQueryParameters);

/**
* Optional execution timeout for the request.
*
* @return The optional execution timeout.
*/
Duration requestExecutionTimeout();

/**
* Set an optional execution timeout for the request.
*
* @param requestExecutionTimeout The optional execution timeout for the request.
*
* @return This object for method chaining.
*/
B requestExecutionTimeout(Duration requestExecutionTimeout);

/**
* The optional names of the higher level libraries that constructed the request.
*
Expand Down Expand Up @@ -234,7 +206,6 @@ protected BuilderImpl() {
protected BuilderImpl(RequestOverrideConfig sdkRequestOverrideConfig) {
sdkRequestOverrideConfig.headers().ifPresent(this::headers);
sdkRequestOverrideConfig.rawQueryParameters().ifPresent(this::rawQueryParameters);
sdkRequestOverrideConfig.requestExecutionTimeout().ifPresent(this::requestExecutionTimeout);
sdkRequestOverrideConfig.apiNames().ifPresent(apiNames -> apiNames.forEach(this::addApiName));
}

Expand Down Expand Up @@ -288,18 +259,6 @@ public B rawQueryParameters(Map<String, List<String>> rawQueryParameters) {
return (B) this;
}

@Override
public Duration requestExecutionTimeout() {
return requestExecutionTimeout;
}

@Override
@SuppressWarnings("unchecked")
public B requestExecutionTimeout(Duration requestExecutionTimeout) {
this.requestExecutionTimeout = requestExecutionTimeout;
return (B) this;
}

@Override
public List<ApiName> apiNames() {
return apiNames;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,12 @@

package software.amazon.awssdk.core.config;

import java.time.Duration;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.function.Consumer;
import software.amazon.awssdk.annotations.ReviewBeforeRelease;
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
import software.amazon.awssdk.core.retry.RetryPolicy;
Expand All @@ -39,9 +37,7 @@
* <p>Use {@link #builder()} to create a set of options.</p>
*/
public class ClientOverrideConfiguration
implements ToCopyableBuilder<ClientOverrideConfiguration.Builder, ClientOverrideConfiguration> {
private final Duration httpRequestTimeout;
private final Duration totalExecutionTimeout;
implements ToCopyableBuilder<ClientOverrideConfiguration.Builder, ClientOverrideConfiguration> {
private final Map<String, List<String>> additionalHttpHeaders;
private final Boolean gzipEnabled;
private final RetryPolicy retryPolicy;
Expand All @@ -52,8 +48,6 @@ public class ClientOverrideConfiguration
* Initialize this configuration. Private to require use of {@link #builder()}.
*/
private ClientOverrideConfiguration(DefaultClientOverrideConfigurationBuilder builder) {
this.httpRequestTimeout = builder.httpRequestTimeout;
this.totalExecutionTimeout = builder.totalExecutionTimeout;
this.additionalHttpHeaders = CollectionUtils.deepUnmodifiableMap(builder.additionalHttpHeaders);
this.gzipEnabled = builder.gzipEnabled;
this.retryPolicy = builder.retryPolicy;
Expand All @@ -64,8 +58,6 @@ private ClientOverrideConfiguration(DefaultClientOverrideConfigurationBuilder bu
@Override
public Builder toBuilder() {
return new DefaultClientOverrideConfigurationBuilder().advancedOptions(advancedOptions.toBuilder())
.httpRequestTimeout(httpRequestTimeout)
.totalExecutionTimeout(totalExecutionTimeout)
.additionalHttpHeaders(additionalHttpHeaders)
.gzipEnabled(gzipEnabled)
.retryPolicy(retryPolicy)
Expand All @@ -79,49 +71,6 @@ public static Builder builder() {
return new DefaultClientOverrideConfigurationBuilder();
}

/**
* The amount of time to wait for the request to complete before giving up and timing out. An empty value disables this
* feature.
*
* <p>This feature requires buffering the entire response (for non-streaming APIs) into memory to enforce a hard timeout when
* reading the response. For APIs that return large responses this could be expensive.</p>
*
* <p>The request timeout feature doesn't have strict guarantees on how quickly a request is aborted when the timeout is
* breached. The typical case aborts the request within a few milliseconds but there may occasionally be requests that don't
* get aborted until several seconds after the timer has been breached. Because of this, the request timeout feature should
* not be used when absolute precision is needed.</p>
*
* @see Builder#httpRequestTimeout(Duration)
*/
@ReviewBeforeRelease("This doesn't currently work.")
public Duration httpRequestTimeout() {
return httpRequestTimeout;
}

/**
* The amount of time to allow the client to complete the execution of an API call. This timeout covers the entire client
* execution except for marshalling. This includes request handler execution, all HTTP requests including retries,
* unmarshalling, etc. An empty value disables this feature.
*
* <p>This feature requires buffering the entire response (for non-streaming APIs) into memory to enforce a hard timeout when
* reading the response. For APIs that return large responses this could be expensive.</p>
*
* <p>The client execution timeout feature doesn't have strict guarantees on how quickly a request is aborted when the
* timeout
* is breached. The typical case aborts the request within a few milliseconds but there may occasionally be requests that
* don't get aborted until several seconds after the timer has been breached. Because of this, the client execution timeout
* feature should not be used when absolute precision is needed.</p>
*
* <p>This may be used together with {@link #httpRequestTimeout()} to enforce both a timeout on each individual HTTP request
* (i.e. each retry) and the total time spent on all requests across retries (i.e. the 'client execution' time). A
* non-positive value disables this feature.</p>
*
* @see Builder#totalExecutionTimeout(Duration)
*/
public Duration totalExecutionTimeout() {
return totalExecutionTimeout;
}

/**
* An unmodifiable representation of the set of HTTP headers that should be sent with every request. If not set, this will
* return an empty map.
Expand Down Expand Up @@ -173,8 +122,6 @@ public List<ExecutionInterceptor> executionInterceptors() {
@Override
public String toString() {
return ToString.builder("ClientOverrideConfiguration")
.add("httpRequestTimeout", httpRequestTimeout)
.add("totalExecutionTimeout", totalExecutionTimeout)
.add("additionalHttpHeaders", additionalHttpHeaders)
.add("gzipEnabled", gzipEnabled)
.add("retryPolicy", retryPolicy)
Expand All @@ -189,43 +136,6 @@ public String toString() {
* <p>All implementations of this interface are mutable and not thread safe.</p>
*/
public interface Builder extends CopyableBuilder<Builder, ClientOverrideConfiguration> {
/**
* Configure the amount of time to wait for the request to complete before giving up and timing out. A non-positive value
* disables this feature.
*
* <p>This feature requires buffering the entire response (for non-streaming APIs) into memory to enforce a hard timeout
* when reading the response. For APIs that return large responses this could be expensive.</p>
*
* <p>The request timeout feature doesn't have strict guarantees on how quickly a request is aborted when the timeout is
* breached. The typical case aborts the request within a few milliseconds but there may occasionally be requests that
* don't get aborted until several seconds after the timer has been breached. Because of this, the request timeout
* feature
* should not be used when absolute precision is needed.</p>
*
* @see ClientOverrideConfiguration#httpRequestTimeout()
*/
Builder httpRequestTimeout(Duration httpRequestTimeout);

/**
* Configure the amount of time to allow the client to complete the execution of an API call. This timeout covers the
* entire client execution except for marshalling. This includes request handler execution, all HTTP request including
* retries, unmarshalling, etc.
*
* <p>This feature requires buffering the entire response (for non-streaming APIs) into memory to enforce a hard timeout
* when reading the response. For APIs that return large responses this could be expensive.</p>
*
* <p>The client execution timeout feature doesn't have strict guarantees on how quickly a request is aborted when the
* timeout is breached. The typical case aborts the request within a few milliseconds but there may occasionally be
* requests that don't get aborted until several seconds after the timer has been breached. Because of this, the client
* execution timeout feature should not be used when absolute precision is needed.</p>
*
* <p>This may be used together with {@link #httpRequestTimeout()} to enforce both a timeout on each individual HTTP
* request (i.e. each retry) and the total time spent on all requests across retries (i.e. the 'client execution' time).
* A non-positive value disables this feature.</p>
*
* @see ClientOverrideConfiguration#totalExecutionTimeout()
*/
Builder totalExecutionTimeout(Duration totalExecutionTimeout);

/**
* Define a set of headers that should be added to every HTTP request sent to AWS. This will override any headers
Expand Down Expand Up @@ -322,34 +232,12 @@ default Builder retryPolicy(Consumer<RetryPolicy.Builder> retryPolicy) {
* An SDK-internal implementation of {@link ClientOverrideConfiguration.Builder}.
*/
private static final class DefaultClientOverrideConfigurationBuilder implements Builder {
private Duration httpRequestTimeout;
private Duration totalExecutionTimeout;
private Map<String, List<String>> additionalHttpHeaders = new HashMap<>();
private Boolean gzipEnabled;
private RetryPolicy retryPolicy;
private List<ExecutionInterceptor> executionInterceptors = new ArrayList<>();
private AttributeMap.Builder advancedOptions = AttributeMap.builder();

@Override
public Builder httpRequestTimeout(Duration httpRequestTimeout) {
this.httpRequestTimeout = httpRequestTimeout;
return this;
}

public void setHttpRequestTimeout(Duration httpRequestTimeout) {
httpRequestTimeout(httpRequestTimeout);
}

@Override
public Builder totalExecutionTimeout(Duration totalExecutionTimeout) {
this.totalExecutionTimeout = totalExecutionTimeout;
return this;
}

public void setTotalExecutionTimeout(Duration totalExecutionTimeout) {
totalExecutionTimeout(totalExecutionTimeout);
}

@Override
public Builder additionalHttpHeaders(Map<String, List<String>> additionalHttpHeaders) {
this.additionalHttpHeaders = CollectionUtils.deepCopyMap(additionalHttpHeaders);
Expand Down
Loading