Add acknowledgement mode on @SqsListener annotation #870
Conversation
- Added missing JavaDoc for getAcknowledgementMode public method - Added test to ensure that new SqsListenerAcknowledgementMode enum will have the same values as the original AcknowledgementMode enum
github-actions
bot
added
component: sqs
jvcalassio
requested review from
maciejwalkowiak,
MatejNedic and
tomazfernandes
as code owners
jvcalassio
changed the title
There was a problem hiding this comment.
Thanks for the PR @jvcalassio, looks good.
Left a couple of comments, please let me know if they make sense to you.
...d-aws-sqs/src/main/java/io/awspring/cloud/sqs/annotation/SqsListenerAcknowledgementMode.java
Outdated
Show resolved
Hide resolved
...g-cloud-aws-sqs/src/main/java/io/awspring/cloud/sqs/listener/SqsContainerOptionsBuilder.java
Outdated
Show resolved
Hide resolved
spring-cloud-aws-sqs/src/test/java/io/awspring/cloud/sqs/integration/SqsIntegrationTests.java
Outdated
Show resolved
Hide resolved
- Change SqsListenerAcknowledgementMode from enum to class with const strings - Change typo on "acknowledgement" annotation field - Remove unnecessary field on SqsContainerOptionsBuilder - Move integration tests to its own test suite
- Update docs to reflect new "DEFAULT" annotation ack mode - Update comments with latest changes
|
Thanks for the comments @tomazfernandes. They all made a lot of sense, so I pushed a new version with the suggested changes. Let me know if you have any other suggestions |
There was a problem hiding this comment.
Hey @jvcalassio, thanks for the changes!
Left a couple other comments, let me know if they make sense to you.
spring-cloud-aws-sqs/src/main/java/io/awspring/cloud/sqs/annotation/SqsListener.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@jvcalassio not sure if you were done with the changes, just looking at them again.
Also please add your name as an author to the classes you changed.
Thanks!
spring-cloud-aws-sqs/src/main/java/io/awspring/cloud/sqs/annotation/SqsListener.java
Outdated
Show resolved
Hide resolved
...d-aws-sqs/src/main/java/io/awspring/cloud/sqs/annotation/SqsListenerAcknowledgementMode.java
Outdated
Show resolved
Hide resolved
- Remove "DEFAULT" annotation acknowledgement mode, use empty string as default - Update docs
- Add author on modified classes
Did it! I also put |
|
@jvcalassio seems there are some failing tests - would you mind taking a look? Thanks. |
Sorry! The condition for resolving the value was inverted (even my own tests were failing 🤦). I ran all the SQS tests locally and it seems to be fine now. |
📢 Type of change
📜 Description
This PR adds a parameter on the
@SqsListenerannotation that configures the acknowledgement mode for the created container. The default behavior is the same as it's now: uses the acknowledgement mode defined on the container factory. However, once the parameter is set, the factory value is overwritten in favor of the value defined on the annotation.💡 Motivation and Context
This feature was requested on issue #761. By having this feature, users coming from the 2.x.x versions will have a easier time migrating to 3.x.x, given that this parameter was used to configure the acknowledgement mode on the previous major version of the library.
💚 How did you test it?
📝 Checklist
🔮 Next steps