Issue 7459: Hadoop can't create S3 client with latest AWS SDK by rtjd6554 · Pull Request #7520 · gchq/sleeper

rtjd6554 · 2026-06-24T10:26:03Z

Make sure you have checked all steps below.

Issue

My PR fully resolves the following issues. I've referenced an issue in the PR title, for example "Issue 1234 - My
Feature". Note that before an issue is finished, you can still make a pull request by raising a separate issue
for your progress.
- Resolves Hadoop can't create S3 client with latest AWS SDK #7459

Tests

My PR adds the following tests based on our test strategy OR does not need testing for this extremely good reason:
- Execution of existing test suite, primary focus has been around the integration tests that utilise s3client within it.
  E.g. DockerInstanceIT, TpchSchemaPartitionedInsertPartitioningEnabledIT or DataFusionCompactionRunnerLocalStackIT

Documentation

In case of new functionality, my PR adds documentation that describes how to use it, or I have linked to a
separate issue for that below.
If I have added new Java code, I have added Javadoc that explains it following our conventions and style.
If I have added or removed any dependencies from the project, I have updated the NOTICES file.

ca61688 · 2026-07-02T07:47:19Z

There's a lot of code added here, I know the ticket says the existing test covers everything for this ticket but are there any unit tests that could be added for this class?

There is a lot of coverage of this already. It's used in all the integration tests for code that uses Hadoop, although that's just against LocalStack.

I think if we run the full performance test suite against this branch before merging that's probably enough?

I think the PR description needs updating to clarify which tests have been run or need to be run.

I think we do need to run the performance test suite against this branch before we can merge it, because we don't know how this affects performance, or if it's working for Spark in a deployed instance.

Thinking about it, the behaviour of applying the Hadoop configuration properties would be good to have unit tests for. Can we do that?

If I could see a direct one-to-one correspondence with the Hadoop equivalent of this class I'd be less worried. As it is, the diff between this and the Hadoop version isn't easily comparable.

Would you prefer to unit test the behaviour or make it a more direct copy of the Hadoop code?

Here's a link to the original thread if you're reading this in the review: #7520 (comment)

Unit tests added

ca61688 · 2026-07-02T07:47:56Z

Overall looks good, one comment about if we can add more tests

patchwork01 · 2026-07-17T11:48:21Z

Thinking about it, the behaviour of applying the Hadoop configuration properties would be good to have unit tests for. Can we do that?

If I could see a direct one-to-one correspondence with the Hadoop equivalent of this class I'd be less worried. As it is, the diff between this and the Hadoop version isn't easily comparable.

Would you prefer to unit test the behaviour or make it a more direct copy of the Hadoop code?

Here's a link to the original thread if you're reading this in the review: #7520 (comment)

patchwork01 · 2026-07-20T08:59:06Z

+import static org.apache.hadoop.fs.s3a.Constants.REQUEST_TIMEOUT;
+import static org.assertj.core.api.Assertions.assertThat;
+
+class HadoopS3ClientFactoryTest {


It looks like most of the behaviour isn't covered by these tests yet?

There's quite a bit of logic in HadoopS3ClientFactory, and right now this is testing it can:

Set fs.s3a.s3.client.factory.impl on a Hadoop configuration

Apply an explicit endpoint override from S3ClientCreationParameters

Apply configuration that's set via AWSClientConfig.createClientConfigBuilder (just the request timeout is tested, which might be okay)

These look good, but it just touches the fringes of the actual behaviour implemented in HadoopS3ClientFactory.

I think we could do with some combination of more tests, and/or a clearer equivalence between ours and the Hadoop implementation. Some of the other comments on this review address the equivalence between our code and the Hadoop implementation. I'd focus on testing the parts where our code is different.

More equivalance gone for

patchwork01 · 2026-07-20T09:14:16Z

+    }
+
+    @Test
+    void shouldApplyRequestTimeoutFromConfiguration() throws Exception {


It looks like this behaviour is delegated to the Hadoop implementation in AWSClientConfig. I think we probably don't need to cover everything that's done via AWSClientConfig.createClientConfigBuilder, having one test like this looks good.

We could clarify the intention of this test to demonstrate that HadoopS3ClientFactory applies configuration to ClientOverrideConfiguration through AWSClientConfig. Maybe just add a comment explaining this?

Comment added.

patchwork01 · 2026-07-20T09:19:03Z

+            clientOverrideConfigBuilder.addMetricPublisher(
+                    new AwsStatisticsCollector(parameters.getMetrics()));
+        }
+


