Samrat002 commented on PR #29127:
URL: https://github.com/apache/flink/pull/29127#issuecomment-5682977354
Thank you @davidradl.
Please find the inline reply
> 1. 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.
> 2. 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)`
> 3. 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
