Issue 63: Upgrade to aws-sdk-java-v2 #83

ptirador · 2020-09-10T16:23:43Z

Pull Request Description

This pull request closes #63 .

Acceptance Test

Building the code with mvn clean install -Pintegration-tests still works.

Questions

Does this pull request break backward compatibility?
- Yes
- No
Does this pull request require other pull requests to be merged first?
- Yes, please see #...
- No
Does this require an update of the documentation?
- Yes, please see [provide details here]
- No

Test classes:

org.carlspring.cloud.storage.s3fs

org.carlspring.cloud.storage.s3fs.fileSystemProvider:

org.carlspring.cloud.storage.s3fs.path:

org.carlspring.cloud.storage.s3fs.spike

org.carlspring.cloud.storage.s3fs.util:

src/main/java/org/carlspring/cloud/storage/s3fs/S3FileSystemProvider.java

pom.xml

carlspring · 2020-09-11T16:55:33Z

@ptirado : This pull request might require re-working the examples in the wiki site that we're putting together in #22 / #70 .

carlspring · 2020-09-12T21:02:48Z

@ptirador : This page will also need to be updated once the documentation pull request is merged: docs/content/reference/configuration-options.md.

steve-todorov · 2020-09-12T21:47:48Z

Nice work @ptirador! :)
The new aws sdk v2 seems to provide async non-blocking client. Maybe we can also switch the nio implementation to use AsynchronousFileChannel? This should give a significant performance boost IMO. @carlspring maybe we should add this as a follow-up task? :)

carlspring · 2020-09-12T22:06:19Z

Nice work @ptirador! :)
The new aws sdk v2 seems to provide async non-blocking client. Maybe we can also switch the nio implementation to use AsynchronousFileChannel? This should give a significant performance boost IMO. @carlspring maybe we should add this as a follow-up task? :)

Yeah, we should investigate how we can benefit from the AsynchronousFileChannel. It would probably make more sense to add this in a follow-up task, because the migration to the new SDK is quite a bit of a change on its own, but, if @ptirador is up for it as part of this pull request, I'm fine with that as well. :)

steve-todorov · 2020-09-12T23:44:38Z

P.S. I've just merged some build improvements in the master branch. Please rebase to have faster build (including win and mac checks) :)

src/main/java/org/carlspring/cloud/storage/s3fs/S3AccessControlList.java

ptirador · 2020-09-13T09:25:44Z

Nice work @ptirador! :)
The new aws sdk v2 seems to provide async non-blocking client. Maybe we can also switch the nio implementation to use AsynchronousFileChannel? This should give a significant performance boost IMO. @carlspring maybe we should add this as a follow-up task? :)

Yeah, we should investigate how we can benefit from the AsynchronousFileChannel. It would probably make more sense to add this in a follow-up task, because the migration to the new SDK is quite a bit of a change on its own, but, if @ptirador is up for it as part of this pull request, I'm fine with that as well. :)

I agree. This should be created in a follow-up task.

ptirador · 2020-09-13T09:34:19Z

P.S. I've just merged some build improvements in the master branch. Please rebase to have faster build (including win and mac checks) :)

Rebase done 👍

src/test/java/org/carlspring/cloud/storage/s3fs/S3FileSystemTest.java

src/main/java/org/carlspring/cloud/storage/s3fs/S3Iterator.java

carlspring · 2020-09-15T18:37:44Z

src/main/java/org/carlspring/cloud/storage/s3fs/S3FileStore.java

+        // Originally getBucket was being used to determine presence of a bucket
+        //
+        // This is incorrect for two reasons:
+        // 1. The list bucket operation provides buckets for which you are the owner
+        //    It would not, therefore, allow you to work with buckets for which you
+        //    have access but are not the owner.
+        // 2. The way this information is being used later is to determine the
+        //    bucket owner, which by definition, is now "you".
+        // https://docs.aws.amazon.com/AmazonS3/latest/API/RESTServiceGET.html
+        //
+        // However, note that the revised code below now has a different permissions
+        // model as HeadBucket is now required


Are these notes coming from @elerch 's fork?

Yeah, some parts are from this fork that you mention. Could be helpful.

The ability to head the bucket is the true test that a bucket "exists" for the Principle as listing buckets only shows those that are in the account of the user whereas "head" will work if the bucket has been shared with the Principle from another account.

fixup! Issue-63: Socket Buffer Size Hint not supported in v2.

fixup! Issue-63: Fix tests.

…obtain rest of properties missed.

… fileChannel/seekableByteChannel creation. fixup! Issue-63: Add boolean parameter to use or not a temporary file in the fileChannel/seekableByteChannel creation.

fixup! Issue-63: Remove not valid case for URI.

ptirador · 2020-10-30T16:52:51Z

Nice work @ptirador! :)
The new aws sdk v2 seems to provide async non-blocking client. Maybe we can also switch the nio implementation to use AsynchronousFileChannel? This should give a significant performance boost IMO. @carlspring maybe we should add this as a follow-up task? :)

@carlspring, @steve-todorov : Follow-up task created: #99

carlspring · 2020-10-30T21:35:43Z

