S3 support #2
S3 support #2
Conversation
…d original implementations so that I can override them
…alues for root storage uri; handle the null root storage uri
… path or storage URI
…ntService; implemented two endpoints from VolumeStorageResource and fixed unit tests
…ys takes bucket as an arg so the path should no longer contain the bucket
…t; upgrade mongo driver
…iginalStoragePathURI
…ncy that added prefixes to be search even if they had greater depth than allowed
…r a single object
…ays using default which does not include anonymous provider; separated the code that creates sync and async clients
There was a problem hiding this comment.
I tried to poke around and find the most important changes but there are too many files to go through one by one. For large PRs it's helpful to have a high level overview about where to look.
None of my comments should prevent merging, they can be addressed later.
| private static final ConcurrentMap<String, S3Adapter> S3_ADAPTERS = new ConcurrentHashMap<>(); | ||
|
|
||
| public S3Adapter getS3Adapter(String bucket, String endpoint, JADEOptions s3Options) { | ||
| return S3_ADAPTERS.computeIfAbsent(bucket, (b) -> new S3Adapter(b, endpoint, s3Options)); |
There was a problem hiding this comment.
If you have a bucket with the same name in both AWS and VAST, this would only cache one of them. Why not key on endpoint+bucket?
There was a problem hiding this comment.
that's a good point - will do it
| return credentialsProvider; | ||
| } | ||
|
|
||
| private boolean isTryAnonymousFirst(JADEOptions s3Options) { |
There was a problem hiding this comment.
it would be nice to explain options like this. What does it mean to try anonymous first? Why would you want to set this? It's not documented here or in JADEOptions.
There was a problem hiding this comment.
This is something that should be removed in the end and leave the anonymous credentials always first in the chain of credentials. Being first is needed in order to access open bucket that do not require a signature but the AWSCRT async client fails when that is first in the list.
| private Path getContentPath(String contentLocation) { | ||
| Path contentPath = Paths.get(contentLocation); | ||
| if (Files.notExists(contentPath)) { | ||
| return null; |
There was a problem hiding this comment.
Why is this null, but the IOException causes an exception? Shouldn't it be FileNotFoundException or something?
There was a problem hiding this comment.
I don't know what is the best behaviour here. My option was to return an empty list which is in line with 'aws cli ls' does when you provide an invalid prefix
|
|
||
| private List<ContentNode> listContentFromPath(Path contentPath, ContentAccessParams contentAccessParams) { | ||
| if (contentPath == null) { | ||
| return Collections.emptyList(); |
There was a problem hiding this comment.
This seems dangerous. If the file doesn't exist, the content path is set to null and passed into this method. Then this method claims that a non existing path has no files. Not sure that makes sense. That means if you pass a non existing path to listContentNodes you will get an empty list which can be very misleading.
There was a problem hiding this comment.
I am changing it to throw the exception
| throw new ContentException("Content found at " + contentLocation + " is not a regular file"); | ||
| } | ||
| } else { | ||
| throw new NoContentFoundException("No object found at " + contentLocation); |
There was a problem hiding this comment.
Here file not found is treated correctly, as an exception. Why doesn't listContentNodes use the same pattern?
This is a large PR that implements support for accessing S3 buckets using AWS java2 SDK.