-
Notifications
You must be signed in to change notification settings - Fork 263
New spring-batch-s3 module #182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Andrea Cioni <supercionci@hotmail.it>
b6e9643 to
e3f3ce1
Compare
dgray16
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work!
Here are some places that could be improved from my point of view.
...tch-s3/src/main/java/org/springframework/batch/extensions/s3/stream/S3MultipartUploader.java
Show resolved
Hide resolved
spring-batch-s3/src/test/java/org/springframework/batch/extensions/s3/S3ItemReaderTests.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/springframework/batch/extensions/s3/stream/S3MultipartOutputStreamTests.java
Outdated
Show resolved
Hide resolved
...3/src/test/java/org/springframework/batch/extensions/s3/stream/S3MultipartUploaderTests.java
Outdated
Show resolved
Hide resolved
...tch-s3/src/test/java/org/springframework/batch/extensions/s3/stream/S3OutputStreamTests.java
Outdated
Show resolved
Hide resolved
scordio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @andreacioni, thanks for the PR!
I suggest adding a CI job, see for example the one for Spring Batch Notion.
Also, the new module should be mentioned in the root README.
Signed-off-by: Andrea Cioni <supercionci@hotmail.it>
- Builders moved to a dedicated package - project README.adoc updated; - root README.md updated: add reference to new module; - use org.springframework.util.unit.DataSize to express data sizes; - removed apache-client exclusion in pom.xml to allow user to not specify an actual implementation in their dependencies - updated spring-javaformat-checkstyle to match the latest version - update gitignore to match the one of Spring Batch Notion - Add Maven Wrapper executables - Add Github Action workflow - Removed maven-wrapper.jar - Code cleanup Signed-off-by: Andrea Cioni <supercionci@hotmail.it>
- pom.xml reformatted - root README missing Amazon S3 link - atLeastOnce abandoned in favor of times(x) - minor corrections Signed-off-by: Andrea Cioni <supercionci@hotmail.it>
Signed-off-by: Andrea Cioni <supercionci@hotmail.it>
Signed-off-by: Andrea Cioni <supercionci@hotmail.it>
dgray16
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a review from @fmbenhassine also.
...ch-s3/src/main/java/org/springframework/batch/extensions/s3/builder/S3ItemWriterBuilder.java
Outdated
Show resolved
Hide resolved
spring-batch-s3/src/main/java/org/springframework/batch/extensions/s3/stream/Defaults.java
Outdated
Show resolved
Hide resolved
spring-batch-s3/src/test/java/org/springframework/batch/extensions/s3/S3ItemReaderTests.java
Show resolved
Hide resolved
Co-authored-by: Volodymyr <dgray16@users.noreply.github.com> Signed-off-by: Andrea Cioni <supercionci@hotmail.it>
…ions/s3/S3ItemReaderTests.java Co-authored-by: Volodymyr <dgray16@users.noreply.github.com> Signed-off-by: Andrea Cioni <supercionci@hotmail.it>
…ions/s3/builder/S3ItemWriterBuilder.java Co-authored-by: Volodymyr <dgray16@users.noreply.github.com> Signed-off-by: Andrea Cioni <supercionci@hotmail.it>
…ions/s3/stream/Defaults.java Co-authored-by: Volodymyr <dgray16@users.noreply.github.com> Signed-off-by: Andrea Cioni <supercionci@hotmail.it>
|
Any news on this? I don't think I'm alone in wanting this in. |
|
Hi @andreacioni Thank you for contributing this extension! I did not follow the SDK updates from AWS (the last time I checked was when I wrote spring-projects/spring-batch#3818 (comment)), but I have no objection to add this S3 extension if there they now provide the necessary bits to properly create an item reader and writer for Spring Batch. However, we need someone to lead and maintain this module after merging this PR. If you are interested, please reach out to me by email (my email is the one I use in git) to explain the responsibility of extension leads and the onboarding process to the Best regards, |
scordio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @andreacioni, I have a few follow-up comments, but feel free to ignore them until things are sorted out with @fmbenhassine.
| uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: 'temurin' | ||
| java-version: '21' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| java-version: '21' | |
| java-version: '25' |
| <parent> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-parent</artifactId> | ||
| <version>3.5.3</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <version>3.5.3</version> | |
| <version>4.0.0</version> |
| wrapperVersion=3.3.2 | ||
| distributionType=only-script | ||
| distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.8/apache-maven-3.9.8-bin.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time has passed and new versions are now available:
| wrapperVersion=3.3.2 | |
| distributionType=only-script | |
| distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.8/apache-maven-3.9.8-bin.zip | |
| wrapperVersion=3.3.4 | |
| distributionType=only-script | |
| distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.11/apache-maven-3.9.11-bin.zip |
These two properties are not enough for a proper upgrade, and the following should be executed locally instead:
./mnvw org.apache.maven.plugins:maven-wrapper-plugin:3.3.4:wrapper -Dmaven=3.9.11
| pull_request: | ||
| paths: | ||
| - 'spring-batch-s3/**' | ||
| push: | ||
| paths: | ||
| - 'spring-batch-s3/**' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pull_request: | |
| paths: | |
| - 'spring-batch-s3/**' | |
| push: | |
| paths: | |
| - 'spring-batch-s3/**' | |
| pull_request: | |
| paths: | |
| - '.github/workflows/spring-batch-s3.yml' | |
| - 'spring-batch-s3/**' | |
| push: | |
| paths: | |
| - '.github/workflows/spring-batch-s3.yml' | |
| - 'spring-batch-s3/**' |
| * memory. | ||
| */ | ||
|
|
||
| package org.springframework.batch.extensions.s3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the migration to Batch 6 and Boot 4, I'd also consider the Jspecify integration.
For this, you might take inspiration from what I've done at a9648a7.
Hello everyone,
after struggling a bit with S3 and Spring Batch I want to contribute to this ecosystem with my little knowledge by proposing this PR to your attention. In the past I've followed some discussion that lead me here (mainly #3818). I had to work with S3 in the past years and I had to implement such Reader/Writer each time, moreover it's a new addition to AWS SDK the API to upload objects with unknown size (reference) from streams. Given this inputs I decided that was time for Spring Batch to have some support for S3, hope this will be welcome.
More information are in the README, please let me know if there is something missing I'd be happy to fix it.