@ptirador ,

Thanks for your massive work on this pull request! As agreed, I will merge this into the master.

@elerch, @markjschreiber ,

Thanks for reviewing and for all your comments! We've reviewed them and have decided to handle them as separate follow-up tasks. Your guidance has been extremely valuable and much appreciated and we hope you can keep an eye on our future pull requests as well!

src/main/java/org/carlspring/cloud/storage/s3fs/S3Factory.java

src/main/java/org/carlspring/cloud/storage/s3fs/S3AccessControlList.java

src/main/java/org/carlspring/cloud/storage/s3fs/S3Factory.java

src/main/java/org/carlspring/cloud/storage/s3fs/S3Iterator.java

src/test/java/org/carlspring/cloud/storage/s3fs/S3ClientIT.java

src/test/java/org/carlspring/cloud/storage/s3fs/S3FileChannelTest.java

src/test/java/org/carlspring/cloud/storage/s3fs/S3FileStoreTest.java

ptirador · 2020-11-05T18:36:20Z

@carlspring: Your suggestions have been fixed.

carlspring · 2020-11-05T21:08:54Z

Awesome work, @ptirador !

Thanks again! :)

ptirador force-pushed the issue-63 branch from cce056a to bf66d08 Compare September 10, 2020 16:37

carlspring reviewed Sep 11, 2020

View reviewed changes

src/main/java/org/carlspring/cloud/storage/s3fs/S3FileSystemProvider.java Outdated Show resolved Hide resolved

carlspring reviewed Sep 11, 2020

View reviewed changes

src/main/java/org/carlspring/cloud/storage/s3fs/S3FileSystemProvider.java Outdated Show resolved Hide resolved

carlspring reviewed Sep 11, 2020

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

carlspring reviewed Sep 11, 2020

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

ptirador changed the title ~~Issue-63: update main code to SDK v2.~~ Issue-63: Upgrade to aws-sdk-java-v2 Sep 12, 2020

ptirador force-pushed the issue-63 branch 2 times, most recently from 5167c30 to 24ae70e Compare September 12, 2020 17:34

carlspring reviewed Sep 13, 2020

View reviewed changes

src/main/java/org/carlspring/cloud/storage/s3fs/S3AccessControlList.java Outdated Show resolved Hide resolved

ptirador force-pushed the issue-63 branch from 24ae70e to 5b8c4d4 Compare September 13, 2020 09:33

ptirador force-pushed the issue-63 branch 4 times, most recently from 4d01965 to 314dae5 Compare September 13, 2020 16:32

carlspring reviewed Sep 15, 2020

View reviewed changes

src/test/java/org/carlspring/cloud/storage/s3fs/S3FileSystemTest.java Outdated Show resolved Hide resolved

carlspring reviewed Sep 15, 2020

View reviewed changes

src/main/java/org/carlspring/cloud/storage/s3fs/S3Iterator.java Outdated Show resolved Hide resolved

carlspring reviewed Sep 15, 2020

View reviewed changes

ptirador force-pushed the issue-63 branch 3 times, most recently from ba80ea6 to fd91fc2 Compare September 21, 2020 15:55

ptirador force-pushed the issue-63 branch 3 times, most recently from b741167 to 92ea6db Compare September 28, 2020 17:26

ptirador added 2 commits October 19, 2020 19:37

Issue-63: Fix Sonar issues.

931dd2f

Issue-63: Fix the use of deprecated classes.

68e2e38

ptirador force-pushed the issue-63 branch from 4b8f6d7 to 8d9edd8 Compare October 19, 2020 17:37

ptirador added 8 commits October 24, 2020 12:40

Issue-63: Socket Buffer Size Hint not supported in v2.

7bf910e

fixup! Issue-63: Socket Buffer Size Hint not supported in v2.

Issue-63: Fix tests.

1a65049

fixup! Issue-63: Fix tests.

Issue-63: Fix S3Factory to obtain correctly region and protocol, and …

3c99dd1

…obtain rest of properties missed.

Issue-63: Revert replace usages to original replaceAll usages.

62c9bba

Issue-63: Refactor to declare S3Client in new line.

9200b5c

Issue-63: Add boolean parameter to use or not a temporary file in the…

0951348

… fileChannel/seekableByteChannel creation. fixup! Issue-63: Add boolean parameter to use or not a temporary file in the fileChannel/seekableByteChannel creation.

Issue-63: Handle integer and long parsing, and add radix.

830c1c9

Issue-63: Remove not valid case for URI.

c379e9a

fixup! Issue-63: Remove not valid case for URI.

ptirador force-pushed the issue-63 branch from 0cfe9c4 to c379e9a Compare October 24, 2020 11:00

ptirador mentioned this pull request Oct 30, 2020

Create a custom provider for AwsRegionProviderChain #100

Closed

5 tasks

carlspring reviewed Oct 30, 2020

View reviewed changes

src/main/java/org/carlspring/cloud/storage/s3fs/S3Factory.java Show resolved Hide resolved

ptirador mentioned this pull request Oct 31, 2020

Analyze options to use SdkHttpClient implementations #101

Open

1 task