In the Hadoop implementation there's code here to set the retry policy. Should that still be here?

Addressed, added Standard Retry strategy

patchwork01 · 2026-07-20T09:22:12Z

+
+    @Override
+    public S3Client createS3Client(URI uri, S3ClientCreationParameters params) throws IOException {
+        return configureClientBuilder(S3Client.builder(), params, getConf(), uri.getHost())


There's a lot of connection settings that were set on the HTTP client at this point in the Hadoop code, via AWSClientConfig.createHttpClientBuilder in AWSClientConfig.createConnectionSettings.

Those look like they probably matter. Can we replicate that configuration against the new AWS client? It doesn't look like this is resolved in the latest Hadoop, since they haven't updated the AWS SDK yet.

I think the proxy settings don't look like they matter to us, though I could be wrong about that.

This also applies to createS3AsyncClient.

patchwork01 · 2026-07-20T09:34:03Z

+    }
+
+    @Override
+    public S3AsyncClient createS3AsyncClient(final URI uri, final S3ClientCreationParameters parameters) throws IOException {


Please can these public methods be moved above the private methods, as described in our coding conventions: https://github.com/gchq/sleeper/blob/develop/docs/development/conventions.md#ordering-within-a-java-class

This would also match the ordering in the original Hadoop code. In general it would be nice if we can match the ordering of methods from the Hadoop version.

patchwork01 · 2026-07-20T09:39:05Z

+        if (configuredRegion != null && !configuredRegion.isEmpty()) {
+            region = Region.of(configuredRegion);
+        }
+


The Hadoop implementation sets fipsEnabled on the builder here. Should that still be here?

patchwork01 · 2026-07-20T09:40:17Z

+        }
+
+        if (endpoint != null) {
+            boolean endpointEndsWithCentral = endpointStr.endsWith(CENTRAL_ENDPOINT);


The Hadoop implementation has some validation code here with checkArgument. Should that still be here?

patchwork01 · 2026-07-20T09:48:28Z

+    }
+
+    private static Region getS3RegionFromEndpoint(final String endpoint,
+            final boolean endpointEndsWithCentral) {


It looks like the logging has been stripped out of this method. Should that still be here?

Logging was removed as part of simplification as little value would be gained.

patchwork01 · 2026-07-20T09:49:52Z

+            }
+            return AwsHostNameUtils.parseSigningRegion(endpoint, "s3").orElse(null);
+        }
+        return Region.EU_WEST_2;


The use of this as the default region is duplicated between here and configureEndpointAndRegion. Please can we set this in one place?

In general, using eu-west-2 as the default region could cause problems for us because things could continue to work if the region is defaulted, just because it happens to match what we're using. This could hide real failures. It might be worth keeping Hadoop's default region instead, or failing completely if it would default?

Addressed, defaulting back to us-east-2

patchwork01 · 2026-07-20T09:53:26Z

+        final ClientOverrideConfiguration.Builder clientOverrideConfigBuilder = AWSClientConfig.createClientConfigBuilder(conf, AWS_SERVICE_IDENTIFIER_S3);
+
+        parameters.getHeaders().forEach((h, v) -> clientOverrideConfigBuilder.putHeader(h, v));
+


In the Hadoop implementation there's code here to set the requester pays header. Should that still be here?

rtjd6554 and others added 21 commits June 24, 2026 06:52

7459: Update sdk version

5e57abe

7459: Implmentation of abstract methods and S3Factory

0b7a429

Merge branch 'develop' into 7459-Hadoop-S3-Client

d080d1c

7459: Improve class name

963357e

7459: Relocate new class

ffd686c

7459: Update imports

fc35926

7459: Inclusion in configuration provider

d3bd3a2

Merge branch 'develop' into 7459-Hadoop-S3-Client

8f49c79

Merge branch 'develop' into 7459-Hadoop-S3-Client

5cb1680

Merge branch 'develop' into 7459-Hadoop-S3-Client

12022c9

7389: Hard code value for build test

e6c19f0

Merge branch 'develop' into 7459-Hadoop-S3-Client

308d30f

Merge branch 'develop' into 7459-Hadoop-S3-Client

e2b473b

Update version number

f6a6183

re-add missing section

978ecdf

7459: Adjust region setting

67606ef

7459: Adjust implementation

9568ef5

Merge branch 'develop' into 7459-Hadoop-S3-Client

41ad961

7459: Tidy up code

0dc7756

