Upplication / Amazon-S3-FileSystem-NIO2

An S3 File System Provider for Java 7
MIT License
122 stars 67 forks source link

Using real AmazonS3Client #12

Closed zsoltessig closed 10 years ago

jarnaiz commented 10 years ago

Hi,

I review your commit, and some tests are failing :(. I resolve the issue #11 at the master branch, commit: https://github.com/jarnaiz/Amazon-S3-FileSystem-NIO2/commit/0786b9120899902de1d63b148dbcff7b3ab5cdf2 using your recommendation about amazonClient#listNextBatchOfObjects, thanks!

I read this http://stackoverflow.com/questions/6247018/how-do-you-test-code-written-against-aws-api and i decide continue using a custom class for now. https://github.com/jarnaiz/Amazon-S3-FileSystem-NIO2/blob/master/src/main/java/org/weakref/s3fs/AmazonS3Client.java

Any other comments or suggestions, will be happy to hear

zsoltessig commented 10 years ago

In this case, the best solution I think is if your AmazonS3Client implement the AmazonS3 interface, because what if somebody wants to use another method of the aws.AmazonS3Client?

zsoltessig commented 10 years ago

or make a method where we can access the real AmazonS3Client.

zsoltessig commented 10 years ago

Is it good for you, if I create a new pull request where I implement the AmazonS3 interface? I'll do this today. I think thats the best solution.

Because In our project we use aws.TransferManager to download an upload directories, and I'll need an AmazonS3 interface for that.

jarnaiz commented 10 years ago

Hello zsoltessig,

first of all, thanks for all your feedback :)

I don´t understand why you want to implement the amazonS3 interface. I use a custom class to expose only the desire methods to the fileSystem class and if I want to test the fileSystem class I only need to mock my amazons3 class and not all the interface.

If i need more methods, thats not a problem, the only thing i need is create the new method in my amazons3 class and use the real amazon s3 to implement. And, of course, review all my tests and mock the new method if is necessary.

cheers

zsoltessig commented 10 years ago

Hi. I need it because I want to use the TransferManager class too(uploading, downloading directories can be 10 times faster than with the S3FileSystemProvider). Where I need an AmazonS3 object for the constructor. I didn't want to recreate the aws.AmazonS3Client(because its a new connection? or I don't know, but we decided to not to make a new AmazonS3Client), and thats why I deleted your mock class for that, because i wanted to access the real AmazonS3 client.

jarnaiz commented 10 years ago

The purpose of this project is to implement Path and FileSystem with amazon s3 in background, expose the client its not a problem but i think is not his responsibility. I think is not a problem recreate the aws.AmazonS3Client but im not sure, im going to investigate this.

On the other hand if TransferManager is 10 times faster, we can replace some of the operations implemented with amazonS3Client with the TransferManager and improve the performance :)

Thanks for the suggestions.

zsoltessig commented 10 years ago

Just the directory uploading and downloading is faster. So in our project, if we want to access one file we use your S3FileSystemProvider because its very very comfortable, but when we want to download, or upload a directory, we use the TransferManager.

jarnaiz commented 10 years ago

What do you think if i add the option to pass the s3.AmazonS3 to the S3FileSystem with the credentials setted.

Something like this:

Map env = new HashMap(); env.put("client", amazonS3); FileSystems.newFIleSystem(uri, env);

You can share the same instance of s3.AmazonS3 to the S3FileSystem and your TransferManager.

I think is a option :)

zsoltessig commented 10 years ago

I like the idea.

jarnaiz commented 10 years ago

I will study it in depth in the issue #14