Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Third party S3 services support + #61 #168

Merged
merged 8 commits into from
Sep 12, 2023

Conversation

stefanofornari
Copy link
Contributor

@stefanofornari stefanofornari commented Sep 10, 2023

This pull request should finalize the work to seamlessly support 3rd party S3 services like minio. It brings three main improvements, which I kept together as they are small enough and they complement each other.

  1. All information to build a file system (bucket, endpoint, credentials, etc.) are now encapsulated in a S3NioConfiguration object that is passed to the constructor; this simplifies quite a bit the constructors and the need of accessors. This should also close File System settings #61

  2. Deprecation of constructors taking a URI: in the NIO world, the FileSystem object is usually create indirectly, by the provider or even more subtly by using Files or Path; it is therefore cleaner the provider digests the URIs creating a corresponding configuration object that can then be passed to S3FileSystem's constructor. This brings also the valuable advantage of making the file S3FileSystem completely independent by the URI used to create and configure it.

  3. Last but most importantly, it introduces a new file system provider, which manages the s3x:// URI schema as per the following URI pattern (as discussed earlier in other tickets and PRs):

    s3x://[key:secret#@]hostename[:port]/bucket/key

Thanks to bullet 1 above, it handed up to be quite clean, as the new S3XFileSystemProvider extends S3FileSystemProvider with very little code, while S3FileSystem is i completely in common between the two providers.

@markjschreiber , let me know what you think

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@markjschreiber markjschreiber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. There is one minor comment about some if statements that appear to not be indented but I think we are looking good.

@stefanofornari
Copy link
Contributor Author

fixed, thanks!

Copy link
Contributor

@markjschreiber markjschreiber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution and for adding support for alternative S3 providers. I will bump the minor version later this week and make a release to maven central.

@markjschreiber markjschreiber merged commit 17f58a4 into awslabs:main Sep 12, 2023
1 check passed
@stefanofornari
Copy link
Contributor Author

@markjschreiber do we have the new version on central?
Also, I have created a couple of minor "cleanup" PRs. Would you mind review and merge them as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File System settings
2 participants