Adapt retries to new PhysicalIO #340

fuatbasik · 2025-08-04T10:33:08Z

Description of change

This is adopting Retry logic to the new physical IO. Now, we are merging timeout and retries into a unified place.

With this change we are letting consumers also to pass Timeout setting as a part of Retry strategy. If passed, AAL will not use timeout values from PhysicalIO configuration.

I also removed timeouts from Telemetry as they are no longer relevant. Failsafe now manages retries and timeouts.

Relevant issues

N/A

Does this contribution introduce any breaking changes to the existing APIs or behaviors?

No.

Does this contribution introduce any new public APIs or behaviors?

Yes, Please see description Javadocs are updated accordingly.

How was the contribution tested?

Added new unit tests, and modified existing unit tests.

Does this contribution need a changelog entry?

I have updated the CHANGELOG or README if appropriate

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

ozkoca

LGTM. Added a few minor requests

...src/main/java/software/amazon/s3/analyticsaccelerator/common/telemetry/DefaultTelemetry.java

common/src/main/java/software/amazon/s3/analyticsaccelerator/common/telemetry/Telemetry.java

ozkoca · 2025-08-05T09:34:52Z

common/src/main/java/software/amazon/s3/analyticsaccelerator/util/retry/RetryStrategy.java

+   * @param durationInMillis Timeout duration for reading from storage
+   * @param retryCount Number of times to retry if Timeout Exceeds
+   */
+  void timeout(long durationInMillis, int retryCount);


nit: can be renamed sthg like setTimeoutPolicy()

ahmarsuhail

thanks @fuatbasik really like how simple this is becoming. left a few questions.

Also - Do we already have an ITest for the retry behaviour from a previous PR/or is there another test that covers this behaviour?

It would be good to test the number of test GET requests made when there is a timeout (maybe the gray failure ones cover this?). And also that when timeout is set in openStream information, that overrides the physicalIo configuration (sorry if these tests already exist and I've just missed them!)

ahmarsuhail · 2025-08-06T14:04:50Z

...m/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/impl/PhysicalIOImpl.java

    boolean shouldEvict = false;

-    // Check for IO errors while reading data
-    if (e instanceof IOException


why are we no longer evicting on a tiemout?

retry should retry the timeout blocks. I am not sure if Timeout should be considered as a persistent change (like etag has changed) therefore, evicting already existing blocks sound wrong to me.

ahmarsuhail · 2025-08-06T14:13:49Z

...m/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/reader/StreamReader.java

-              } catch (IOException e) {
-                LOG.error("IOException while reading blocks", e);
-                setErrorOnBlocksAndRemove(blocks, e);
+                      if (objectContent == null) {


not related to your PR, but in what scenario do we expect ObjectContent to be null?

actually we dont. i think this is just and additional check to avoid NPE.

ahmarsuhail · 2025-08-06T14:21:42Z

...m/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/reader/StreamReader.java

+      provided = new DefaultRetryStrategyImpl();
    }

-    if (this.physicalIOConfiguration.getBlockReadTimeout() > 0) {


ok so the major change is that failsafe handles the timeout for us, if processReadTask() takes longer than X seconds for whatever reason, it gets retried. this is so much simpler now, nice :)

ahmarsuhail · 2025-08-06T14:28:25Z

...src/test/java/software/amazon/s3/analyticsaccelerator/io/physical/data/BlockManagerTest.java

    ArgumentCaptor<GetRequest> requestCaptor = ArgumentCaptor.forClass(GetRequest.class);
    verify(objectClient, timeout(1_000).times(3)).getObject(requestCaptor.capture(), any());

    List<GetRequest> getRequestList = requestCaptor.getAllValues();


why did you need to update this test?

in this test we know we will make 3 reads (2 5MB and 1 3MB) but we do not know their order due to async. behaviour. Therefore, updated the tests.

ahmarsuhail · 2025-08-07T10:44:28Z

...src/integrationTest/java/software/amazon/s3/analyticsaccelerator/access/GrayFailureTest.java

+              openStreamInfo);
+
+      // Asserting there is timeout
+      assertThrows(IOException.class, overrideStream::read);


would be good to also assert that the cause of the IoException is Timeout

ahmarsuhail

+1, LGTM

fuatbasik added 4 commits August 1, 2025 13:24

Add retries to new PhysicalIO

fd844e1

Remove timeouts from telemetry

1736b0a

Remove disabled tests

47c7ca2

Update javadocs

7fb58df

fuatbasik had a problem deploying to integration-tests August 4, 2025 10:33 — with GitHub Actions Failure

fuatbasik had a problem deploying to integration-tests August 4, 2025 13:18 — with GitHub Actions Failure

Fix flaky read vectored test

a0adea0

fuatbasik force-pushed the retries branch from 8af0721 to a0adea0 Compare August 4, 2025 13:36

fuatbasik had a problem deploying to integration-tests August 4, 2025 13:36 — with GitHub Actions Failure

Fix static OpenStreamInformation

37d318a

fuatbasik temporarily deployed to integration-tests August 4, 2025 16:17 — with GitHub Actions Inactive

fuatbasik changed the title ~~Adopt retries to new PhysicalIO~~ Adapt retries to new PhysicalIO Aug 4, 2025

fuatbasik mentioned this pull request Aug 5, 2025

AWS: Add Retries to Analytics Stream apache/iceberg#13739

Closed

ozkoca reviewed Aug 5, 2025

View reviewed changes

Address feedback on javadoc and method name

5a192e5

fuatbasik temporarily deployed to integration-tests August 6, 2025 12:35 — with GitHub Actions Inactive

ahmarsuhail reviewed Aug 6, 2025

View reviewed changes

fuatbasik temporarily deployed to integration-tests August 7, 2025 10:36 — with GitHub Actions Inactive

ahmarsuhail reviewed Aug 7, 2025

View reviewed changes

Add new integrationTest to check override

f08c5ea

fuatbasik force-pushed the retries branch from b965330 to f08c5ea Compare August 7, 2025 10:46

fuatbasik temporarily deployed to integration-tests August 7, 2025 10:46 — with GitHub Actions Inactive

ahmarsuhail approved these changes Aug 7, 2025

View reviewed changes

fuatbasik merged commit 30393bd into awslabs:main Aug 7, 2025
4 checks passed

fuatbasik mentioned this pull request Sep 1, 2025

Add Retries for InputStream exceptions on Analytics Stream apache/hadoop#7923

Open

2 tasks

shkhrgpt mentioned this pull request Sep 4, 2025

[Forked] Add Retries for InputStream exceptions on Analytics Stream rais-nspct/hadoop#2

Open

2 tasks

Adapt retries to new PhysicalIO #340

Adapt retries to new PhysicalIO #340

Uh oh!

Conversation

fuatbasik commented Aug 4, 2025

Description of change

Relevant issues

Does this contribution introduce any breaking changes to the existing APIs or behaviors?

Does this contribution introduce any new public APIs or behaviors?

How was the contribution tested?

Does this contribution need a changelog entry?

Uh oh!

ozkoca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmarsuhail left a comment

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!

fuatbasik Aug 7, 2025 • 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!

ahmarsuhail left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fuatbasik Aug 7, 2025 •

edited

Loading