Add request coalescing and relevant configuration by fuatbasik · Pull Request #356 · awslabs/analytics-accelerator-s3

fuatbasik · 2025-09-29T11:01:05Z

Description of change

With this change, AAL can coalesce requests in a close proximity to each other.

This enables to take spatial locality
into account and reduce request count. In my testing I observed that this is particularly important for vectored reads
reading vectors near each other.

There are two configurations to manage this feature. First one is to enable disable request coalescing:
request.coalesce which is by default set to true.

Second one is request.coalesce.tolerance, which is the number of bytes to tolerate (read even if not immediately needed) when merging ranges.

Two ranges will be merged if end of the first range is less than tolerate bytes away from the second one.

Relevant issues

N/A

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

No. It changes the behaviour of vectored reads and merges smaller requests to bigger requests.
These bigger requests might be split to smaller requests later to make sure all requests are around target.request.size

Does this contribution introduce any new public APIs or behaviors?

How was the contribution tested?

Added new unit tests. Extended existing IntegrationTests to run for both coalescing enabled and disabled.

Does this contribution need a changelog entry?

Yes, i shall be updating the README after we finalise the behaviour part of this PR.

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).

stubz151

ty this awesome, just some smallish comments.

stubz151 · 2025-09-29T11:11:42Z

common/src/main/java/software/amazon/s3/analyticsaccelerator/common/ObjectRange.java

+
+  @Override
+  public String toString() {
+    return String.format("offset: %d, length: %d", offset, length);


what do u think about adding some info about the future as well? like is there any id? or can even add it's current state? it might come in handy down the line.

Thanks, i am now adding future's to-string here

stubz151 · 2025-09-29T11:19:06Z

...src/test/java/software/amazon/s3/analyticsaccelerator/common/ConnectorConfigurationTest.java

@@ -264,6 +264,45 @@ void testGetRequiredStringThrowsIfNotSet() {
        IllegalArgumentException.class, () -> configuration.getRequiredString("stringConfig1"));
  }



Can we squash some of these tests down using parameterized tests?

stubz151 · 2025-09-29T11:26:08Z

...c/main/java/software/amazon/s3/analyticsaccelerator/io/physical/PhysicalIOConfiguration.java

+  /** Flag to enable request Coalescing */
+  @Builder.Default private boolean requestCoalesce = DEFAULT_COALESCE_REQUEST;
+
+  private static final String REQUEST_COALESCE_KEY = "request.coalesce";


why add it as a config? when would we not want to do this?

stubz151 · 2025-09-29T11:56:51Z

input-stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/plan/IOPlan.java

+        currentRange = nextRange;
+      }
+    }
+    coalescedRanges.add(currentRange);


