[FLINK-39115][s3] Support URI Handling Utility by Samrat002 · Pull Request #29127 · apache/flink

Samrat002 · 2026-09-08T06:38:48Z

What is the purpose of the change

Add S3URIUtils and remove duplicated helpers.

Brief change log

Introduce S3URIUtils.

Verifying this change

Exisiting UT and IT on Seaweedfs
Running WordCount job with s3 as statebacked for snapshot ,
Write output to s3 using flink job

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no) no
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no) no
The serializers: (yes / no / don't know) no
The runtime per-record code paths (performance sensitive): (yes / no / don't know) no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know) no
The S3 file system connector: (yes / no / don't know) yes

Documentation

Does this pull request introduce a new feature? (yes / no) no
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

Was generative AI tooling used to co-author this PR?

Yes (please specify the tool below)

Generated-by: [Tool Name and Version]

flinkbot · 2026-09-08T07:00:46Z

CI report:

78b073c Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

Samrat002 · 2026-09-08T10:35:56Z

@flinkbot run azure

spuru9

LGTM. One optional nit.

spuru9 · 2026-09-08T10:44:35Z

+    }
+
+    public static String extractKey(String s3Uri) {
+        String uri = s3Uri.replaceFirst(S3A, S3);


nit: now that extractKey(Path) and extractKey(String) live side by side in the same class, they still use two completely different parsing strategies (one via Path/URI, the other via manual indexOf/substring). Was consolidating them into one implementation considered, or is keeping them separate intentional?

thanks , made changes

gaborgsomogyi · 2026-09-08T10:52:49Z

- *
- * <p><b>S3 URI Handling:</b> The {@link #extractKey(Path)} and {@link #extractBucketName(Path)}
- * methods expect URIs in the standard {@code s3://bucket/key} format. Other formats like path-style
- * ({@code https://s3.amazonaws.com/bucket/key}) or virtual-hosted-style ({@code
- * https://bucket.s3.amazonaws.com/key}) are not currently supported.


Now we're dropping the format expectation and we don't have any enforcement in code too. Maybe we should add and blow up early.

Though I guess requireSupportedScheme will catch these.

Izeren · 2026-09-08T14:26:45Z

+    }
+
+    @ParameterizedTest
+    @CsvSource({"file:///tmp/foo", "hdfs://bucket/key", "gs://bucket/key"})


Should we also have no schema option test?

Izeren · 2026-09-08T14:28:38Z

+    @ParameterizedTest
+    @CsvSource({
+        "s3://bucket",
+        "s3://bucket/",


what would happen for s3://bucket//?

Izeren

Thank you @Samrat002, LGTM assuming that CI is green and feedback about indexof checks is addressed

Samrat002 · 2026-09-11T10:57:40Z

@flinkbot run azure

Samrat002 · 2026-09-11T13:32:59Z

CI is failing due to https://issues.apache.org/jira/browse/FLINK-40635.
this failure is unrelated to the code changes in current PR.

gaborgsomogyi · 2026-09-11T13:41:28Z

Is this flaky? We're not in a rush

Samrat002 · 2026-09-11T14:20:56Z

@flinkbot run azure

Samrat002 · 2026-09-15T11:40:17Z

@flinkbot run azure

davidradl

My AI says (with slight amendments from me) :

Behaviour change on extractKey(Path) — now throws instead of silently accepting
I suggest this is a change in behaviour - I suggest calling this out in the release notes on back ports.
extractBucket(String) vs extractBucketName(Path) — naming inconsistency
The two string-based methods are extractBucket(String) and extractKey(String). The two Path-based methods are extractBucketName(Path) and extractKey(Path). The inconsistency (extractBucket vs extractBucketName) is a small but unnecessary cognitive bump. extractBucketName is arguably more explicit, but consistency across overloads would be cleaner. This was noted by a reviewer as a nit.
extractKey(String) with s3://bucket/ returns "" but test case says it should
Looking at testExtractKeyReturnsEmptyForBucketOnly:

"s3://bucket/", // expects empty

In requireSupportedScheme, s3://bucket/ doesn't start with s3a://, so stays as-is. S3.length() = 5, uri.indexOf('/', 5) finds the / at index 11 (s3://bucket/), so keyStart = 11, returns uri.substring(12) = "".

CI is failing — unrelated flaky test (FLINK-40635)
The author confirmed this. Reviewer gaborgsomogyi questioned whether it's flaky; the author re-triggered CI but the failure persists on the latest push. This should be resolved before merge to confirm it's truly unrelated and not masking a real regression.
Minor: extractKey(String) javadoc is missing
extractBucket(String) has a Javadoc with @throws. extractKey(String) has no Javadoc at all, even though it can also throw IllegalArgumentException for unsupported schemes. Inconsistent documentation.

Samrat002 · 2026-09-15T15:24:58Z

Thank you @davidradl.

Please find the inline reply

Behaviour change on extractKey(Path) — now throws instead of silently accepting
I suggest this is a change in behaviour - I suggest calling this out in the release notes on backports.

native-s3-fs has been released as experimental in flink-2.3. Since then, there have been huge changes it has gone through. The entry-point classes NativeS3FileSystemFactory, NativeS3AFileSystemFactory, and NativeS3RecoverableWriter are @Experimental and S3UriUtils is @Internal, so there's no released behaviour to change and nothing to back-port. Happy to add a note if the module is ever back-ported.

extractBucket(String) vs extractBucketName(Path) — naming inconsistency
The two string-based methods are extractBucket(String) and extractKey(String). The two Path-based methods are extractBucketName(Path) and extractKey(Path). The inconsistency (extractBucket vs extractBucketName) is a small but unnecessary cognitive bump. extractBucketName is arguably more explicit, but consistency across overloads would be cleaner. This was noted by a reviewer as a nit.

I have made changes to address the concern and renamed the method to extractBucketName(String)

extractKey(String) with s3://bucket/ returns "" but test case says it should
Looking at testExtractKeyReturnsEmptyForBucketOnly:
"s3://bucket/", // expects empty
In requireSupportedScheme, s3://bucket/ doesn't start with s3a://, so stays as-is. S3.length() = 5, uri.indexOf('/', 5) finds the / at index 11 (s3://bucket/), so keyStart = 11, returns uri.substring(12) = "".

Test behaviour is as expected. I think AI was confused by the lack of docs. Fixed it now

Minor: extractKey(String) javadoc is missing
extractBucket(String) has a Javadoc with @throws. extractKey(String) has no Javadoc at all, even though it can also throw IllegalArgumentException for unsupported schemes. Inconsistent documentation.

Thanks for pointing it out. Added minimal Javadoc

spuru9 reviewed Sep 8, 2026

View reviewed changes

spuru9 approved these changes Sep 8, 2026

View reviewed changes

gaborgsomogyi reviewed Sep 8, 2026

View reviewed changes

Izeren reviewed Sep 8, 2026

View reviewed changes

Comment thread ...ilesystems/flink-s3-fs-native/src/test/java/org/apache/flink/fs/s3native/S3UriUtilsTest.java

Izeren reviewed Sep 8, 2026

View reviewed changes

Izeren approved these changes Sep 8, 2026

View reviewed changes

github-actions Bot added the community-reviewed-LGTM Applied if there are 2 non-committer approves on a PR. (The submitter cannot approve their own PR.) label Sep 8, 2026

gaborgsomogyi approved these changes Sep 10, 2026

View reviewed changes

github-actions Bot added community-reviewed PR has been reviewed by the community. and removed community-reviewed PR has been reviewed by the community. community-reviewed-LGTM Applied if there are 2 non-committer approves on a PR. (The submitter cannot approve their own PR.) labels Sep 10, 2026

Samrat002 force-pushed the FLINK-39115 branch from 51e8f99 to 760bacd Compare September 11, 2026 11:20

davidradl reviewed Sep 15, 2026

View reviewed changes

Samrat002 force-pushed the FLINK-39115 branch from 760bacd to 7f57fde Compare September 15, 2026 14:57

[FLINK-39115][s3] Support URI Handling Utility

78b073c

Samrat002 force-pushed the FLINK-39115 branch from 7f57fde to 78b073c Compare September 15, 2026 15:36

Uh oh!

Conversation

Samrat002 commented Sep 8, 2026

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Was generative AI tooling used to co-author this PR?

Uh oh!

flinkbot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

Samrat002 commented Sep 8, 2026

Uh oh!

spuru9 left a comment

Choose a reason for hiding this comment

Uh oh!

spuru9 Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Samrat002 Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gaborgsomogyi Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

davidradl Sep 15, 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!

Uh oh!

Izeren Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Izeren Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Izeren left a comment

Choose a reason for hiding this comment

Uh oh!

Samrat002 commented Sep 11, 2026

Uh oh!

Samrat002 commented Sep 11, 2026

Uh oh!

gaborgsomogyi commented Sep 11, 2026

Uh oh!

Samrat002 commented Sep 11, 2026

Uh oh!

Samrat002 commented Sep 15, 2026

Uh oh!

davidradl left a comment • 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!

Samrat002 commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

flinkbot commented Sep 8, 2026 •

edited

Loading

davidradl Sep 15, 2026 •

edited

Loading

davidradl left a comment •

edited

Loading