Implemented Retries functionality #3307

srsaikumarreddy · 2022-07-18T05:41:17Z

Implemented Retries functionality and wrote test cases.

Motivation and Context

Implemented Retries functionality and wrote test cases.

Modifications

Implemented Retries functionality and wrote test cases. Wrote new class for Retry Policy.

Testing

Wrote test cases and ran in IDE.

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

[X ] I have read the CONTRIBUTING document
[ X] Local run of mvn install succeeds
[ X] My code follows the code style of this project
[X ] My change requires a change to the Javadoc documentation
[ X] I have updated the Javadoc documentation accordingly
[ X] I have added tests to cover my changes
[ X] All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

[X ] I confirm that this pull request can be released under the Apache 2 license

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2MetadataRetryPolicy.java

zoewangg · 2022-07-18T22:02:56Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2MetadataRetryPolicy.java

+    @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() {
+


Let's add tests for equals and hashcode

Will add EqualsVerifier method

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2MetadataRetryPolicy.java

zoewangg · 2022-07-18T22:21:47Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

+        for (int tries = 1 ; tries <= retryPolicy.numRetries() ; tries ++) {
+
+            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();
+                    String 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();
+                }
+            } catch (SdkServiceException sd) {
+                throw SdkServiceException.builder().message(sd.getMessage()).cause(sd).build();
+            } catch (IOException | SdkClientException io) {
+
+                log.warn("Received an IOException {0} ", io);
+                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();
+                }
+            } finally {
+                IoUtils.closeQuietly(abortableInputStream, log);
            }


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.

Noted. Will modularize the code.

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

zoewangg · 2022-07-18T22:26:50Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