public void coalesce(long tolerance) { if (this.prefetchRanges.size() < 2) { return; } // Ensure ranges are ordered by their start position Collections.sort(this.prefetchRanges); int writeIndex = 0; Range currentRange = this.prefetchRanges.get(0); for (int i = 1; i < this.prefetchRanges.size(); i++) { Range nextRange = this.prefetchRanges.get(i); if (currentRange.getEnd() + tolerance >= nextRange.getStart()) { // Merge ranges currentRange = new Range(currentRange.getStart(), Math.max(currentRange.getEnd(), nextRange.getEnd())); } else { // Store the completed merged range this.prefetchRanges.set(writeIndex++, currentRange); currentRange = nextRange; } } this.prefetchRanges.set(writeIndex++, currentRange); //test this a lil. while (this.prefetchRanges.size() > writeIndex) { this.prefetchRanges.remove(this.prefetchRanges.size() - 1); } }

What do you think of this? you don't need a second list to keep track of it then which I like. I also find it a bit easier to follow.

Thanks. I updated this part in rev2

ahmarsuhail

Looks good, just think we need to add some additional logic in that execute function

ahmarsuhail · 2025-10-01T11:51:10Z

...rc/integrationTest/java/software/amazon/s3/analyticsaccelerator/access/ReadVectoredTest.java

@@ -391,12 +404,12 @@ protected void testReadVectoredInSingleBlock(
  protected void testReadVectoredForSequentialRanges(


i see that this tests range coalescing, but it would be good to add a range coalescing test as well, which will test ranges are within that 1MB distance. eg: [0-4MB, 4.3MB - 5MB, 5.9MB - 7MB].

Thanks @ahmarsuhail. Do test under IOPlanTest cover your concern. I was trying to put corner cases for actual coalesing business logic there instead of here.

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

input-stream/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/plan/IOPlan.java

ozkoca

LGTM

ozkoca · 2025-10-07T16:12:47Z

...c/main/java/software/amazon/s3/analyticsaccelerator/io/physical/PhysicalIOConfiguration.java

+  /** Flag to enable request Coalescing */
+  @Builder.Default private boolean requestCoalesce = DEFAULT_COALESCE_REQUEST;
+
+  private static final String REQUEST_COALESCE_KEY = "request.coalesce";


nit: We may rename like request.coalesce.enabled

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

ahmarsuhail

LGTM

common/src/main/java/software/amazon/s3/analyticsaccelerator/request/ReadMode.java

ahmarsuhail

typo in the coalesce method, using the wrong variable

ahmarsuhail

+1, LGTM

### What changes were proposed in this pull request? This PR aims to upgrade `analyticsaccelerator-s3` to 1.3.1 for Apache Spark 4.2.0 in line with Apache Hadoop 3.4.3 (HADOOP-19742). - apache/hadoop#8093 ### Why are the changes needed? To bring the latest fixes. - https://github.com/awslabs/analytics-accelerator-s3/releases/tag/v1.3.1 - awslabs/analytics-accelerator-s3#360 - awslabs/analytics-accelerator-s3#361 - awslabs/analytics-accelerator-s3#363 - awslabs/analytics-accelerator-s3#356 - awslabs/analytics-accelerator-s3#358 ### Does this PR introduce _any_ user-facing change? No behavior change. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #54031 from dongjoon-hyun/SPARK-55254. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

Add request coalescing and relevant configuration

e887998

fuatbasik temporarily deployed to integration-tests September 29, 2025 11:01 — with GitHub Actions Inactive

fuatbasik requested review from ahmarsuhail and stubz151 September 29, 2025 11:01

stubz151 reviewed Sep 29, 2025

View reviewed changes

ahmarsuhail requested changes Oct 1, 2025

View reviewed changes

fuatbasik had a problem deploying to integration-tests October 7, 2025 15:06 — with GitHub Actions Failure

address reviewer comments

c6960a7

fuatbasik force-pushed the add-request-coalescing branch from c6cf086 to c6960a7 Compare October 7, 2025 15:24

fuatbasik temporarily deployed to integration-tests October 7, 2025 15:24 — with GitHub Actions Inactive

ozkoca previously approved these changes Oct 7, 2025

View reviewed changes

ahmarsuhail reviewed Oct 8, 2025

View reviewed changes

...m/src/main/java/software/amazon/s3/analyticsaccelerator/io/physical/impl/PhysicalIOImpl.java Outdated Show resolved Hide resolved

ahmarsuhail previously approved these changes Oct 8, 2025

View reviewed changes

fuatbasik dismissed stale reviews from ahmarsuhail and ozkoca via d3fa0ba October 8, 2025 15:41

fuatbasik had a problem deploying to integration-tests October 8, 2025 15:41 — with GitHub Actions Failure

Move coalesce decision to ReadMode

19c1be7

fuatbasik force-pushed the add-request-coalescing branch from d3fa0ba to 19c1be7 Compare October 8, 2025 15:50

fuatbasik had a problem deploying to integration-tests October 8, 2025 15:50 — with GitHub Actions Failure

ahmarsuhail reviewed Oct 8, 2025

View reviewed changes

common/src/main/java/software/amazon/s3/analyticsaccelerator/request/ReadMode.java Outdated Show resolved Hide resolved

ahmarsuhail reviewed Oct 8, 2025

View reviewed changes

common/src/main/java/software/amazon/s3/analyticsaccelerator/request/ReadMode.java Outdated Show resolved Hide resolved

ahmarsuhail requested changes Oct 8, 2025

View reviewed changes

Fix using wrong variable

6404903

fuatbasik temporarily deployed to integration-tests October 8, 2025 18:54 — with GitHub Actions Inactive

ahmarsuhail approved these changes Oct 8, 2025

View reviewed changes

fuatbasik merged commit e2b2de6 into awslabs:main Oct 9, 2025
4 checks passed

dongjoon-hyun mentioned this pull request Jan 28, 2026

[SPARK-55254][BUILD] Upgrade analyticsaccelerator-s3 to 1.3.1 apache/spark#54031

Closed

		@@ -264,6 +264,45 @@ void testGetRequiredStringThrowsIfNotSet() {
		IllegalArgumentException.class, () -> configuration.getRequiredString("stringConfig1"));
		}

		@@ -391,12 +404,12 @@ protected void testReadVectoredInSingleBlock(
		protected void testReadVectoredForSequentialRanges(

Conversation

fuatbasik commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

stubz151 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!

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!

Uh oh!

Uh oh!

ozkoca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahmarsuhail left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ahmarsuhail left a comment

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!

4 participants

fuatbasik commented Sep 29, 2025 •

edited

Loading