7459: Comment out for test simplify

5a57f93

Re-enable functions

f773b97

rtjd6554 added the needs-reviewer Pull requests that need a reviewer to be assigned label Jul 1, 2026

rtjd6554 marked this pull request as ready for review July 1, 2026 12:01

Update aws-java-sdk-v2.version

1215a87

ca61688 self-assigned this Jul 2, 2026

ca61688 reviewed Jul 2, 2026

View reviewed changes

ca61688 removed their assignment Jul 2, 2026

patchwork01 requested changes Jul 2, 2026

View reviewed changes

Comment thread java/common/parquet/src/main/java/sleeper/parquet/HadoopS3ClientFactory.java

Comment thread java/common/parquet/src/main/java/sleeper/parquet/HadoopS3ClientFactory.java

Comment thread java/common/parquet/src/main/java/sleeper/parquet/HadoopS3ClientFactory.java

patchwork01 assigned rtjd6554 Jul 2, 2026

rtjd6554 and others added 5 commits July 10, 2026 07:49

Merge branch 'develop' into 7459-Hadoop-S3-Client

d77b1b3

Merge branch 'develop' into 7459-Hadoop-S3-Client

b473f44

Update version number to latest

c71b34d

Merge branch 'develop' into 7459-Hadoop-S3-Client

2c977f5

Merge branch 'develop' into 7459-Hadoop-S3-Client

a49db7e

patchwork01 reviewed Jul 17, 2026

View reviewed changes

rtjd6554 and others added 4 commits July 17, 2026 14:01

Merge branch 'develop' into 7459-Hadoop-S3-Client

3e59a01

7459: Adjust static modifier for methods

b99cd3f

7459: Private access modifier

ea20c5c

7459: Add unit tests for HadoopS3ClientFactory

0103752

rtjd6554 removed their assignment Jul 20, 2026

Merge branch 'develop' into 7459-Hadoop-S3-Client

c6d4f2e

rtjd6554 added the needs-reviewer Pull requests that need a reviewer to be assigned label Jul 20, 2026

ca61688 self-assigned this Jul 20, 2026

ca61688 approved these changes Jul 20, 2026

View reviewed changes

ca61688 removed their assignment Jul 20, 2026

patchwork01 requested changes Jul 20, 2026

View reviewed changes

patchwork01 removed the needs-reviewer Pull requests that need a reviewer to be assigned label Jul 20, 2026

patchwork01 assigned rtjd6554 Jul 20, 2026

rtjd6554 added 5 commits July 20, 2026 10:18

Review comments addressed

f7ce8ca

7459: Update to add default region

115272a

7459: Change default region

9e5a90b

Update to latest version of sdkv2

fc36067

Re-add missing elements from review

11a35aa

rtjd6554 mentioned this pull request Jul 22, 2026

Bump aws-java-sdk-v2.version from 2.45.1 to 2.49.0 in /java #7724

Closed

rtjd6554 and others added 5 commits July 22, 2026 07:33

Merge branch 'develop' into 7459-Hadoop-S3-Client

1339eb6

Update to latest version of library

7c385a0

Merge branch 'develop' into 7459-Hadoop-S3-Client

f8aa186

7459: Add retry strategy

da562bc

Merge branch 'develop' into 7459-Hadoop-S3-Client

afbd3fb

		final ClientOverrideConfiguration.Builder clientOverrideConfigBuilder = AWSClientConfig.createClientConfigBuilder(conf, AWS_SERVICE_IDENTIFIER_S3);

		parameters.getHeaders().forEach((h, v) -> clientOverrideConfigBuilder.putHeader(h, v));

Uh oh!

Conversation

rtjd6554 commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Tests

Documentation

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!

patchwork01 Jul 17, 2026 • 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!

ca61688 commented Jul 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patchwork01 Jul 17, 2026 • 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!

patchwork01 Jul 20, 2026 • 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!

patchwork01 Jul 20, 2026 • 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!

patchwork01 Jul 20, 2026 • 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!

patchwork01 Jul 20, 2026 • 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

rtjd6554 commented Jun 24, 2026 •

edited

Loading

patchwork01 Jul 17, 2026 •

edited

Loading

patchwork01 Jul 17, 2026 •

edited

Loading

patchwork01 Jul 20, 2026 •

edited

Loading

patchwork01 Jul 20, 2026 •

edited

Loading

patchwork01 Jul 20, 2026 •

edited

Loading

patchwork01 Jul 20, 2026 •

edited

Loading