Addressing issue #30 about NIO FileSystem lifecycle

[stefanofornari]

Issue #, if available:
#30

Description of changes:
As per the discussion on #30 we want to implement NIO FileSystem semantic, which is lead by the methods newFileSystem() and getFileSystem(); the former shall return a new instance or throw an exception, while the latter should return an existing instance. This means that S3FileSystem instances shall be cached (as opposed to the current implementation that caches AWSClients).
Another change introduced and discussed in other thread is the use of S3ClientProvider at S3FileSystem level in order to better decouple file system logic from the aws client creation logic (discussed in #50). This obsoletes IMHO the methods in S3FileSystemProvider that receives a AWSAsyncClient, which I would remove in future version to keep S3FileSystemProvider cleaner and simpler.

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

PS: as often gradle complains, this time about javadoc I think :( it builds locally with both gradle and maven ...

[stefanofornari]

Hi @markjschreiber, do you mind looking at this one? It's probably the last one with remarkable architectural changes to the current codebase. After that we should be moving smoothly with the S3X provider.

[markjschreiber]

Thanks for putting in this PR. I have some changes requested but I think we can get there.

[stefanofornari]

thanks! I am going to work on it in the next days

[stefanofornari]

@markjschreiber I should be done with your comments, thanks a lot for your detailed review. I left open just one comment that was not clear to me. Let me know if I can do anything else.

[markjschreiber]

Hi, thanks for this update. I will be out of the office for the next week and with very limited connectivity. I will attempt to provide reviews as I am able.

[markjschreiber]

Looking good. A couple of minor comments and some suggestions on possible refactoring of some of the protected/ utility pieces.

[markjschreiber]

An alternative way to make this testable would be to refactor it out of this class and into a helper/ utility class as a public method. That will avoid this class being polluted with protected method that isn't central to the function of the class.

[stefanofornari]

I actually found the proper (I think) solution: cover the cases by using newDirectoryStream and getting the iterator from it:

Iterator<Path> pathIterator =
            provider.newDirectoryStream(Paths.get(URI.create(pathUri+"/")), filter).iterator();

HOWEVER, I was not able to make the test on pagination to work. I think I have done it quite right, but I miss how pagination is controlled. Can you please have a look at it (method pathIteratorForPublisher_withPagination)? For now I disabled it and added a TODO.

[stefanofornari]

Hi, thanks for this update. I will be out of the office for the next week and with very limited connectivity. I will attempt to provide reviews as I am able.

have fun!

[stefanofornari]

Hi, thanks for this update. I will be out of the office for the next week and with very limited connectivity. I will attempt to provide reviews as I am able.

have fun!

Hey @markjschreiber , I hope you had a good time! can we resume this work? The PR should be ready to be merged IMHO, but I am indeed available for updates.

[markjschreiber]

Apologies for the delay in the review. A few small changes requested but it should be quick to fix them.

[stefanofornari]

@markjschreiber, I should have done with everything. let me know.

[markjschreiber]

Why is the pagination test disabled?

[stefanofornari]

sorry @markjschreiber , I closed it by miskate... can you still see the changes and are you able to merge?

[markjschreiber]

Re-opening

[markjschreiber]

I just did some quick checks with the applications in examples. None of them currently work so something has broken in this PR for (at least) S3 native buckets/ URIs.

[stefanofornari]

uh... unexpected : ) can you give me a bucket I can try with?

[stefanofornari]

do you get the NPE: Exception in thread "main" java.lang.NullPointerException: Cannot invoke "software.amazon.nio.spi.s3.S3ClientProvider.generateAsyncClient(String) ?

[stefanofornari]

yes, there is a bug here I believe. I am working on it

[markjschreiber]

Yes, I get a null pointer exception for main

The other two example apps give "no such filesystem" errors. For testing purposes any of the buckets listed in the "registry of open data" are usually OK. I tend to use s3://broad-references/ or some text file therein.

[stefanofornari]

ok, I fixed the issue. I introduced couple of regressions not captured by unit tests. I tested it with sample applications and it works now.

[markjschreiber]

ok, I fixed the issue. I introduced couple of regressions not captured by unit tests. I tested it with sample applications and it works now.

Nice. If you haven't already can you open an issue(s) that outline the code parts/ scenarios where we need to add unit tests to spot similar issues in the future? It would be better to rely on automated testing rather than manually running the example apps.

[markjschreiber]

Looks great. Will squash and merge.

[stefanofornari]

ok, I fixed the issue. I introduced couple of regressions not captured by unit tests. I tested it with sample applications and it works now.

Nice. If you haven't already can you open an issue(s) that outline the code parts/ scenarios where we need to add unit tests to spot similar issues in the future? It would be better to rely on automated testing rather than manually running the example apps.

#165