+                throw SdkServiceException.builder().message(sd.getMessage()).cause(sd).build();
+            } catch (IOException | SdkClientException io) {
+
+                log.warn("Received an IOException {0} ", io);


Can we use SdkLogger?

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

zoewangg · 2022-07-18T22:30:37Z

core/imds/src/test/java/software/amazon/awssdk/imds/Ec2MetadataTest.java

+        thrown.expect(SdkClientException.class);
+        thrown.expectMessage("Unable to contact EC2 metadata service.");


I think thrown.expect is deprecated. Can we use assertThatThrownBy? You can search it in this project to find usage.

zoewangg · 2022-07-18T22:35:19Z

core/imds/src/test/java/software/amazon/awssdk/imds/Ec2MetadataTest.java

@@ -196,4 +208,192 @@ public void getAmiId_onTokenResource_200() throws IOException {
        WireMock.verify(getRequestedFor(urlPathEqualTo(AMI_ID_RESOURCE)).withHeader(TOKEN_HEADER, equalTo("some-token")));
    }

+    @Test
+    public void get_AmiId_on_2nd_try_getToken() throws IOException {


Nit: the naming pattern for unit test code should be method_when_expectedBehavior, eg: getAmiId_failedOnce_shouldSuceedOnSecondAttempt

zoewangg · 2022-07-20T19:50:04Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2MetadataRetryPolicy.java

+/**
+ * Interface for specifying a retry policy to use when evaluating whether or not a request should be retried , and the gap
+ * between each retry. The {@link #builder()} can be used to construct a retry policy with numRetries and backoffStrategy.
+ *<p></p>


https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#styleguide

If you have more than one paragraph in the doc comment, separate the paragraphs with a
paragraph tag, as shown.

</p> should be removed

zoewangg · 2022-07-20T19:51:41Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

-                throw SdkClientException.builder()
-                                        .message("Instance metadata service returned unexpected status code " + statusCode)
-                                        .build();
+        for (int tries = 1 ; tries <= retryPolicy.numRetries() ; tries ++) {


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

zoewangg · 2022-07-20T19:54:33Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

-                throw SdkClientException.builder()
-                                        .message("Instance metadata service returned unexpected status code " + statusCode)
-                                        .build();
+                doTokenExceptionHandling(statusCode);


nit, handleErrorResponse(statusCode)?

zoewangg · 2022-07-20T19:55:24Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

+            throw SdkClientException.builder()
+                                    .message("Response body empty with Status Code " + statusCode).build();


Would this ever happen?

zoewangg · 2022-07-20T19:59:06Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2MetadataRetryPolicy.java

+     * Method to return the number of retries allowed.
+     * @return The number of retries allowed.
+     */
+


nit, empty line

zoewangg · 2022-07-20T20:02:56Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

+                try {
+                    doRetryBehavior(tries, io);
+                } catch (InterruptedException e) {
+                    Thread.currentThread().interrupt();


I think interruptedException is already handled in doRetryBehavior

zoewangg · 2022-07-20T20:03:16Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

+
+    }
+
+    private void doRetryBehavior(int tries , Exception io) throws InterruptedException {


nit, pauseBeforeRetryIfNeeded?
throws InterruptedException can be removed

zoewangg · 2022-07-20T20:04:57Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

+        SdkStandardLogger.REQUEST_LOGGER.warn(() -> "Received an IOException {0} ", io);
+
+        if (tries == retryPolicy.numRetries()) {
+            throw SdkClientException.builder().message("Unable to contact EC2 metadata service.").cause(io).build();


Let's add more details such as "exceeded max number of retries"

zoewangg · 2022-07-20T20:08:04Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

        return metadataResponse;
    }

+    private void doDataExceptionBehavior(int statusCode, String path) {


nit: handleErrorResponse?

Instead of using exception as control flow, can we only throw exceptions for non-retryable cases? This way, the retryable cases will just get retried in the for loop

zoewangg · 2022-07-20T20:13:00Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

        return metadataResponse;
    }

+    private void doDataExceptionBehavior(int statusCode, String path) {
+        if (statusCode == 404) {
+            throw SdkServiceException.builder()


I think it may be worth introducing IMDS specific exceptions, something like ImdsException. I understand this may be out of scope, but could you add a TODO somewhere

zoewangg · 2022-07-21T16:50:12Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2MetadataRetryPolicy.java

+ * between each retry. The {@link #builder()} can be used to construct a retry policy with numRetries and backoffStrategy.
+ * When using the {@link #builder()} the SDK will use default values for fields that are not provided.A custom BackoffStrategy
+ * can be used to construct a policy or a default {@link BackoffStrategy} is used .
+ * <p></p>


To clarify, we need to have <p> but not </p>. Please check out https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#styleguide

zoewangg · 2022-07-21T16:51:33Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

+            } catch (SdkClientException sd) {
+                throw SdkClientException.builder().message(sd.getMessage()).cause(sd).build();


We should not catch and re-throw the same exception. No catch is needed for SdkClientException

zoewangg · 2022-07-21T16:55:45Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

+            } catch (SdkClientException sd) {
+                throw SdkClientException.builder().message(sd.getMessage()).cause(sd).build();
+            } catch (IOException io) {
+                SdkStandardLogger.REQUEST_LOGGER.warn(() -> "Received an IOException {0} ", io);


I meant using this Logger https://github.com/aws/aws-sdk-java-v2/blob/master/utils/src/main/java/software/amazon/awssdk/utils/Logger.java instead of LoggerFactory, not the sdk request logger

zoewangg · 2022-07-21T16:56:41Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

+                    }
+                    handleException(statusCode, path);
+                }
+                pauseBeforeRetryIfNeeded(tries);


It may be cleaner if we move pauseBeforeRetryIfNeeded(tries); to after finally block so that we can remove pauseBeforeRetryIfNeeded(tries); on line 186

zoewangg · 2022-07-21T18:38:15Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2Metadata.java

@@ -253,10 +247,10 @@ private Optional<String> getToken() throws IOException {
            }
            handleErrorResponse(statusCode);
        } catch (IOException e) {
-            SdkStandardLogger.REQUEST_LOGGER.warn(() -> "Received an IOException {0} ", e);
+            log.warn(() -> "Received an IOException {0} ", e);


nit: {0} should be removed, same as the other log statement

sonarqubecloud · 2022-07-21T20:39:19Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
16 Code Smells

30.5% Coverage
2.8% Duplication

…ac935f741 Pull request: release <- staging/ffbc8125-0078-4c6b-a206-909ac935f741

Implemented Retries functionality

25c20cd

srsaikumarreddy requested a review from a team as a code owner July 18, 2022 05:41

zoewangg reviewed Jul 18, 2022

View reviewed changes

Incorporated Code Review comments

f28b6a8

zoewangg reviewed Jul 20, 2022

View reviewed changes

Sai Chandupatla added 2 commits July 20, 2022 18:57

Implemented PR Changes-2

3b69c18

Adding CR review files

dec0884

zoewangg reviewed Jul 21, 2022

View reviewed changes

Implemented CR comments -4

802606a

zoewangg approved these changes Jul 21, 2022

View reviewed changes

nit comment fixed

45a97e1

zoewangg enabled auto-merge (squash) July 21, 2022 19:41

zoewangg merged commit 092a580 into aws:feature/master/imds Jul 21, 2022

aws-sdk-java-automation added a commit that referenced this pull request Oct 3, 2024

Merge pull request #3307 from aws/staging/ffbc8125-0078-4c6b-a206-909…

79e8160

…ac935f741 Pull request: release <- staging/ffbc8125-0078-4c6b-a206-909ac935f741

		thrown.expect(SdkClientException.class);
		thrown.expectMessage("Unable to contact EC2 metadata service.");

		throw SdkClientException.builder()
		.message("Response body empty with Status Code " + statusCode).build();


		}

		private void doRetryBehavior(int tries , Exception io) throws InterruptedException {

		} catch (SdkClientException sd) {
		throw SdkClientException.builder().message(sd.getMessage()).cause(sd).build();

Implemented Retries functionality #3307

Implemented Retries functionality #3307

Uh oh!

Conversation

srsaikumarreddy commented Jul 18, 2022

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoewangg Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jul 21, 2022

Uh oh!

Uh oh!

zoewangg Jul 20, 2022 •

edited

Loading