-
-
Notifications
You must be signed in to change notification settings - Fork 327
ImmediateRetryAsyncErrorHandler changes msg visibility timeout to zero. (#1314) #1370
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
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.
Hey Bruno and Rafael, thanks for the PR!
I left a few comments, please let me know if they make sense to you or if you have any questions.
Thanks!
...-sqs/src/main/java/io/awspring/cloud/sqs/listener/errorhandler/AsyncDefaultErrorHandler.java
Outdated
Show resolved
Hide resolved
...-sqs/src/main/java/io/awspring/cloud/sqs/listener/errorhandler/AsyncDefaultErrorHandler.java
Outdated
Show resolved
Hide resolved
...-sqs/src/main/java/io/awspring/cloud/sqs/listener/errorhandler/AsyncDefaultErrorHandler.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
...-sqs/src/main/java/io/awspring/cloud/sqs/listener/errorhandler/AsyncDefaultErrorHandler.java
Outdated
Show resolved
Hide resolved
28e34c3
to
d65e453
Compare
Hey there Tomaz! Thank you for reviewing our PR. Your suggestions make a lot of sense and we have just commited the changes. Please let us know if there is anything else we could change/improve. |
d65e453
to
6eb4d7f
Compare
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.
Hey @rafaelcgpava, thanks for the adjustments!
This looks good, I've just left a couple more details for your consideration.
I'll also ask you to add this feature to the docs (sqs.adoc
), including how to set it up (just declare it as a bean for auto-configuration, or add to a Factory / Container directly).
Please let me know if you have any questions / suggestions.
...c/main/java/io/awspring/cloud/sqs/listener/errorhandler/ImmediateRetryAsyncErrorHandler.java
Outdated
Show resolved
Hide resolved
...t/java/io/awspring/cloud/sqs/listener/errorhandler/ImmediateRetryAsyncErrorHandlerTests.java
Outdated
Show resolved
Hide resolved
...t/java/io/awspring/cloud/sqs/listener/errorhandler/ImmediateRetryAsyncErrorHandlerTests.java
Outdated
Show resolved
Hide resolved
6eb4d7f
to
a84644f
Compare
a84644f
to
9bf1c2b
Compare
9bf1c2b
to
c9d96a9
Compare
Hey @tomazfernandes, thank you for the review! We've just pushed the requested changes. Please let us know if there's anything else we should revise or improve. 🎉 🚀 |
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.
Almost there @rafaelcgpava and @brun0-4ugusto!
c9d96a9
to
3053ed0
Compare
@tomazfernandes once again, thank you very much for your feedback and the suggestions you have been making! It has been a great learning experience! I´ve just commited the changes we've made from your suggestions =) I had a question about the note we´ve left saying that there was only the AsyncErrorHandler implementation for ImmediateRetry, however I checked that we adapt ErrorHandlers to AsyncErrorHandlers using BlockingErrorHandlerAdapter, and it makes a lot of sense to remove that note. |
Hi @tomazfernandes! I hope this message finds you well. I just wanted to kindly check if there are any changes needed on the PR — I’m available to assist with any updates you might require. |
📢 Type of change
📜 Description
New ImmediateRetryAsyncErrorHandler to change message visibility timeout to zero when an error occurs
💡 Motivation and Context
As mentioned in (#1313), it was not possible to immediatly report back an error to AWS SQS. This enhancement allows this feature to immediatly make the message available for retry by changing it's visibility timeout to zero.
See #1314
💚 How did you test it?
Unit test, Integration test, manual tests with a sample application
📝 Checklist
🔮 Next steps