HADOOP-19348. Integrate analytics accelerator into S3A. #7334

ahmarsuhail · 2025-01-28T13:29:34Z

Description of PR

Initial integration of analytics accelerator.

How was this patch tested?

In progress

For code changes:

Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE, LICENSE-binary, NOTICE-binary files?

ahmarsuhail · 2025-02-04T13:47:35Z

Few things to discuss here:

Now that we're using S3A's async client, which already has the execution interceptors attached, a lot of tests fail as out of span operations get rejected. Since we're not support auditing right now, can we recommend that if you're running with AAL turned on, turn off fs.s3a.audit.reject.out.of.span.operations?
The async client from the current SDK version doesn't do ranged GETs if multipartEnabled is enabled on it. For ranged GETs, either upgrade SDK or disable multipartEnabled temporary when AAL is enabled, similar to

hadoop/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/DefaultS3ClientFactory.java

Line 169 in 950b3eb

if (!parameters.isClientSideEncryptionEnabled()) {

ahmarsuhail · 2025-02-04T16:51:08Z

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AS3SeekableStream.java

+  private static final String LOGICAL_IO_PREFIX = "logicalio";
+
+  @Test
+  public void testConnectorFrameWorkIntegration() throws IOException {


small parquet file, src/test/parquet

can we read the file ~10sKB
does it just complete and not complete

malformed footer

mukund-thakur

some old comments about javadoc

...op-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/streams/AnalyticsStream.java

...s/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/streams/AnalyticsStreamFactory.java

mukund-thakur · 2025-02-03T22:38:55Z

...tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/contract/s3a/ITestS3AContractCreate.java

+  public void testOverwriteExistingFile() throws Throwable {
+    // Will remove this when Analytics Accelerator supports overwrites
+    skipIfAnalyticsAcceleratorEnabled(this.createConfiguration(),
+        "Analytics Accelerator does not support overwrites yet");


Analytics Accelerator is about read optimizations right? How does this relate to overwrite?
Is it because the file will be changed? You mean it doesn't support the RemoteFileChangedException?

mukund-thakur · 2025-02-03T22:45:21Z

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3ADelayedFNF.java

@@ -65,6 +66,8 @@ protected Configuration createConfiguration() {
   */
  @Test
  public void testNotFoundFirstRead() throws Exception {
+    skipIfAnalyticsAcceleratorEnabled(getConfiguration(),
+        "Temporarily disabling to fix Exception handling on Analytics Accelerator");


needs to be enabled.

hadoop-yetus · 2025-02-11T16:26:02Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 0s		Docker mode activated.
-1 ❌	patch	0m 20s		#7334 does not apply to trunk. Rebase required? Wrong Branch? See https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute for help.

Subsystem	Report/Notes
GITHUB PR	#7334
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-7334/13/console
versions	git=2.34.1
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

mukund-thakur · 2025-02-11T21:05:23Z

...adoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/streams/StreamFactoryRequirements.java

+import org.apache.hadoop.fs.s3a.VectoredIOContext;
+
+/**
+ * Requirements for requirements for streams from this factory,


java doc correction.

hadoop-yetus · 2025-02-12T14:02:44Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 0s		Docker mode activated.
-1 ❌	patch	0m 20s		#7334 does not apply to trunk. Rebase required? Wrong Branch? See https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute for help.

Subsystem	Report/Notes
GITHUB PR	#7334
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-7334/14/console
versions	git=2.34.1
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

ahmarsuhail · 2025-02-12T16:21:32Z

Commit 99fbdeb means this will no longer build as is, as AAL with the new constructor that lets you pass in file information awslabs/analytics-accelerator-s3#223 must be merged in and released first (WIP!)

To test this currently, set the branch to commit: 038a692

steveloughran · 2025-02-17T15:51:40Z

...op-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/streams/AnalyticsStream.java

+import java.io.EOFException;
+import java.io.IOException;
+
+import org.apache.hadoop.fs.FSExceptionMessages;


nit, imports are out of order

steveloughran · 2025-02-17T15:55:13Z

...s/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/streams/AnalyticsStreamFactory.java

+
+package org.apache.hadoop.fs.s3a.impl.streams;
+
+import org.apache.hadoop.conf.Configuration;


usual nit: import ordering, and I'd prefer an explicit import of those Constants which are being used

steveloughran · 2025-02-17T16:04:29Z

...s/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/streams/AnalyticsStreamFactory.java

+    @Override
+    public void bind(final FactoryBindingParameters factoryBindingParameters) throws IOException {
+        super.bind(factoryBindingParameters);
+        this.s3SeekableInputStreamFactory = new LazyAutoCloseableReference<>(createS3SeekableInputStreamFactory());


can you chop this line down..it's too wide fo side-by-side reviews

steveloughran · 2025-02-17T16:10:32Z

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/RequestFactoryImpl.java

@@ -115,7 +115,7 @@ public class RequestFactoryImpl implements RequestFactory {
  /**
   * Callback to prepare requests.
   */
-  private final PrepareRequest requestPreparer;
+  private PrepareRequest requestPreparer;


this doesn't need to be non-final any more, I shall fix in my PR

steveloughran

+1 pending:

those little nits
my PR in
new release of your library (which I've just been looking at...may need a bit of resilience there, especially to premature -1 calls.

ahmarsuhail · 2025-02-17T16:22:08Z

nice, thanks for the review!

What do you mean by premature -1 calls?

steveloughran · 2025-02-17T18:50:18Z

sometimes a read can return -1 due to network errors, not EOF. in that situation (look at read()) we abort the stream so it doesn't go back into the pool, then ask for a new one. Apparently before the abort() you could get back the same stream again, even through it was now failing. Inevitably, this is a consequence of the stream's long retention of the same connection; if it returned them after 60s this'd be less likely

ahmarsuhail marked this pull request as draft January 28, 2025 13:29

github-actions bot added build trunk TOOLS AWS labels Jan 28, 2025

ahmarsuhail changed the title ~~/HADOOP-19348. Integrate analytics accelerator into S3A.~~ HADOOP-19348. Integrate analytics accelerator into S3A. Jan 28, 2025

ahmarsuhail force-pushed the feature-HADOOP-19363-analytics-accelerator-s3 branch 2 times, most recently from e18d0a4 to d45beae Compare January 31, 2025 14:57

github-actions bot added the Common label Jan 31, 2025

ahmarsuhail commented Feb 4, 2025

View reviewed changes

mukund-thakur reviewed Feb 5, 2025

View reviewed changes

ahmarsuhail force-pushed the feature-HADOOP-19363-analytics-accelerator-s3 branch 2 times, most recently from e18d0a4 to 0d1f291 Compare February 7, 2025 14:57

apache deleted a comment from hadoop-yetus Feb 7, 2025

apache deleted a comment from hadoop-yetus Feb 10, 2025

ahmarsuhail force-pushed the feature-HADOOP-19363-analytics-accelerator-s3 branch from a3c7498 to f408ec5 Compare February 11, 2025 16:23

apache deleted a comment from hadoop-yetus Feb 11, 2025

mukund-thakur reviewed Feb 11, 2025

View reviewed changes

steveloughran approved these changes Feb 17, 2025

View reviewed changes

steveloughran requested changes Feb 17, 2025

View reviewed changes

SanjayMarreddi mentioned this pull request Feb 17, 2025

AWS: Integrate S3 analytics accelerator library apache/iceberg#12299

Draft

ahmarsuhail added 3 commits February 24, 2025 10:44

Integrates AAL into S3A

6e0e567

lazy eval of stream factory + test fixes

3828ad5

pass down file open options such as read policy, file status to AAL

b92a661

ahmarsuhail force-pushed the feature-HADOOP-19363-analytics-accelerator-s3 branch from 99fbdeb to b92a661 Compare February 24, 2025 10:54

review nits

5a2db0a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HADOOP-19348. Integrate analytics accelerator into S3A. #7334

HADOOP-19348. Integrate analytics accelerator into S3A. #7334

ahmarsuhail commented Jan 28, 2025

ahmarsuhail commented Feb 4, 2025

ahmarsuhail Feb 4, 2025

mukund-thakur left a comment

mukund-thakur Feb 3, 2025

mukund-thakur Feb 3, 2025

hadoop-yetus commented Feb 11, 2025

mukund-thakur Feb 11, 2025

hadoop-yetus commented Feb 12, 2025

ahmarsuhail commented Feb 12, 2025

steveloughran Feb 17, 2025

steveloughran Feb 17, 2025

steveloughran Feb 17, 2025

steveloughran Feb 17, 2025

steveloughran left a comment

ahmarsuhail commented Feb 17, 2025

steveloughran commented Feb 17, 2025


		package org.apache.hadoop.fs.s3a.impl.streams;

		import org.apache.hadoop.conf.Configuration;

HADOOP-19348. Integrate analytics accelerator into S3A. #7334

Are you sure you want to change the base?

HADOOP-19348. Integrate analytics accelerator into S3A. #7334

Conversation

ahmarsuhail commented Jan 28, 2025

Description of PR

How was this patch tested?

For code changes:

ahmarsuhail commented Feb 4, 2025

Choose a reason for hiding this comment

mukund-thakur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadoop-yetus commented Feb 11, 2025

Choose a reason for hiding this comment

hadoop-yetus commented Feb 12, 2025

ahmarsuhail commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveloughran left a comment

Choose a reason for hiding this comment

ahmarsuhail commented Feb 17, 2025

steveloughran commented Feb 17, 2025