Add InMemoryBufferingS3OutputStream #400
Conversation
sgarfinkel
requested review from
eddumelendez,
maciejwalkowiak and
MatejNedic
as code owners
|
The failed security gate is complaining about the MD5 algorithm being used to generate a hash of the content... |
sgarfinkel
force-pushed
the
spring-cloud-aws-392
branch
from
1157f16
to
ef2af05
Compare
There was a problem hiding this comment.
Thanks @sgarfinkel for a great contribution! I agree that we should consider it making a default one.
Some things to polish:
S3ResourceIntegrationTests should be updated to also use InMemoryBufferingS3OutputStream among other output stream providers (as far as i can see one tests for InMemoryBufferingS3OutputStream is going to fail).
.../spring-cloud-aws-s3/src/main/java/io/awspring/cloud/s3/InMemoryBufferingS3OutputStream.java
Outdated
Show resolved
Hide resolved
- cover in memory stream with tests - fix setting content type
|
I did some polishing. Docs update still pending. @MatejNedic take a look please. |
maciejwalkowiak
added
component: s3
Thank you! I’ve had some personal things come up and have had very little time to do OSS work the past week. |
|
@sgarfinkel no worries, you've done great job already! btw, perhaps you can consider contributing it to aws-sdk-v2? (aws/aws-sdk-java-v2#3128) |
There was a problem hiding this comment.
Hey @sgarfinkel ,
Tnx on contribution. Looks good to me, only 1 thing I am thinking if we should introduce as well.
| Optional<S3ObjectContentTypeResolver> contentTypeResolver) { | ||
| return new DiskBufferingS3OutputStreamProvider(s3Client, | ||
| return new InMemoryBufferingS3OutputStreamProvider(s3Client, |
There was a problem hiding this comment.
We should provide a way that bufferSize can be set in properties.
There was a problem hiding this comment.
it can be easily defined by overwriting a bean. i am not sure how to structure it at this stage, we can add a property later when feedback like this comes.
github-actions
bot
added
the
type: documentation
|
SonarCloud Quality Gate failed.
|
📢 Type of change
📜 Description
Resolves #392 by adding a new InMemoryBufferingS3OutputStream which uses MultiPartUploads to stream large files to S3 without using the Filesystem.
💡 Motivation and Context
As discussed in the issue, this feature existed in the old version of spring-cloud-aws. It has been brought over in this new implementation, which although inspired by it, is entirely new.
💚 How did you test it?
There's a pretty extensive set of tests in the new
InMemoryBufferingS3OutputStreamTestsfile. Test coverage is quite good, at about 75% of lines. That 25% represents conditionals that log warnings and thrown exceptions and the like. All the business logic is fully covered.📝 Checklist
🔮 Next steps
Consider making this the default implementation used by the AutoConfiguration to backport the old functionality that used this same mechanism by default.