Skip to content

Implemented Retries functionality #3307

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 6 commits into from
Jul 21, 2022
Merged

Implemented Retries functionality #3307

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
25c20cd
Implemented Retries functionality
Jul 18, 2022
f28b6a8
Incorporated Code Review comments
Jul 20, 2022
3b69c18
Implemented PR Changes-2
Jul 21, 2022
dec0884
Adding CR review files
Jul 21, 2022
802606a
Implemented CR comments -4
Jul 21, 2022
45a97e1
nit comment fixed
Jul 21, 2022
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
3 changes: 1 addition & 2 deletions core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import java.net.URI;
import java.time.Duration;
import software.amazon.awssdk.annotations.SdkPublicApi;
import software.amazon.awssdk.core.retry.RetryPolicy;
import software.amazon.awssdk.http.SdkHttpClient;
import software.amazon.awssdk.imds.internal.DefaultEc2Metadata;
import software.amazon.awssdk.imds.internal.EndpointMode;
Expand Down Expand Up @@ -67,7 +66,7 @@ interface Builder {
* @param retryPolicy The retry policy which includes the number of retry attempts for any failed request.
* @return Returns a reference to this builder
*/
Builder retryPolicy(RetryPolicy retryPolicy);
Builder retryPolicy(Ec2MetadataRetryPolicy retryPolicy);

/**
* Define the endpoint of IMDS.
Expand Down
140 changes: 140 additions & 0 deletions core/imds/src/main/java/software/amazon/awssdk/imds/Ec2MetadataRetryPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

package software.amazon.awssdk.imds;

import java.util.Objects;
import software.amazon.awssdk.annotations.SdkPublicApi;
import software.amazon.awssdk.core.retry.RetryMode;
import software.amazon.awssdk.core.retry.backoff.BackoffStrategy;
import software.amazon.awssdk.utils.builder.CopyableBuilder;
import software.amazon.awssdk.utils.builder.ToCopyableBuilder;

/**
* Interface for specifying a retry policy to use when evaluating whether or not a request should be retried. The
* {@link #builder()}} can be used to construct a retry policy from SDK provided policies or policies that directly implement
* {@link BackoffStrategy} .
*
* When using the {@link #builder()} the SDK will use default values for fields that are not provided. The default number of
* retries and condition is based on the current {@link RetryMode}.
*
* @see BackoffStrategy for a list of SDK provided backoff strategies
*/
@SdkPublicApi
public class Ec2MetadataRetryPolicy implements ToCopyableBuilder<Ec2MetadataRetryPolicy.Builder, Ec2MetadataRetryPolicy> {

private final BackoffStrategy backoffStrategy;
private final Integer numRetries;

private Ec2MetadataRetryPolicy(BuilderImpl builder) {

this.numRetries = builder.numRetries != null ? builder.numRetries : 3;

this.backoffStrategy = builder.backoffStrategy != null ? builder.backoffStrategy :
BackoffStrategy.defaultStrategy(RetryMode.STANDARD);
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
Ec2MetadataRetryPolicy ec2MetadataRetryPolicy = (Ec2MetadataRetryPolicy) o;

if (!Objects.equals(numRetries, ec2MetadataRetryPolicy.numRetries)) {
return false;
}
return Objects.equals(backoffStrategy, ec2MetadataRetryPolicy.backoffStrategy);
}

@Override
public int hashCode() {

Comment on lines +48 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add tests for equals and hashcode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add EqualsVerifier method

int result = numRetries.hashCode();
result = 31 * result + backoffStrategy.hashCode();
return result;
}

public Integer numRetries() {
return numRetries;
}

public BackoffStrategy backoffStrategy() {
return backoffStrategy;
}

public static Builder builder() {
return new BuilderImpl();
}

@Override
public Builder toBuilder() {
return builder().numRetries(numRetries)
.backoffStrategy(backoffStrategy);
}

public interface Builder extends CopyableBuilder<Ec2MetadataRetryPolicy.Builder, Ec2MetadataRetryPolicy> {

/**
* Configure the backoff strategy that should be used for waiting in between retry attempts.
*/
Builder backoffStrategy(BackoffStrategy backoffStrategy);

/**
* Configure the maximum number of times that a single request should be retried, assuming it fails for a retryable error.
*/
Builder numRetries(Integer numRetries);

@Override
Ec2MetadataRetryPolicy build();
}

private static final class BuilderImpl implements Builder {

private Integer numRetries;
private BackoffStrategy backoffStrategy;

private BuilderImpl() {
}

@Override
public Builder numRetries(Integer numRetries) {
this.numRetries = numRetries;
return this;
}

public void setNumRetries(Integer numRetries) {
numRetries(numRetries);
}

@Override
public Builder backoffStrategy(BackoffStrategy backoffStrategy) {
this.backoffStrategy = backoffStrategy;
return this;
}

public void setBackoffStrategy(BackoffStrategy backoffStrategy) {
backoffStrategy(backoffStrategy);
}

@Override
public Ec2MetadataRetryPolicy build() {
return new Ec2MetadataRetryPolicy(this);
}
}
}
92 changes: 55 additions & 37 deletions core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@
import software.amazon.awssdk.annotations.ThreadSafe;
import software.amazon.awssdk.core.exception.SdkClientException;
import software.amazon.awssdk.core.exception.SdkServiceException;
import software.amazon.awssdk.core.retry.RetryPolicy;
import software.amazon.awssdk.core.retry.RetryPolicyContext;
import software.amazon.awssdk.http.AbortableInputStream;
import software.amazon.awssdk.http.HttpExecuteRequest;
import software.amazon.awssdk.http.HttpExecuteResponse;
import software.amazon.awssdk.http.SdkHttpClient;
import software.amazon.awssdk.http.SdkHttpMethod;
import software.amazon.awssdk.http.urlconnection.UrlConnectionHttpClient;
import software.amazon.awssdk.imds.Ec2Metadata;
import software.amazon.awssdk.imds.Ec2MetadataRetryPolicy;
import software.amazon.awssdk.imds.MetadataResponse;
import software.amazon.awssdk.utils.IoUtils;

Expand All @@ -54,7 +55,8 @@ public final class DefaultEc2Metadata implements Ec2Metadata {
private static final RequestMarshaller REQUEST_MARSHALLER = new RequestMarshaller();

private static final EndpointProvider ENDPOINT_PROVIDER = EndpointProvider.builder().build();
private final RetryPolicy retryPolicy;

private final Ec2MetadataRetryPolicy retryPolicy;

private final URI endpoint;

Expand All @@ -68,7 +70,7 @@ public final class DefaultEc2Metadata implements Ec2Metadata {

private DefaultEc2Metadata(DefaultEc2Metadata.Ec2MetadataBuilder builder) {

this.retryPolicy = builder.retryPolicy != null ? builder.retryPolicy : RetryPolicy.builder().build();
this.retryPolicy = builder.retryPolicy != null ? builder.retryPolicy : Ec2MetadataRetryPolicy.builder().build();
this.endpoint = URI.create(ENDPOINT_PROVIDER.resolveEndpoint(builder.endpoint, builder.endpointMode));
this.tokenTtl = builder.tokenTtl != null ? builder.tokenTtl : Duration.ofSeconds(21600);
this.endpointMode = ENDPOINT_PROVIDER.resolveEndpointMode(builder.endpointMode);
Expand Down Expand Up @@ -152,41 +154,57 @@ public String toString() {
public MetadataResponse get(String path) {

MetadataResponse metadataResponse = null;
String data = null;
AbortableInputStream abortableInputStream = null;
try {
String token = getToken();
URI uri = URI.create(endpoint + path);
HttpExecuteRequest httpExecuteRequest = REQUEST_MARSHALLER.createDataRequest(uri, SdkHttpMethod.GET, token,
tokenTtl);
HttpExecuteResponse response = httpClient.prepareRequest(httpExecuteRequest).call();
int statusCode = response.httpResponse().statusCode();
Optional<AbortableInputStream> responseBody = response.responseBody();

if (statusCode == HttpURLConnection.HTTP_OK && responseBody.isPresent()) {
abortableInputStream = responseBody.get();
data = IoUtils.toUtf8String(abortableInputStream);
metadataResponse = new MetadataResponse(data);
} else if (statusCode == HttpURLConnection.HTTP_NOT_FOUND) {
throw SdkServiceException.builder()
.message("The requested metadata at path ( " + path + " ) is not found ").build();
} else if (statusCode == HttpURLConnection.HTTP_OK) {
throw SdkClientException.builder()
.message("Response body empty with Status Code " + statusCode).build();
} else {
throw SdkClientException.builder()
.message("Instance metadata service returned unexpected status code " + statusCode)
.build();
for (int tries = 1 ; tries <= retryPolicy.numRetries() ; tries ++) {
Copy link
Contributor

@zoewangg zoewangg Jul 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The max number of attempts should be retryPolicy.numRetries() + 1, right?
Nit, s/tries/attempts


try {
String token = getToken();
URI uri = URI.create(endpoint + path);
HttpExecuteRequest httpExecuteRequest = REQUEST_MARSHALLER.createDataRequest(uri, SdkHttpMethod.GET, token,
tokenTtl);
HttpExecuteResponse response = httpClient.prepareRequest(httpExecuteRequest).call();
int statusCode = response.httpResponse().statusCode();
Optional<AbortableInputStream> responseBody = response.responseBody();

if (statusCode == HttpURLConnection.HTTP_OK && responseBody.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not use HttpURLConnection.HTTP_OK since the http client is pluggable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used HTTP_OK as it would be more neat. But as per discussion , as the variables are only of HttpURLConnection, will change it to direct status codes.

abortableInputStream = responseBody.get();
String data = IoUtils.toUtf8String(abortableInputStream);
metadataResponse = new MetadataResponse(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we return metadataResponse here to avoid else block?

           if (statusCode == 200 && responseBody.isPresent()) {
               abortableInputStream = responseBody.get();
               String data = IoUtils.toUtf8String(abortableInputStream);
               metadataResponse = new MetadataResponse(data);
              return metadataResponse;
           } 

          handleException(statusCode, path);

} else if (statusCode == HttpURLConnection.HTTP_NOT_FOUND) {
throw SdkServiceException.builder()
.message("The requested metadata at path ( " + path + " ) is not found ").build();
} else if (statusCode == HttpURLConnection.HTTP_OK) {
throw SdkClientException.builder()
.message("Response body empty with Status Code " + statusCode).build();
} else {
throw SdkClientException.builder()
.message("Instance metadata service returned unexpected status code " + statusCode)
.build();
}
} catch (SdkServiceException sd) {
throw SdkServiceException.builder().message(sd.getMessage()).cause(sd).build();
} catch (IOException | SdkClientException io) {

log.warn("Received an IOException {0} ", io);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use SdkLogger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted.

if (tries == 3) {
throw SdkClientException.builder().message("Unable to contact EC2 metadata service.").cause(io).build();
}

Duration backoffTime = retryPolicy.backoffStrategy()
.computeDelayBeforeNextRetry(RetryPolicyContext.builder()
.retriesAttempted(tries - 1)
.build());

try {
Thread.sleep(backoffTime.toMillis());
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think interruptedException is already handled in doRetryBehavior

}
} finally {
IoUtils.closeQuietly(abortableInputStream, log);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you break up this block(line 159 - 206 in case multi-line comment is not working) a little bit? It looks like it's doing retry, sending request and handling response. Ideally, a method should just do one thing.
Per Clean Code,

The first rule of functions is that they should be small.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. Will modularize the code.

} catch (SdkServiceException sd) {
throw SdkServiceException.builder().message(sd.getMessage()).cause(sd).build();
} catch (IOException | SdkClientException io) {
// TODO Retry Logic will be added
log.warn("Received an IOException {0} " , io);
} finally {
IoUtils.closeQuietly(abortableInputStream, log);
}

return metadataResponse;
}

Expand Down Expand Up @@ -223,7 +241,7 @@ private String getToken() throws IOException {

private static final class Ec2MetadataBuilder implements Ec2Metadata.Builder {

private RetryPolicy retryPolicy;
private Ec2MetadataRetryPolicy retryPolicy;

private URI endpoint;

Expand All @@ -238,7 +256,7 @@ private static final class Ec2MetadataBuilder implements Ec2Metadata.Builder {
private Ec2MetadataBuilder() {
}

public void setRetryPolicy(RetryPolicy retryPolicy) {
public void setRetryPolicy(Ec2MetadataRetryPolicy retryPolicy) {
this.retryPolicy = retryPolicy;
}

Expand All @@ -263,7 +281,7 @@ public void setHttpClient(SdkHttpClient httpClient) {
}

@Override
public Builder retryPolicy(RetryPolicy retryPolicy) {
public Builder retryPolicy(Ec2MetadataRetryPolicy retryPolicy) {
this.retryPolicy = retryPolicy;
return this;
}
Expand Down
Loading